Bug 160705

Summary: AX: @alt and bounds ignored when using img[src] points to an inaccessible SVG
Product: WebKit Reporter: James Craig <jcraig>
Component: AccessibilityAssignee: Andres Gonzalez <andresg_22>
Status: RESOLVED FIXED    
Severity: Normal CC: aboxhall, andresg_22, apinheiro, cfleizach, darin, dmazzoni, ews-watchlist, jdiggs, jorge.f, samuel_white, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description James Craig 2016-08-09 13:11:18 PDT
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.
Comment 1 Radar WebKit Bug Importer 2016-08-09 13:11:37 PDT
<rdar://problem/27771579>
Comment 2 James Craig 2016-08-09 13:18:28 PDT
See related discussion from bug 145263.
Comment 3 Andres Gonzalez 2021-01-22 14:32:55 PST
Created attachment 418181 [details]
Patch
Comment 4 chris fleizach 2021-01-22 14:38:18 PST
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
Comment 5 chris fleizach 2021-01-22 14:57:31 PST
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"
Comment 6 Darin Adler 2021-01-22 16:04:19 PST
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.
Comment 7 Darin Adler 2021-01-22 16:18:17 PST
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.
Comment 8 chris fleizach 2021-01-22 16:23:45 PST
(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
Comment 9 Darin Adler 2021-01-22 16:29:54 PST
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.
Comment 10 chris fleizach 2021-01-22 17:19:35 PST
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
Comment 11 Andres Gonzalez 2021-01-23 11:28:10 PST
Created attachment 418229 [details]
Patch
Comment 12 Andres Gonzalez 2021-01-23 11:33:02 PST
(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.
Comment 13 Andres Gonzalez 2021-01-23 11:36:16 PST
(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.
Comment 15 Andres Gonzalez 2021-01-25 07:57:49 PST
Created attachment 418295 [details]
Patch
Comment 16 EWS 2021-01-25 08:52:57 PST
Committed r271796: <https://trac.webkit.org/changeset/271796>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 418295 [details].
Comment 17 chris fleizach 2021-03-15 12:31:45 PDT
*** Bug 202431 has been marked as a duplicate of this bug. ***