[Webkit-unassigned] [Bug 120814] AX: Self-referencing aria-labelledby only uses contents.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=120814





--- Comment #9 from Samuel White <samuel_white at apple.com>  2013-09-06 14:30:52 PST ---
(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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.