Summary: | AX: @alt and bounds ignored when using img[src] points to an inaccessible SVG | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | James Craig <jcraig> | ||||||||
Component: | Accessibility | Assignee: | 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
James Craig
2016-08-09 13:11:18 PDT
See related discussion from bug 145263. Created attachment 418181 [details]
Patch
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 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 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 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. (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 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 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 Created attachment 418229 [details]
Patch
(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. (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. One new test failure https://ews-build.s3-us-west-2.amazonaws.com/macOS-Mojave-Release-WK1-Tests-EWS/r418229-26105/results.html Created attachment 418295 [details]
Patch
Committed r271796: <https://trac.webkit.org/changeset/271796> All reviewed patches have been landed. Closing bug and clearing flags on attachment 418295 [details]. *** Bug 202431 has been marked as a duplicate of this bug. *** |