[webkit-reviews] review granted: [Bug 273152] AX: SVG g elements should only be AccessibilityRole::Group if they are focusable or have an accname : [Attachment 471092] Patch

[bugzilla-daemon at webkit.org]

chris fleizach <cfleizach at apple.com> has granted  review:
Bug 273152: AX: SVG g elements should only be AccessibilityRole::Group if they
are focusable or have an accname
https://bugs.webkit.org/show_bug.cgi?id=273152

Attachment 471092: Patch

https://bugs.webkit.org/attachment.cgi?id=471092&action=review




--- Comment #5 from chris fleizach <cfleizach at apple.com> ---
Comment on attachment 471092
  --> https://bugs.webkit.org/attachment.cgi?id=471092
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=471092&action=review

>>> Source/WebCore/accessibility/AXObjectCache.cpp:602
>>> +	     const auto& value = element.attributeWithDefaultARIA(attribute);
>> 
>> Can we use getAttributeTrimmed instead of making a new utility?
> 
> I thought about this too, but ultimately decided to just inline this lambda
for a few reasons:
>   1. getAttributeTrimmed both trims and simplifies whitespace (because that's
what its callers need) -- we only need to trim here
>   2. getAttributeTrimmed is a class method on AccessibilityObject, this is a
free-standing method, so there's no object to call it on
>   3. getAttributeTrimmed does a virtual function call to get element(), which
is unnecessary in some contexts (we already have an element)
> 
> But I could probably switch some things around to make this work if you feel
strongly about this one.

I guess its ok, its just weird that we have to have so many ways to trim
attributes

>>> Source/WebCore/accessibility/AccessibilitySVGElement.cpp:202
>>> +bool AccessibilitySVGElement::hasTitleOrDescChild() const
>> 
>> Can we avoid abbreviations in names?
> 
> "desc" is the element name, so I thought it might be right to match it. What
do you think?
> 
> https://developer.mozilla.org/en-US/docs/Web/SVG/Element/desc

I guess that's ok. can you add a comment to that effect what this does? If I'm
just reading it I don't know if this means it has a title or a descendant
child? or a title or a child named desc