Bug 273152 – AX: SVG g elements should only be AccessibilityRole::Group if they are focusable or have an accname

• Component: Accessibility
• Status: RESOLVED FIXED

[Tyler Wilcock]

...

[Radar WebKit Bug Importer]

<rdar://problem/126946719>

[Tyler Wilcock]

Created attachment 471092 [details]
Patch

[chris fleizach]

Comment on attachment 471092 [details]
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?

> Source/WebCore/accessibility/AccessibilitySVGElement.cpp:202
> +bool AccessibilitySVGElement::hasTitleOrDescChild() const

Can we avoid abbreviations in names?

[Tyler Wilcock]

(In reply to chris fleizach from comment #3)
> Comment on attachment 471092 [details]
> 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.

> > 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

[chris fleizach]

Comment on attachment 471092 [details]
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

[Tyler Wilcock]

Created attachment 471134 [details]
Patch

[Tyler Wilcock]

> >>> 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
OK yeah, I see your point now. I've changed the name to hasTitleOrDescriptionChild() and added a comment so that it's clear.

[EWS]

Committed 278024@main (9f05a02fc85a): <https://commits.webkit.org/278024@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 471134 [details].