Bug 160705 – AX: @alt and bounds ignored when using img[src] points to an inaccessible SVG

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[James Craig]

Test case from Deque
http://a11yideas.com/testcode/mwp/iOSsvg.html

If you link an entirely inaccessible SVG (no accessible contents), WebKit should respect the @alt attribute from the linking HTML, use the outer bounds of the SVG canvas as the image bounds, and use the AXImage role instead of AXGroup.

[Radar WebKit Bug Importer]

<rdar://problem/27771579>

[James Craig]

See related discussion from bug 145263.

[Andres Gonzalez]

Created attachment 418181 [details]
Patch

[chris fleizach]

Comment on attachment 418181 [details]
Patch

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

> Source/WebCore/accessibility/AccessibilitySVGElement.cpp:78
> +    for (const auto& child : childrenOfType<SVGElement>(element)) {

the code block worries me. it seems like it's going to be a n! algorithm. for every child, you're going to call into this same method for every descendant. and then at the next level, you'll call that same method again

[chris fleizach]

Comment on attachment 418181 [details]
Patch

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

> Source/WebCore/accessibility/AccessibilitySVGElement.cpp:72
> +    // If the role or aria-label attributes are specified, this is accessible.

are there other attributes we need to check like "alt"

[Darin Adler]

Comment on attachment 418181 [details]
Patch

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

>> Source/WebCore/accessibility/AccessibilitySVGElement.cpp:78
>> +    for (const auto& child : childrenOfType<SVGElement>(element)) {
> 
> the code block worries me. it seems like it's going to be a n! algorithm. for every child, you're going to call into this same method for every descendant. and then at the next level, you'll call that same method again

Using descendantsOfType instead, only at the top level, should resolve that cleanly.

[Darin Adler]

Comment on attachment 418181 [details]
Patch

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

>>> Source/WebCore/accessibility/AccessibilitySVGElement.cpp:78
>>> +    for (const auto& child : childrenOfType<SVGElement>(element)) {
>> 
>> the code block worries me. it seems like it's going to be a n! algorithm. for every child, you're going to call into this same method for every descendant. and then at the next level, you'll call that same method again
> 
> Using descendantsOfType instead, only at the top level, should resolve that cleanly.

But on reflection, I don’t think it will be n! or n^2; even in this code every descendant is visited only once.

[chris fleizach]

(In reply to Darin Adler from comment #7)
> Comment on attachment 418181 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=418181&action=review
> 
> >>> Source/WebCore/accessibility/AccessibilitySVGElement.cpp:78
> >>> +    for (const auto& child : childrenOfType<SVGElement>(element)) {
> >> 
> >> the code block worries me. it seems like it's going to be a n! algorithm. for every child, you're going to call into this same method for every descendant. and then at the next level, you'll call that same method again
> > 
> > Using descendantsOfType instead, only at the top level, should resolve that cleanly.
> 
> But on reflection, I don’t think it will be n! or n^2; even in this code
> every descendant is visited only once.

yea I was thinking if this has to be called at every level of the tree for every element, we'd end up descending a lot

[Darin Adler]

Comment on attachment 418181 [details]
Patch

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

>>>>> Source/WebCore/accessibility/AccessibilitySVGElement.cpp:78
>>>>> +    for (const auto& child : childrenOfType<SVGElement>(element)) {
>>>> 
>>>> the code block worries me. it seems like it's going to be a n! algorithm. for every child, you're going to call into this same method for every descendant. and then at the next level, you'll call that same method again
>>> 
>>> Using descendantsOfType instead, only at the top level, should resolve that cleanly.
>> 
>> But on reflection, I don’t think it will be n! or n^2; even in this code every descendant is visited only once.
> 
> yea I was thinking if this has to be called at every level of the tree for every element, we'd end up descending a lot

Oh, yes this is O(descendants) and it’s *not* OK to call something like that for all elements.

Could be a real challenge to deal with that.

[chris fleizach]

Comment on attachment 418181 [details]
Patch

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

>>>>>> Source/WebCore/accessibility/AccessibilitySVGElement.cpp:78
>>>>>> +    for (const auto& child : childrenOfType<SVGElement>(element)) {
>>>>> 
>>>>> the code block worries me. it seems like it's going to be a n! algorithm. for every child, you're going to call into this same method for every descendant. and then at the next level, you'll call that same method again
>>>> 
>>>> Using descendantsOfType instead, only at the top level, should resolve that cleanly.
>>> 
>>> But on reflection, I don’t think it will be n! or n^2; even in this code every descendant is visited only once.
>> 
>> yea I was thinking if this has to be called at every level of the tree for every element, we'd end up descending a lot
> 
> Oh, yes this is O(descendants) and it’s *not* OK to call something like that for all elements.
> 
> Could be a real challenge to deal with that.

talking to Andres offline we determined this would only be called once and only on the root. he had some ideas on how to make that more clear in the code

[Andres Gonzalez]

Created attachment 418229 [details]
Patch

[Andres Gonzalez]

(In reply to chris fleizach from comment #10)
> Comment on attachment 418181 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=418181&action=review
> 
> >>>>>> Source/WebCore/accessibility/AccessibilitySVGElement.cpp:78
> >>>>>> +    for (const auto& child : childrenOfType<SVGElement>(element)) {
> >>>>> 
> >>>>> the code block worries me. it seems like it's going to be a n! algorithm. for every child, you're going to call into this same method for every descendant. and then at the next level, you'll call that same method again
> >>>> 
> >>>> Using descendantsOfType instead, only at the top level, should resolve that cleanly.
> >>> 
> >>> But on reflection, I don’t think it will be n! or n^2; even in this code every descendant is visited only once.
> >> 
> >> yea I was thinking if this has to be called at every level of the tree for every element, we'd end up descending a lot
> > 
> > Oh, yes this is O(descendants) and it’s *not* OK to call something like that for all elements.
> > 
> > Could be a real challenge to deal with that.
> 
> talking to Andres offline we determined this would only be called once and
> only on the root. he had some ideas on how to make that more clear in the
> code

I moved the hasAccessibleContent method to the AccessibilitySVGRoot class which makes it clearer that this method would be called only on the SVG root.
Also used the descendantsOfType function suggested by Darin that makes it a lot cleaner.
Thank you both very much for the feedback.

[Andres Gonzalez]

(In reply to chris fleizach from comment #5)
> Comment on attachment 418181 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=418181&action=review
> 
> > Source/WebCore/accessibility/AccessibilitySVGElement.cpp:72
> > +    // If the role or aria-label attributes are specified, this is accessible.
> 
> are there other attributes we need to check like "alt"

Investigating this. have to consult the ARIA and SVG accessibility specs.

[chris fleizach]

One new test failure

https://ews-build.s3-us-west-2.amazonaws.com/macOS-Mojave-Release-WK1-Tests-EWS/r418229-26105/results.html

[Andres Gonzalez]

Created attachment 418295 [details]
Patch

[EWS]

Committed r271796: <https://trac.webkit.org/changeset/271796>

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

[chris fleizach]

*** Bug 202431 has been marked as a duplicate of this bug. ***