Summary: | AX: in an aria-labelledby computation, do not traverse into elements whose nameFrom value does not include 'contents' | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | James Craig <jcraig> | ||||||||||
Component: | Accessibility | Assignee: | chris fleizach <cfleizach> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | aboxhall, apinheiro, cfleizach, commit-queue, dmazzoni, jdiggs, mario, samuel_white, webkit-bug-importer | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | All | ||||||||||||
OS: | All | ||||||||||||
Bug Depends on: | 136689 | ||||||||||||
Bug Blocks: | |||||||||||||
Attachments: |
|
Description
James Craig
2014-09-10 13:56:39 PDT
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.
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 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 |