Regression r173459: in an aria-labelledby computation, do not traverse into elements whose nameFrom value does not include "contents"
Assigning to myself to patch the test cases.
Created attachment 237926 [details] layout test update New test case demonstrated a few regressions and other failures of previous patch.
<rdar://problem/18300212>
Created attachment 238554 [details] patch
Comment on attachment 238554 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=238554&action=review > Source/WebCore/ChangeLog:19 > +2014-09-23 Chris Fleizach <cfleizach@apple.com> > + > + 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)
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
Created attachment 238766 [details] new patch
Created attachment 238767 [details] patch
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?
(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
http://trac.webkit.org/changeset/174074
Looks like this change broke two accessibility tests: https://build.webkit.org/results/Apple%20Mavericks%20Release%20WK1%20(Tests)/r174074%20(8854)/results.html
(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
I think that Roger addressed this with <https://trac.webkit.org/changeset/174080>.
(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