When aria-labelledby specifies self, only the contents of the labeling element is used. According to the "Text Alternative Computation" (http://www.w3.org/WAI/PF/aria/complete#textalternativecomputation) several other label sources need to be respected to make self-referencing labeling work as expected. <rdar://problem/10791187>
<rdar://problem/10791187>
A concrete example is: <a aria-label="YES" aria-labelledby="e1" href="#" id="e1">NO</a> The alternativeText() method on AccessibilityNodeObject should return YES here rather than NO.
Created attachment 210688 [details] Patch. Added additional 'Text Alternative Computation' steps to make self-referencing work as expected.
Attachment 210688 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/platform/mac/accessibility/self-referencing-aria-labelledby-expected.txt', u'LayoutTests/platform/mac/accessibility/self-referencing-aria-labelledby.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/accessibility/AccessibilityNodeObject.cpp']" exit_code: 1 Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1763: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Total errors found: 1 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 210689 [details] Patch. Fixed style.
Comment on attachment 210689 [details] Patch. View in context: https://bugs.webkit.org/attachment.cgi?id=210689&action=review > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1742 > + if (axObject->isAccessibilityNodeObject()) { we should make textUnderElement() virtual so that we don't have to check whether the object is a node object and can just call it directly
Created attachment 210765 [details] Patch. Updated per review. Thanks for the feedback Chris.
Comment on attachment 210765 [details] Patch. View in context: https://bugs.webkit.org/attachment.cgi?id=210765&action=review > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1725 > + if (!node->isHTMLElement()) > + return String(); Text nodes used to return their text. Why was this removed? > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1729 > + const AtomicString& ariaLabel = element->getAttribute(aria_labelAttr); These can be fastGetAttribute. The only time you can’t call fastGetAttribute is when the attribute is an attribute that SVG can animate or a style attribute. > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:-1748 > - for (Node* n = idElement->firstChild(); n; n = NodeTraversal::next(n, idElement)) > - builder.append(accessibleNameForNode(n)); What makes it OK to remove this loop? Nothing in change log mentions that. > Source/WebCore/ChangeLog:11 > + Test: platform/mac/accessibility/self-referencing-aria-labelledby.html I don’t see any coverage for the text node parts of this patch in this test.
(In reply to comment #8) > (From update of attachment 210765 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=210765&action=review > > > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1725 > > + if (!node->isHTMLElement()) > > + return String(); > > Text nodes used to return their text. Why was this removed? The removal of text nodes here was intended to bring us closer to a strict implementation of the text alt computation steps mentioned above. Because those steps expect html elements, I've updated this method to mirror that. Previously, we would gather labeler nodes, and iterate their text nodes which meant we were ignoring other alt text comp rules like aria_label should beat out contents. This change adds additional considerations such as aria_label and title to label generation. Text contents are now handled via textUnderElement. In summary, our previous method only considered steps 2A.3 (alt text) and 2C (content text). This change adds additional support for steps 2A.2 (aria_label) and 2D (title) as well as slightly better support for step 2c (content text) by using textUnderElement in place of text node iteration. > > > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1729 > > + const AtomicString& ariaLabel = element->getAttribute(aria_labelAttr); > > These can be fastGetAttribute. The only time you can’t call fastGetAttribute is when the attribute is an attribute that SVG can animate or a style attribute. Will update, thanks. > > > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:-1748 > > - for (Node* n = idElement->firstChild(); n; n = NodeTraversal::next(n, idElement)) > > - builder.append(accessibleNameForNode(n)); > > What makes it OK to remove this loop? Nothing in change log mentions that. This loop was removed because we no longer need to iterate individual text nodes. They are instead handled via textUnderElement as mentioned above. While doing the alt text comp if we make it to step 2C we use textUnderElement to gather inner text information rather than doing a simple iteration of text nodes. > > > Source/WebCore/ChangeLog:11 > > + Test: platform/mac/accessibility/self-referencing-aria-labelledby.html > > I don’t see any coverage for the text node parts of this patch in this test. Test 1 was designed to make sure content labels still worked. I assumed that since the existing aria-labelledby tests in LayoutTests/accessibility still passed we were safe. Any thoughts on what additional tests we could add to cover your concern. Thanks for the great feedback.
Created attachment 210823 [details] Patch. Updated logs and attribute fetching per review. Thanks.
Comment on attachment 210823 [details] Patch. View in context: https://bugs.webkit.org/attachment.cgi?id=210823&action=review The patch looks quite good already, Just adding some comments below... > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:-1730 > - if (isHTMLInputElement(node)) > - return toHTMLInputElement(node)->value(); I'm not sure whether this removal is correct or not, since I can't see it covered in the new additions. Also, it might be interesting to test that everything keeps working as expected for input elements as well > LayoutTests/ChangeLog:11 > + * platform/mac/accessibility/self-referencing-aria-labelledby-expected.txt: Added. > + * platform/mac/accessibility/self-referencing-aria-labelledby.html: Added. I might be wrong, but I believe this new test could be perfectly placed in the general LayoutTests/accessibility directory, since it's a generic enough behaviour that should work fine in other platforms like GTK/EFL (maybe they will need some adapted results, but the test itself should be fine)
Created attachment 211059 [details] Patch. Added additional input element tests and moved test to LayoutTests/accessibility.
(In reply to comment #11) > (From update of attachment 210823 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=210823&action=review > > The patch looks quite good already, Just adding some comments below... Thanks for the feedback! > > > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:-1730 > > - if (isHTMLInputElement(node)) > > - return toHTMLInputElement(node)->value(); > > I'm not sure whether this removal is correct or not, since I can't see it covered in the new additions. This bit covers the input element value case you mentioned. + const AtomicString& value = element->fastGetAttribute(valueAttr); + if (!value.isEmpty()) + return value; > > Also, it might be interesting to test that everything keeps working as expected for input elements as well Good point. Added input element tests including value check mentioned above. > > > LayoutTests/ChangeLog:11 > > + * platform/mac/accessibility/self-referencing-aria-labelledby-expected.txt: Added. > > + * platform/mac/accessibility/self-referencing-aria-labelledby.html: Added. > > I might be wrong, but I believe this new test could be perfectly placed in the general LayoutTests/accessibility directory, since it's a generic enough behaviour that should work fine in other platforms like GTK/EFL (maybe they will need some adapted results, but the test itself should be fine) I've moved the test here. Will see how the build bot responds and adapt results if needed. Thank again.
Comment on attachment 211059 [details] Patch. View in context: https://bugs.webkit.org/attachment.cgi?id=211059&action=review > LayoutTests/accessibility/self-referencing-aria-labelledby.html:28 > +<input aria-label="X" aria-labelledby="t6 s0 s1" class="test" id="t6" type="button" value="Y"> is this input field pulling the Y value from s0 or from the value?
(In reply to comment #14) > (From update of attachment 211059 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=211059&action=review > > > LayoutTests/accessibility/self-referencing-aria-labelledby.html:28 > > +<input aria-label="X" aria-labelledby="t6 s0 s1" class="test" id="t6" type="button" value="Y"> > > is this input field pulling the Y value from s0 or from the value? s0 and s1 get applied to all tests. So if the Y came from value we would generate a label of Y Y Z which would fail. Every test should result in a label of X Y Z. So this test makes sure aria_label takes precedence over value.
(In reply to comment #15) > (In reply to comment #14) > > (From update of attachment 211059 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=211059&action=review > > > > > LayoutTests/accessibility/self-referencing-aria-labelledby.html:28 > > > +<input aria-label="X" aria-labelledby="t6 s0 s1" class="test" id="t6" type="button" value="Y"> > > > > is this input field pulling the Y value from s0 or from the value? > > s0 and s1 get applied to all tests. So if the Y came from value we would generate a label of Y Y Z which would fail. Every test should result in a label of X Y Z. > > So this test makes sure aria_label takes precedence over value. We should probably make those cases use a different value like <input value='A'> so that if something that like that happens 1) it's very clear 2) we won't run the risk of s0 not applying and value applying, and the test NOT failing
Created attachment 211060 [details] Patch. Updated test to make failures easier to spot. Attributes that should "lose" during name computation are now question marks.
(In reply to comment #13) > [..] > Thanks for the feedback! No problem. Happy to help. > > > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:-1730 > > > - if (isHTMLInputElement(node)) > > > - return toHTMLInputElement(node)->value(); > > > > I'm not sure whether this removal is correct or not, since I > > can't see it covered in the new additions. > > This bit covers the input element value case you mentioned. > > + const AtomicString& value = element->fastGetAttribute(valueAttr); > + if (!value.isEmpty()) > + return value; I'm not sure about it, since HTMLInputElement::value() does some more things besides returning the content of the 'value' attribute, such as sanitize the value: String HTMLInputElement::value() const { String value; if (m_inputType->getTypeSpecificValue(value)) return value; value = m_valueIfDirty; if (!value.isNull()) return value; AtomicString valueString = fastGetAttribute(valueAttr); value = sanitizeValue(valueString); if (!value.isNull()) return value; return m_inputType->fallbackValue(); } Perhaps it will be worth it to keep a dedicated if branch for that specific situation? > > Also, it might be interesting to test that everything keeps working > > as expected for input elements as well > > Good point. Added input element tests including value check mentioned above. Thanks for considering it. > I've moved the test here. Will see how the build bot responds and > adapt results if needed. Yes, I'll keep an eye on it in the GTK port
Comment on attachment 211060 [details] Patch. View in context: https://bugs.webkit.org/attachment.cgi?id=211060&action=review > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1741 > + const AtomicString& value = element->fastGetAttribute(valueAttr); > + if (!value.isEmpty()) > + return value; As I mention before, I think this not covering properly the case for HTMLInputElement that's being removed, since you might find it not working properly in cases such as text fields with a limit on the number of characters, as well as range and number input types. So, I believe it would be better to keep that snippet as a specific case before going for the generic case here. What do you think?
Comment on attachment 211060 [details] Patch. Some additional work to do here, removing review request. Will have something shortly. Thanks.
Created attachment 211257 [details] Patch. Switching value fetching back to the value method on HTMLInputElement per Mario's suggestion to leverage the existing sanitation and fallback code that exists there (thanks Mario).
(In reply to comment #21) > Created an attachment (id=211257) [details] > Patch. > > Switching value fetching back to the value method on HTMLInputElement per Mario's suggestion to leverage the existing sanitation and fallback code that exists there (thanks Mario). Your welcome. Btw, you haven't set r? in this new patch, not sure if you want me to review it or not. Just set the flag if that's the case :)
(In reply to comment #22) > (In reply to comment #21) > > Created an attachment (id=211257) [details] [details] > > Patch. > > > > Switching value fetching back to the value method on HTMLInputElement per Mario's suggestion to leverage the existing sanitation and fallback code that exists there (thanks Mario). > > Your welcome. Btw, you haven't set r? in this new patch, not sure if you want me to review it or not. > > Just set the flag if that's the case :) I've recently been making a point to wait for EWS to complete before setting r? to make sure I don't waste anyones cycles. A review would be great! Thanks.
Comment on attachment 211257 [details] Patch. It looks good to me now. Thanks! (In reply to comment #23) > [...] > I've recently been making a point to wait for EWS to complete before setting > r? to make sure I don't waste anyones cycles. A review would be great! Thanks. Ah, ok. Just asking since it was not clear to me, but I agree that thing of waiting for the EWS sounds like a good idea indeed.
Comment on attachment 211257 [details] Patch. Clearing flags on attachment: 211257 Committed r155601: <http://trac.webkit.org/changeset/155601>
All reviewed patches have been landed. Closing bug.