While working on another bug, I discovered that we're not implementing certain relation types associated with ARIA attributes. This includes aria-owns and most of the reciprocal relations (e.g. description-for on elements referenced by aria-describedby). While implementing support for the relation types set on the element is easy, it appears that implementing the reciprocal relations will require a more significant fix: We're currently doing all the work in WebKitAccessibleWrapper.cpp's setAtkRelationSetFromCoreObject(). At that point, it doesn't make sense to go on a hunt for elements which might have an ARIA attribute that references the current element. Thus we probably should cache these attributes and elements when creating the accessible tree. In addition, we're not doing any real testing of the relations other than labelled-by (via AXTitleUIElement). The test runner will need some additions there....
<rdar://problem/25168402>
Created attachment 311627 [details] Patch
Attachment 311627 [details] did not pass style-queue: ERROR: Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.h:198: 'ariaLabelledByElementAtIndex' is incorrectly named. It should be named 'protector' or 'protectedUnsigned'. [readability/naming/protected] [4] ERROR: Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.h:199: 'ariaDescribedByElementAtIndex' is incorrectly named. It should be named 'protector' or 'protectedUnsigned'. [readability/naming/protected] [4] ERROR: Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.h:200: 'ariaOwnsReferencingElementAtIndex' is incorrectly named. It should be named 'protector' or 'protectedUnsigned'. [readability/naming/protected] [4] ERROR: Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.h:201: 'ariaFlowToReferencingElementAtIndex' is incorrectly named. It should be named 'protector' or 'protectedUnsigned'. [readability/naming/protected] [4] ERROR: Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.h:202: 'ariaControlsReferencingElementAtIndex' is incorrectly named. It should be named 'protector' or 'protectedUnsigned'. [readability/naming/protected] [4] ERROR: Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.h:203: 'ariaLabelledByReferencingElementAtIndex' is incorrectly named. It should be named 'protector' or 'protectedUnsigned'. [readability/naming/protected] [4] ERROR: Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.h:204: 'ariaDescribedByReferencingElementAtIndex' is incorrectly named. It should be named 'protector' or 'protectedUnsigned'. [readability/naming/protected] [4] Total errors found: 7 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 311627 [details] Patch Chris: What are your thoughts on the "incorrectly named" functions? I named the methods in my patch to be consistent with the existing methods. Beyond that, a general review would be much appreciated. Thanks!
(In reply to Joanmarie Diggs (irc: joanie) from comment #4) > Comment on attachment 311627 [details] > Patch > > Chris: What are your thoughts on the "incorrectly named" functions? I named > the methods in my patch to be consistent with the existing methods. Beyond > that, a general review would be much appreciated. Thanks! those incorrectly named method warnings don't make any sense to me
Comment on attachment 311627 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=311627&action=review > Source/WebCore/accessibility/AccessibilityObject.cpp:3291 > + Node* node = this->node(); I think you can just do Element* element = this->element() and save a check here > Source/WebCore/accessibility/AccessibilityObject.cpp:3299 > + AXObjectCache* cache = axObjectCache(); do we need to check if cache is nil here? > Source/WebCore/accessibility/AccessibilityObject.cpp:3301 > + String idList = element.attributeWithoutSynchronization(attribute).string(); can this be a const AtomicString& > Source/WebCore/accessibility/AccessibilityObject.cpp:3306 > + idList.replace("\n", " ").split(" ", ids); should this be more inclusive of other whitespace? \t \r
Comment on attachment 311627 [details] Patch Attachment 311627 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/3850428 New failing tests: compositing/iframes/display-none-subframe.html fast/css/target-fragment-match.html
Created attachment 311644 [details] Archive of layout-test-results from ews124 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews124 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.5
(In reply to Build Bot from comment #3) > If any of these errors are false positives, please file a bug against > check-webkit-style. I think the style checker is getting tripped up because the variable names contain "Ref". It would be smarter to search for " Ref<" or " RefPtr<" instead.
Created attachment 311706 [details] Patch
Attachment 311706 [details] did not pass style-queue: ERROR: Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.h:198: 'ariaLabelledByElementAtIndex' is incorrectly named. It should be named 'protector' or 'protectedUnsigned'. [readability/naming/protected] [4] ERROR: Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.h:199: 'ariaDescribedByElementAtIndex' is incorrectly named. It should be named 'protector' or 'protectedUnsigned'. [readability/naming/protected] [4] ERROR: Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.h:200: 'ariaOwnsReferencingElementAtIndex' is incorrectly named. It should be named 'protector' or 'protectedUnsigned'. [readability/naming/protected] [4] ERROR: Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.h:201: 'ariaFlowToReferencingElementAtIndex' is incorrectly named. It should be named 'protector' or 'protectedUnsigned'. [readability/naming/protected] [4] ERROR: Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.h:202: 'ariaControlsReferencingElementAtIndex' is incorrectly named. It should be named 'protector' or 'protectedUnsigned'. [readability/naming/protected] [4] ERROR: Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.h:203: 'ariaLabelledByReferencingElementAtIndex' is incorrectly named. It should be named 'protector' or 'protectedUnsigned'. [readability/naming/protected] [4] ERROR: Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.h:204: 'ariaDescribedByReferencingElementAtIndex' is incorrectly named. It should be named 'protector' or 'protectedUnsigned'. [readability/naming/protected] [4] Total errors found: 7 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 311706 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=311706&action=review > Source/WebCore/accessibility/AccessibilityObject.cpp:3291 > + if (id.isNull() || id.isEmpty()) This should only check isEmpty, since null strings are also empty. > Source/WebCore/accessibility/AccessibilityObject.cpp:3300 > + if (idList.isNull() || idList.isEmpty()) Ditto. > Source/WebCore/accessibility/AccessibilityObject.cpp:3306 > + Vector<String> ids; > + String(idList).simplifyWhiteSpace(isHTMLSpace).split(" ", ids); > + if (!ids.contains(id)) > + continue; There is a class to do this efficiently, called SpaceSplitString. That should be used rather than this much less efficient implementation, in part to be consistent with how the actual DOM implements this. One way to use it would be: if (!SpaceSplitString(idList, false).contains(id)) continue; If doing this, then the id local variable should be an AtomicString rather than a String. That leads me to question why identifierAttribute returns a String rather than a const AtomicString&; I can’t see any reason why it should, so that is worth fixing eventually for better efficiency.
Comment on attachment 311706 [details] Patch Attachment 311706 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/3854196 New failing tests: accessibility/aria-namefrom-author.html
Created attachment 311718 [details] Archive of layout-test-results from ews114 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews114 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Created attachment 311720 [details] Patch
(In reply to Joanmarie Diggs (irc: joanie) from comment #15) > Created attachment 311720 [details] > Patch Ignore this one as I didn't see the aria-namefrom-author.html failure.
Attachment 311720 [details] did not pass style-queue: ERROR: Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.h:198: 'ariaLabelledByElementAtIndex' is incorrectly named. It should be named 'protector' or 'protectedUnsigned'. [readability/naming/protected] [4] ERROR: Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.h:199: 'ariaDescribedByElementAtIndex' is incorrectly named. It should be named 'protector' or 'protectedUnsigned'. [readability/naming/protected] [4] ERROR: Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.h:200: 'ariaOwnsReferencingElementAtIndex' is incorrectly named. It should be named 'protector' or 'protectedUnsigned'. [readability/naming/protected] [4] ERROR: Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.h:201: 'ariaFlowToReferencingElementAtIndex' is incorrectly named. It should be named 'protector' or 'protectedUnsigned'. [readability/naming/protected] [4] ERROR: Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.h:202: 'ariaControlsReferencingElementAtIndex' is incorrectly named. It should be named 'protector' or 'protectedUnsigned'. [readability/naming/protected] [4] ERROR: Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.h:203: 'ariaLabelledByReferencingElementAtIndex' is incorrectly named. It should be named 'protector' or 'protectedUnsigned'. [readability/naming/protected] [4] ERROR: Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.h:204: 'ariaDescribedByReferencingElementAtIndex' is incorrectly named. It should be named 'protector' or 'protectedUnsigned'. [readability/naming/protected] [4] Total errors found: 7 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 311720 [details] Patch Attachment 311720 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/3855386 New failing tests: accessibility/aria-namefrom-author.html
Created attachment 311755 [details] Archive of layout-test-results from ews115 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews115 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Created attachment 311815 [details] Patch
Attachment 311815 [details] did not pass style-queue: ERROR: Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.h:198: 'ariaLabelledByElementAtIndex' is incorrectly named. It should be named 'protector' or 'protectedUnsigned'. [readability/naming/protected] [4] ERROR: Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.h:199: 'ariaDescribedByElementAtIndex' is incorrectly named. It should be named 'protector' or 'protectedUnsigned'. [readability/naming/protected] [4] ERROR: Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.h:200: 'ariaOwnsReferencingElementAtIndex' is incorrectly named. It should be named 'protector' or 'protectedUnsigned'. [readability/naming/protected] [4] ERROR: Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.h:201: 'ariaFlowToReferencingElementAtIndex' is incorrectly named. It should be named 'protector' or 'protectedUnsigned'. [readability/naming/protected] [4] ERROR: Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.h:202: 'ariaControlsReferencingElementAtIndex' is incorrectly named. It should be named 'protector' or 'protectedUnsigned'. [readability/naming/protected] [4] ERROR: Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.h:203: 'ariaLabelledByReferencingElementAtIndex' is incorrectly named. It should be named 'protector' or 'protectedUnsigned'. [readability/naming/protected] [4] ERROR: Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.h:204: 'ariaDescribedByReferencingElementAtIndex' is incorrectly named. It should be named 'protector' or 'protectedUnsigned'. [readability/naming/protected] [4] Total errors found: 7 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to Darin Adler from comment #12) > This should only check isEmpty, since null strings are also empty. Done. > Ditto. Done. > There is a class to do this efficiently, called SpaceSplitString. That > should be used rather than this much less efficient implementation, in part > to be consistent with how the actual DOM implements this. One way to use it > would be: > > if (!SpaceSplitString(idList, false).contains(id)) > continue; Cool, thanks, and done. Also made a similar change in AccessibilityObject::elementsFromAttribute(). > If doing this, then the id local variable should be an AtomicString rather > than a String. That leads me to question why identifierAttribute returns a > String rather than a const AtomicString&; I can’t see any reason why it > should, so that is worth fixing eventually for better efficiency. Turns out it's only called on occasion, so also done. Thanks!
Chris or Darin, please review the current patch. Thanks!
Comment on attachment 311815 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=311815&action=review > Source/WebCore/accessibility/AccessibilityObject.cpp:3303 > + if (idList.isEmpty()) > + continue; No need for this optimization; I think SpaceSplitString will handle this fine without this extra check.
Created attachment 312761 [details] Patch patch for landing
Comment on attachment 312761 [details] Patch Clearing flags on attachment: 312761 Committed r218177: <http://trac.webkit.org/changeset/218177>
All reviewed patches have been landed. Closing bug.