Bug 136714 – AX: in an aria-labelledby computation, do not traverse into elements whose nameFrom value does not include 'contents'

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[James Craig]

Regression r173459: in an aria-labelledby computation, do not traverse into elements whose nameFrom value does not include "contents"

[James Craig]

Assigning to myself to patch the test cases.

[James Craig]

Created attachment 237926 [details]
layout test update

New test case demonstrated a few regressions and other failures of previous patch.

[James Craig]

<rdar://problem/18300212>

[chris fleizach]

Created attachment 238554 [details]
patch

[Mario Sanchez Prada]

Comment on attachment 238554 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=238554&action=review

> Source/WebCore/ChangeLog:19
> [phone]  Chris Fleizach  <[e-mail]>
> +
> +        AX: In an aria-labelledby computation, do not traverse into elements whose nameFrom value does not include 'contents'
> +        https://bugs.webkit.org/show_bug.cgi?id=136714
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        There are certain ARIA elements that tell us we should not query their children when determining the name of the object.
> +        Those ones have the "nameFrom" property set to "author" instead of "contents." WebKit needs to honor that status.
> +
> +        Modified: accessibility/aria-labelledby-with-descendants.html
> +
> +        * accessibility/AccessibilityNodeObject.cpp:
> +        (WebCore::shouldUseAccessiblityObjectInnerText):
> +        (WebCore::accessibleNameForNode):
> +        * accessibility/AccessibilityObject.cpp:
> +        (WebCore::AccessibilityObject::accessibleNameDerivesFromContent):
> +        * accessibility/AccessibilityObject.h:
> +

This chunk seems to be duplicated in the patch (yet probably harmless)

[chris fleizach]

Comment on attachment 238554 [details]
patch

Removing review. After scrutizning the layout test results, I realized we were missing a few cases. The changes were big enough that I need a new review

[chris fleizach]

Created attachment 238766 [details]
new patch

[chris fleizach]

Created attachment 238767 [details]
patch

[Darin Adler]

Comment on attachment 238767 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=238767&action=review

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1645
> +static void appendStringBuildTextToName(String& text, StringBuilder& builder)

This should take const String& text. Also the StringBuilder should be the first argument. Also not sure that "append string build text to name" is the best name for this helper function. I think it should probably just be called "append name" or perhaps "append name to string builder".

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1670
> +            String axName = accessibleNameForNode(child->node());
> +            appendStringBuildTextToName(axName, builder);

This would read better without the local variable.

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1864
> +        String valueDesc = axObject->valueDescription();
> +        if (!valueDesc.isEmpty())
> +            return valueDesc;

Please don’t abbreviate “description” as “desc” in WebKit code.

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1875
> +    }
>      else

WebKit style puts the } on the same line as the else.

> Source/WebCore/accessibility/AccessibilityObject.cpp:292
> +bool AccessibilityObject::accessibleNameDerivesFromContent() const

This has some long case lists in it. Do we have test coverage for all these cases?

[chris fleizach]

(In reply to comment #9)
> (From update of attachment 238767 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=238767&action=review
> 

Thanks for your feedback. 

> > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1645
> > +static void appendStringBuildTextToName(String& text, StringBuilder& builder)
> 
> This should take const String& text. Also the StringBuilder should be the first argument. Also not sure that "append string build text to name" is the best name for this helper function. I think it should probably just be called "append name" or perhaps "append name to string builder".
> 

Done

> > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1670
> > +            String axName = accessibleNameForNode(child->node());
> > +            appendStringBuildTextToName(axName, builder);
> 
> This would read better without the local variable.
> 

Will do

> > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1864
> > +        String valueDesc = axObject->valueDescription();
> > +        if (!valueDesc.isEmpty())
> > +            return valueDesc;
> 
> Please don’t abbreviate “description” as “desc” in WebKit code.

Will do
> 
> > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1875
> > +    }
> >      else
> 
> WebKit style puts the } on the same line as the else.
> 

fixed

> > Source/WebCore/accessibility/AccessibilityObject.cpp:292
> > +bool AccessibilityObject::accessibleNameDerivesFromContent() const
> 
> This has some long case lists in it. Do we have test coverage for all these cases?

I'll fill out the rest of the test coverage.

Thanks

[chris fleizach]

http://trac.webkit.org/changeset/174074

[Alexey Proskuryakov]

Looks like this change broke two accessibility tests:

https://build.webkit.org/results/Apple%20Mavericks%20Release%20WK1%20(Tests)/r174074%20(8854)/results.html

[chris fleizach]

(In reply to comment #12)
> Looks like this change broke two accessibility tests:
> 
> https://build.webkit.org/results/Apple%20Mavericks%20Release%20WK1%20(Tests)/r174074%20(8854)/results.html

Ok, I'm on it

[Alexey Proskuryakov]

I think that Roger addressed this with <https://trac.webkit.org/changeset/174080>.

[chris fleizach]

(In reply to comment #14)
> I think that Roger addressed this with <https://trac.webkit.org/changeset/174080>.

I think so. His changes were inline with the same changes I was going to make