|
|
Created:
5 years ago by Elly Fong-Jones Modified:
4 years, 11 months ago CC:
chromium-reviews, plundblad+watch_chromium.org, aboxhall+watch_chromium.org, nektar+watch_chromium.org, yuzo+watch_chromium.org, nektarios, je_julie, dmazzoni+watch_chromium.org, dtseng+watch_chromium.org, blink-reviews, dmazzoni Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement the modifications to the accessible name computation algorithm described by the SVG AAM, and change the default role of SVG roots from "unknown" to "group".
TEST=layout
BUG=566252
Committed: https://crrev.com/2d9269a857221f01a4510ecdb161a8f6fa63f7a2
Cr-Commit-Position: refs/heads/master@{#369410}
Patch Set 1 #Patch Set 2 : + AXSVGRoot override #
Total comments: 3
Patch Set 3 : implement computeAccessibilityIsIgnored #Patch Set 4 : Add role=group default for AXSVGRoot #
Total comments: 5
Patch Set 5 : Clone fieldset/legend code a little #
Total comments: 4
Patch Set 6 : Remember relatedObjects #
Total comments: 2
Patch Set 7 : nit for dmazzoni #
Messages
Total messages: 32 (11 generated)
aboxhall@chromium.org changed reviewers: + aboxhall@chromium.org
https://codereview.chromium.org/1525793002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp (right): https://codereview.chromium.org/1525793002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp:2375: Element* title = nullptr; Before this line, add: nameFrom = AXNameFromRelatedElement; if (nameSources) { nameSources->append(NameSource(*foundTextAlternative)); nameSources->last().type = nameFrom; nameSources->last().nativeSource = AXTextFromNativeHTMLTitleElement; // will need to update test expectation as well } Then you don't need line 2388 or 2391 - and you don't need line 2389 either because it's not an attribute, but a related element (details/summary and fieldset/legend use a similar pattern, see below). The reason for initializing the nameSource first up is that nameSources tracks _potential_ as well as actual name sources, so we add in the nameSource "optimistically" and then fill it in if we actually find something. And we set nameFrom at the same time because nameSources may be null (and the code will potentially early-out much sooner if it is), but nameFrom is used outside it. https://codereview.chromium.org/1525793002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/accessibility/AXSVGRoot.cpp (right): https://codereview.chromium.org/1525793002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/accessibility/AXSVGRoot.cpp:71: bool AXSVGRoot::computeAccessibilityIsIgnored(IgnoredReasons* reasons) const Let's revert this for this change, and use the role=group hack. We should raise a bug to figure out the right way to do this.
dmazzoni@chromium.org changed reviewers: + dmazzoni@chromium.org
https://codereview.chromium.org/1525793002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/accessibility/AXSVGRoot.cpp (right): https://codereview.chromium.org/1525793002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/accessibility/AXSVGRoot.cpp:71: bool AXSVGRoot::computeAccessibilityIsIgnored(IgnoredReasons* reasons) const On 2015/12/15 19:56:03, aboxhall wrote: > Let's revert this for this change, and use the role=group hack. We should raise > a bug to figure out the right way to do this. Why? What's wrong with keeping the SVG root element in the tree? It seems harmless to me.
On 2015/12/15 20:00:45, dmazzoni wrote: > https://codereview.chromium.org/1525793002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/modules/accessibility/AXSVGRoot.cpp (right): > > https://codereview.chromium.org/1525793002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/modules/accessibility/AXSVGRoot.cpp:71: bool > AXSVGRoot::computeAccessibilityIsIgnored(IgnoredReasons* reasons) const > On 2015/12/15 19:56:03, aboxhall wrote: > > Let's revert this for this change, and use the role=group hack. We should > raise > > a bug to figure out the right way to do this. > > Why? What's wrong with keeping the SVG root element in the tree? It seems > harmless to me. At the very least it should respect aria-hidden etc., right?
Oh, good point. Yeah, it should extend the existing algorithm rather than always returning false. On Tue, Dec 15, 2015 at 12:46 PM <aboxhall@chromium.org> wrote: > On 2015/12/15 20:00:45, dmazzoni wrote: > > > https://codereview.chromium.org/1525793002/diff/20001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/modules/accessibility/AXSVGRoot.cpp > > (right): > > > > https://codereview.chromium.org/1525793002/diff/20001/third_party/WebKit/Sour... > > third_party/WebKit/Source/modules/accessibility/AXSVGRoot.cpp:71: bool > > AXSVGRoot::computeAccessibilityIsIgnored(IgnoredReasons* reasons) const > > On 2015/12/15 19:56:03, aboxhall wrote: > > > Let's revert this for this change, and use the role=group hack. We > > should > > raise > > > a bug to figure out the right way to do this. > > > Why? What's wrong with keeping the SVG root element in the tree? It seems > > harmless to me. > > At the very least it should respect aria-hidden etc., right? > > https://codereview.chromium.org/1525793002/ > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to chromium-reviews+unsubscribe@chromium.org. > > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Oh, good point. Yeah, it should extend the existing algorithm rather than always returning false. On Tue, Dec 15, 2015 at 12:46 PM <aboxhall@chromium.org> wrote: > On 2015/12/15 20:00:45, dmazzoni wrote: > > > https://codereview.chromium.org/1525793002/diff/20001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/modules/accessibility/AXSVGRoot.cpp > > (right): > > > > https://codereview.chromium.org/1525793002/diff/20001/third_party/WebKit/Sour... > > third_party/WebKit/Source/modules/accessibility/AXSVGRoot.cpp:71: bool > > AXSVGRoot::computeAccessibilityIsIgnored(IgnoredReasons* reasons) const > > On 2015/12/15 19:56:03, aboxhall wrote: > > > Let's revert this for this change, and use the role=group hack. We > > should > > raise > > > a bug to figure out the right way to do this. > > > Why? What's wrong with keeping the SVG root element in the tree? It seems > > harmless to me. > > At the very least it should respect aria-hidden etc., right? > > https://codereview.chromium.org/1525793002/ > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to chromium-reviews+unsubscribe@chromium.org. > > -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
Description was changed from ========== svg title bug wip BUG= ========== to ========== Implement the modifications to the accessible name computation algorithm described by the SVG AAM, and change the default role of SVG roots from "unknown" to "group". TEST=layout BUG=566252 ==========
aboxhall, dmazzoni: ptal? the tests all pass now :)
https://codereview.chromium.org/1525793002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp (right): https://codereview.chromium.org/1525793002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp:2390: for (Element& element : ElementTraversal::descendantsOf(*(node()))) { Could this be simplified to Element* title = ElementTraversal:firstChild(*(node()), HasTagName(SVGNames::titleTag)) ? https://codereview.chromium.org/1525793002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp:2398: String text = title->innerText(); This should follow the same pattern used by fieldset/legend below, i.e. - get the AXObject for the title element - ensure we haven't already gotten its text alternative (e.g. if <title> had an aria-labelledby which referred back to either itself or an ancestor - I actually had a confusing crash for a little while because of this!) - call recursiveTextAlternative on the title AXObject - also, ensure that relatedObjects is correctly populated.
https://codereview.chromium.org/1525793002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp (right): https://codereview.chromium.org/1525793002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp:2390: for (Element& element : ElementTraversal::descendantsOf(*(node()))) { On 2016/01/06 17:04:07, aboxhall wrote: > Could this be simplified to > Element* title = ElementTraversal:firstChild(*(node()), > HasTagName(SVGNames::titleTag)) > ? Oooh, TIL! Thanks :D https://codereview.chromium.org/1525793002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp:2390: for (Element& element : ElementTraversal::descendantsOf(*(node()))) { On 2016/01/06 17:04:07, aboxhall wrote: > Could this be simplified to > Element* title = ElementTraversal:firstChild(*(node()), > HasTagName(SVGNames::titleTag)) > ? Done. https://codereview.chromium.org/1525793002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp:2398: String text = title->innerText(); On 2016/01/06 17:04:07, aboxhall wrote: > This should follow the same pattern used by fieldset/legend below, i.e. > - get the AXObject for the title element > - ensure we haven't already gotten its text alternative (e.g. if <title> had an > aria-labelledby which referred back to either itself or an ancestor - I actually > had a confusing crash for a little while because of this!) > - call recursiveTextAlternative on the title AXObject > - also, ensure that relatedObjects is correctly populated. Done.
https://codereview.chromium.org/1525793002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp (right): https://codereview.chromium.org/1525793002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp:2390: Element* title = ElementTraversal::firstChild( I just now thought to look at how SVGElement does this: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... Up to you if you want to change it! https://codereview.chromium.org/1525793002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp:2407: source.text = textAlternative; need to set source.relatedObjects here. ...yeah, I'm sorry this is so complicated :(
PTAL? https://codereview.chromium.org/1525793002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp (right): https://codereview.chromium.org/1525793002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp:2390: Element* title = ElementTraversal::firstChild( On 2016/01/07 18:55:26, aboxhall wrote: > I just now thought to look at how SVGElement does this: > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > Up to you if you want to change it! To do this I have to turn the Node into an SVGElement first, and it ends up being not that pretty, so I decided to leave it :) https://codereview.chromium.org/1525793002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp:2407: source.text = textAlternative; On 2016/01/07 18:55:26, aboxhall wrote: > need to set source.relatedObjects here. > > ...yeah, I'm sorry this is so complicated :( Done!
lgtm
The CQ bit was checked by ellyjones@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1525793002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1525793002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
dmazzoni: ptal? this needs owner review :)
lgtm https://codereview.chromium.org/1525793002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp (right): https://codereview.chromium.org/1525793002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp:2389: DCHECK(node()->isContainerNode()); This should be ASSERT
thanks :) https://codereview.chromium.org/1525793002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp (right): https://codereview.chromium.org/1525793002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp:2389: DCHECK(node()->isContainerNode()); On 2016/01/12 16:23:38, dmazzoni wrote: > This should be ASSERT Done.
The CQ bit was checked by ellyjones@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1525793002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1525793002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by ellyjones@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dmazzoni@chromium.org, aboxhall@chromium.org Link to the patchset: https://codereview.chromium.org/1525793002/#ps120001 (title: "nit for dmazzoni")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1525793002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1525793002/120001
Message was sent while issue was closed.
Description was changed from ========== Implement the modifications to the accessible name computation algorithm described by the SVG AAM, and change the default role of SVG roots from "unknown" to "group". TEST=layout BUG=566252 ========== to ========== Implement the modifications to the accessible name computation algorithm described by the SVG AAM, and change the default role of SVG roots from "unknown" to "group". TEST=layout BUG=566252 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Implement the modifications to the accessible name computation algorithm described by the SVG AAM, and change the default role of SVG roots from "unknown" to "group". TEST=layout BUG=566252 ========== to ========== Implement the modifications to the accessible name computation algorithm described by the SVG AAM, and change the default role of SVG roots from "unknown" to "group". TEST=layout BUG=566252 Committed: https://crrev.com/2d9269a857221f01a4510ecdb161a8f6fa63f7a2 Cr-Commit-Position: refs/heads/master@{#369410} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/2d9269a857221f01a4510ecdb161a8f6fa63f7a2 Cr-Commit-Position: refs/heads/master@{#369410} |