Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1917943002:
Accessible name calc should allow aria-labelledby to reference aria-hidden (Closed)
|
Created:
4 years, 8 months ago by dmazzoni Modified:
4 years, 8 months ago CC:
chromium-reviews, aboxhall, aboxhall+watch_chromium.org, nektar+watch_chromium.org, yuzo+watch_chromium.org, haraken, 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. |
DescriptionAccessible name calc should allow aria-labelledby to reference aria-hidden
The new accessible name calculation improved compatibility overall but broke
one specific case, where one element uses aria-labelledby (or aria-describedby)
to reference a subtree that's explicitly aria-hidden. The correct thing to
do here is ignore aria-hidden when an element is referenced explicitly.
<input aria-labelledby="label">
<div id="label" aria-hidden="true">Hidden label</div>
BUG=595494
Committed: https://crrev.com/8dacc7a16ffdee2d4af5a60f6f9079c22fd7399c
Cr-Commit-Position: refs/heads/master@{#389798}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Add more tests #
Messages
Total messages: 21 (4 generated)
dmazzoni@chromium.org changed reviewers: + aboxhall@chromium.org, nektar@chromium.org
https://codereview.chromium.org/1917943002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/accessibility/name-calc-aria-hidden.html (right): https://codereview.chromium.org/1917943002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/accessibility/name-calc-aria-hidden.html:26: }, "Aria-labelledby can get accessible text from aria-hidden subtree"); For completeness, could we have a test where aria-labelledby refers to an element within an aria-hidden subtree? e.g. <div class="container"> <h3 id="heading3" aria-hidden="true"> Before <p id="hidden3">Text within hidden subtree</p> After </h3> <button id="button3" aria-labelledby="hidden3"></button> </div> test(function(t) { var axHeading3 = accessibilityController.accessibleElementById("heading1"); assert_equals(???); // I assume you end up with null here, but not sure var axButton3 = accessibilityController.accessibleElementById("button3"); assert_equals(axButton3.name, "Hidden text"); }, "Aria-labelledby can get accessible text from within aria-hidden subtree"); https://codereview.chromium.org/1917943002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp (right): https://codereview.chromium.org/1917943002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp:1658: continue; Does this have any effect on <label> elements?
https://codereview.chromium.org/1917943002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/accessibility/name-calc-aria-hidden.html (right): https://codereview.chromium.org/1917943002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/accessibility/name-calc-aria-hidden.html:26: }, "Aria-labelledby can get accessible text from aria-hidden subtree"); On 2016/04/25 18:21:25, aboxhall wrote: > For completeness, could we have a test where aria-labelledby refers to an > element within an aria-hidden subtree? e.g. Sure, done. See below: > <div class="container"> > <h3 id="heading3" aria-hidden="true"> > Before > <p id="hidden3">Text within hidden subtree</p> > After > </h3> > <button id="button3" aria-labelledby="hidden3"></button> > </div> > > test(function(t) { > var axHeading3 = accessibilityController.accessibleElementById("heading1"); > assert_equals(???); // I assume you end up with null here, but not sure > var axButton3 = accessibilityController.accessibleElementById("button3"); > assert_equals(axButton3.name, "Hidden text"); > }, "Aria-labelledby can get accessible text from within aria-hidden subtree"); The first test will be that axHeading3 is undefined because you can't get the accessible element for something that's hidden, even though it can participate in a name calculation. https://codereview.chromium.org/1917943002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp (right): https://codereview.chromium.org/1917943002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp:1658: continue; On 2016/04/25 18:21:25, aboxhall wrote: > Does this have any effect on <label> elements? Yes, it looks like it does. A label element with aria-hidden=true on it will now be excluded from the accessibility tree but still used as part of the name calculation for the form control it labels. I added a new test to show this. Does this seem reasonable or should I restrict the logic to only do this for IDREFs like aria-labelledby and aria-describedby?
https://codereview.chromium.org/1917943002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp (right): https://codereview.chromium.org/1917943002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp:1658: continue; On 2016/04/25 22:33:03, dmazzoni wrote: > On 2016/04/25 18:21:25, aboxhall wrote: > > Does this have any effect on <label> elements? > > Yes, it looks like it does. A label element with aria-hidden=true on it will now > be excluded from the accessibility tree but still used as part of the name > calculation for the form control it labels. > > I added a new test to show this. > > Does this seem reasonable or should I restrict the logic to only do this for > IDREFs like aria-labelledby and aria-describedby? My understanding is that only aria-{labelledby, describedby} get this magic, per 2A. Since we already track inAriaLabelledByTraversal (hopefully that gets set for aria-describedby as well - perhaps a single extra test for aria-describedby?) it should be pretty straightforward to thread that through here, I believe?
https://codereview.chromium.org/1917943002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp (right): https://codereview.chromium.org/1917943002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp:1658: continue; On 2016/04/25 22:43:42, aboxhall wrote: > On 2016/04/25 22:33:03, dmazzoni wrote: > > On 2016/04/25 18:21:25, aboxhall wrote: > > > Does this have any effect on <label> elements? > > > > Yes, it looks like it does. A label element with aria-hidden=true on it will > now > > be excluded from the accessibility tree but still used as part of the name > > calculation for the form control it labels. > > > > I added a new test to show this. > > > > Does this seem reasonable or should I restrict the logic to only do this for > > IDREFs like aria-labelledby and aria-describedby? > > My understanding is that only aria-{labelledby, describedby} get this magic, per > 2A. Since we already track inAriaLabelledByTraversal (hopefully that gets set > for aria-describedby as well - perhaps a single extra test for > aria-describedby?) it should be pretty straightforward to thread that through > here, I believe? Here's the text of 2A: If the current node is hidden and is not referenced by aria-labelledby or aria-describedby, nor referenced by a native host language text alternative element or attribute, return the empty string. That second part, "referenced by a native host language text alternative element or attribute", covers things like <label> too. I agree it's not clear but I checked with Bryan and Rich and they agreed with that. I think the only purpose of inAriaLabelledBy in our code should be to make sure we don't keep following aria-labelledby more than once. The visibility part is more straightforward and doesn't require keeping track of it, rather when computing the accessible name of X, you don't worry about whether X itself is visible or not (since you assume that if this function is being called, the accessible name of X is interesting to someone). So you just compute the accessible name of X, but when recursing into descendants you check their visibility.
On 2016/04/26 15:39:32, dmazzoni wrote: > https://codereview.chromium.org/1917943002/diff/1/third_party/WebKit/Source/m... > File third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp (right): > > https://codereview.chromium.org/1917943002/diff/1/third_party/WebKit/Source/m... > third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp:1658: continue; > On 2016/04/25 22:43:42, aboxhall wrote: > > On 2016/04/25 22:33:03, dmazzoni wrote: > > > On 2016/04/25 18:21:25, aboxhall wrote: > > > > Does this have any effect on <label> elements? > > > > > > Yes, it looks like it does. A label element with aria-hidden=true on it will > > now > > > be excluded from the accessibility tree but still used as part of the name > > > calculation for the form control it labels. > > > > > > I added a new test to show this. > > > > > > Does this seem reasonable or should I restrict the logic to only do this for > > > IDREFs like aria-labelledby and aria-describedby? > > > > My understanding is that only aria-{labelledby, describedby} get this magic, > per > > 2A. Since we already track inAriaLabelledByTraversal (hopefully that gets set > > for aria-describedby as well - perhaps a single extra test for > > aria-describedby?) it should be pretty straightforward to thread that through > > here, I believe? > > Here's the text of 2A: > > If the current node is hidden and is not referenced by aria-labelledby or > aria-describedby, nor referenced by a native host language text alternative > element or attribute, return the empty string. > > That second part, "referenced by a native host language text alternative element > or attribute", covers things like <label> too. > > I agree it's not clear but I checked with Bryan and Rich and they agreed with > that. > > I think the only purpose of inAriaLabelledBy in our code should be to make sure > we don't keep following aria-labelledby more than once. > > The visibility part is more straightforward and doesn't require keeping track of > it, rather when computing the accessible name of X, you don't worry about > whether X itself is visible or not (since you assume that if this function is > being called, the accessible name of X is interesting to someone). So you just > compute the accessible name of X, but when recursing into descendants you check > their visibility. Oh I see, makes sense.
aboxhall@google.com changed reviewers: + aboxhall@google.com
lgtm
The CQ bit was checked by dmazzoni@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1917943002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1917943002/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Accessible name calc should allow aria-labelledby to reference aria-hidden The new accessible name calculation improved compatibility overall but broke one specific case, where one element uses aria-labelledby (or aria-describedby) to reference a subtree that's explicitly aria-hidden. The correct thing to do here is ignore aria-hidden when an element is referenced explicitly. <input aria-labelledby="label"> <div id="label" aria-hidden="true">Hidden label</div> BUG=595494 ========== to ========== Accessible name calc should allow aria-labelledby to reference aria-hidden The new accessible name calculation improved compatibility overall but broke one specific case, where one element uses aria-labelledby (or aria-describedby) to reference a subtree that's explicitly aria-hidden. The correct thing to do here is ignore aria-hidden when an element is referenced explicitly. <input aria-labelledby="label"> <div id="label" aria-hidden="true">Hidden label</div> BUG=595494 Committed: https://crrev.com/8dacc7a16ffdee2d4af5a60f6f9079c22fd7399c Cr-Commit-Position: refs/heads/master@{#389798} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/8dacc7a16ffdee2d4af5a60f6f9079c22fd7399c Cr-Commit-Position: refs/heads/master@{#389798}
Message was sent while issue was closed.
1. What if the target of an aria-labelledby or aria-describedby is a subtree that has nodes that are aria-hidden="true" which in turn have nodes that have aria-hidden="false" inside them? Would the code work? <div id="label"> <div aria-hidden="true"> <p aria-hidden="false"> label </p> </div> </div> 2. What if the subtree defining the label has some nodes with display:none? Should they also be ignored?
Message was sent while issue was closed.
On Tue, Apr 26, 2016 at 9:08 AM <nektar@chromium.org> wrote: > 1. What if the target of an aria-labelledby or aria-describedby is a > subtree > that has nodes that are aria-hidden="true" which in turn have nodes that > have > aria-hidden="false" inside them? > As soon as you get to a subtree that has aria-hidden="true" on it you stop recursing. 2. What if the subtree defining the label has some nodes with display:none? > Should they also be ignored? > Yes, and we already have tests for this and for visibility:hidden too. We just didn't have any tests for aria-hidden. -- 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.
Message was sent while issue was closed.
On Tue, Apr 26, 2016 at 9:08 AM <nektar@chromium.org> wrote: > 1. What if the target of an aria-labelledby or aria-describedby is a > subtree > that has nodes that are aria-hidden="true" which in turn have nodes that > have > aria-hidden="false" inside them? > As soon as you get to a subtree that has aria-hidden="true" on it you stop recursing. 2. What if the subtree defining the label has some nodes with display:none? > Should they also be ignored? > Yes, and we already have tests for this and for visibility:hidden too. We just didn't have any tests for aria-hidden. -- 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.
Message was sent while issue was closed.
2. What if the subtree defining the label has some nodes with display:none? > > Should they also be ignored? > > > Yes, and we already have tests for this and for visibility:hidden too. > We just didn't have any tests for aria-hidden. > I thought that when calculating the accessible name, this change only checks for the aria-hidden attribute and ignores a child only if this attribute is set to true but doesn't ignore it in other cases, such as when its CSS display attribute is set to none. -- 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.
Message was sent while issue was closed.
2. What if the subtree defining the label has some nodes with display:none? > > Should they also be ignored? > > > Yes, and we already have tests for this and for visibility:hidden too. > We just didn't have any tests for aria-hidden. > I thought that when calculating the accessible name, this change only checks for the aria-hidden attribute and ignores a child only if this attribute is set to true but doesn't ignore it in other cases, such as when its CSS display attribute is set to none. -- 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.
Message was sent while issue was closed.
On Tue, Apr 26, 2016 at 9:24 AM 'Nektarios Paisios' via Chromium-reviews < chromium-reviews@chromium.org> wrote: > 2. What if the subtree defining the label has some nodes with display:none? > > Should they also be ignored? >> > > Yes, and we already have tests for this and for visibility:hidden too. We > just didn't have any tests for aria-hidden. > > I thought that when calculating the accessible name, this change only > checks for the aria-hidden attribute and ignores a child only if this > attribute is set to true but doesn't ignore it in other cases, such as when > its CSS display attribute is set to none. > The check for display:none and visibility:hidden is there too, just in a slightly different part of the code. See LayoutTests/accessibility/name-calc-visibility.html for most of those tests, and see AXObject::isHiddenForTextAlternativeCalculation for the code that checks visibility:hidden and display:none. -- 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.
Message was sent while issue was closed.
On Tue, Apr 26, 2016 at 9:24 AM 'Nektarios Paisios' via Chromium-reviews < chromium-reviews@chromium.org> wrote: > 2. What if the subtree defining the label has some nodes with display:none? > > Should they also be ignored? >> > > Yes, and we already have tests for this and for visibility:hidden too. We > just didn't have any tests for aria-hidden. > > I thought that when calculating the accessible name, this change only > checks for the aria-hidden attribute and ignores a child only if this > attribute is set to true but doesn't ignore it in other cases, such as when > its CSS display attribute is set to none. > The check for display:none and visibility:hidden is there too, just in a slightly different part of the code. See LayoutTests/accessibility/name-calc-visibility.html for most of those tests, and see AXObject::isHiddenForTextAlternativeCalculation for the code that checks visibility:hidden and display:none. -- 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. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
