| Summary: | AX: Focusable elements without a role should not be ignored | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Dominic Mazzoni <dmazzoni> | ||||||||||||||||
| Component: | Accessibility | Assignee: | Dominic Mazzoni <dmazzoni> | ||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||
| Severity: | Normal | CC: | aboxhall, cfleizach, dglazkov, dominicc, webkit.review.bot | ||||||||||||||||
| Priority: | P2 | ||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||
| Bug Depends on: | 94895 | ||||||||||||||||||
| Bug Blocks: | |||||||||||||||||||
| Attachments: |
|
||||||||||||||||||
Created attachment 159023 [details]
Patch
Comment on attachment 159023 [details] Patch Attachment 159023 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13512804 New failing tests: accessibility/focusable-div.html Created attachment 159038 [details]
Archive of layout-test-results from gce-cr-linux-03
The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-03 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Created attachment 159171 [details]
Patch
Created attachment 159760 [details]
Patch
Sorry about the junk in the last patch's ChangeLog. This is ready for review now when you have a chance. Comment on attachment 159760 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=159760&action=review > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1370 > + const AtomicString& contentEditableValue = toElement(node)->getAttribute(contenteditableAttr); It looks like both callers could rely on the internal node() value to determine this, so I think this could be an instance method instead of a static If you do that, you can then use getAttribute() method on AccessibilityObject. Might be handy to add a hasAttribute() as well so we can avoid all these checks. That has the benefit of doing all the node() and Element checks (and it also uses fastGetAttribute()) > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1416 > + if (canSetFocusAttribute() && !isControl() && !nodeHasContentEditableAttributeSet(node) && !node->hasTagName(bodyTag)) maybe we should code this logic into some coherent method, like isFocusableAndNonEditable() or something. It's hard for me to piece together why this combination deserves to use textUnderElement() but other things don't. A clearly named method would help (we could also role that into the if (isHeading() || isLink()) if Created attachment 159812 [details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=159760&action=review >> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1370 >> + const AtomicString& contentEditableValue = toElement(node)->getAttribute(contenteditableAttr); > > It looks like both callers could rely on the internal node() value to determine this, so I think this could be an instance method instead of a static > > If you do that, you can then use getAttribute() method on AccessibilityObject. Might be handy to add a hasAttribute() as well so we can avoid all these checks. That has the benefit of doing all the node() and Element checks (and it also uses fastGetAttribute()) Good idea, done. >> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1416 >> + if (canSetFocusAttribute() && !isControl() && !nodeHasContentEditableAttributeSet(node) && !node->hasTagName(bodyTag)) > > maybe we should code this logic into some coherent method, like > > isFocusableAndNonEditable() or something. > > It's hard for me to piece together why this combination deserves to use textUnderElement() but other things don't. A clearly named method would help (we could also role that into the if (isHeading() || isLink()) if Sure. I split it into a utility method and added some context to explain the logic. I don't think isHeading or isLink applies. Basically this class covers things that are focusable, but aren't one of the roles that we have code to handle, like links, controls, content editable. Comment on attachment 159812 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=159812&action=review looks good > LayoutTests/ChangeLog:1851 > +>>>>>>> .r125912 errant merge conflict Created attachment 160082 [details]
Patch for landing
Comment on attachment 160082 [details] Patch for landing Clearing flags on attachment: 160082 Committed r126391: <http://trac.webkit.org/changeset/126391> All reviewed patches have been landed. Closing bug. Reverted r126391 for reason: Breaks Chromium browser_tests AccessibilityFooter, AccessibilityListMarkers, AccessibilityUI Committed r126414: <http://trac.webkit.org/changeset/126414> More context about those breaks: They appeared on the Chromium Mac canaries here: <http://build.chromium.org/p/chromium.webkit/builders/Mac10.6%20Tests/builds/13004> Created attachment 160255 [details]
Patch
There's only one change relative to the last patch (that was landed and reverted): in AccessibilityRenderObject::isGenericFocusableElement, I added:
if (roleValue() == WebAreaRole)
return false;
The problem is that because a web area is focusable, querying title() on the root web area element was returning the full document text, which is definitely not desired - and this was caught by some Chrome tests.
Comment on attachment 160255 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=160255&action=review > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:3142 > + if (node() && node()->hasTagName(bodyTag)) what element is the bodyTag if it's not the WebAreaRole? I guess I have assumed that the bodyTag's role is WebArea for some time (In reply to comment #18) > (From update of attachment 160255 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=160255&action=review > > > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:3142 > > + if (node() && node()->hasTagName(bodyTag)) > > what element is the bodyTag if it's not the WebAreaRole? I guess I have assumed that the bodyTag's role is WebArea for some time The document element is the WebArea. The body is sometimes just a group, sometimes it's ignored for accessibility. It's confusing because in JavaScript, calling document.activeElement.blur() puts focus on the body (from JavaScript's perspective), and document.body.focus() (assuming the body is explicitly made focusable) has the same effect as clearing the focus. Comment on attachment 160255 [details] Patch Clearing flags on attachment: 160255 Committed r126496: <http://trac.webkit.org/changeset/126496> All reviewed patches have been landed. Closing bug. Re-opened since this is blocked by 94895 Hmmm, the Chrome test failures don't seem like an actual bug in this patch this time, they're actually just tests that are over-sensitive. I guess I'll need to fix those tests first... Committed r126970: <http://trac.webkit.org/changeset/126970> |
It may be poor practice, but sometimes a web author will create a focusable generic object on a page that doesn't have an ARIA role, e.g.: <div id="myregion" tabindex=0> Region </div> The accessibility object corresponding to that focusable element should not be ignored for accessibility, because then when the element gets focus, no accessibility notification can be generated. Currently this is not the case - if you create an example like the above, WebKit-based browsers announce nothing when you tab to focus that element. I think they should announce the inner text, e.g., "Region". I have a patch.