Bug 94302 – AX: Focusable elements without a role should not be ignored

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

[Dominic Mazzoni]

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.

[Dominic Mazzoni]

Created attachment 159023 [details]
Patch

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Dominic Mazzoni]

Created attachment 159171 [details]
Patch

[Dominic Mazzoni]

Created attachment 159760 [details]
Patch

[Dominic Mazzoni]

Sorry about the junk in the last patch's ChangeLog. This is ready for review now when you have a chance.

[chris fleizach]

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

[Dominic Mazzoni]

Created attachment 159812 [details]
Patch

[Dominic Mazzoni]

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.

[chris fleizach]

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

[Dominic Mazzoni]

Created attachment 160082 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 160082 [details]
Patch for landing

Clearing flags on attachment: 160082

Committed r126391: <http://trac.webkit.org/changeset/126391>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Dominic Cooney]

Reverted r126391 for reason:

Breaks Chromium browser_tests AccessibilityFooter, AccessibilityListMarkers, AccessibilityUI

Committed r126414: <http://trac.webkit.org/changeset/126414>

[Dominic Cooney]

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>

[Dominic Mazzoni]

Created attachment 160255 [details]
Patch

[Dominic Mazzoni]

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.

[chris fleizach]

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

[Dominic Mazzoni]

(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.

[WebKit Review Bot]

Comment on attachment 160255 [details]
Patch

Clearing flags on attachment: 160255

Committed r126496: <http://trac.webkit.org/changeset/126496>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by 94895

[Dominic Mazzoni]

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...

[Dominic Mazzoni]

Committed r126970: <http://trac.webkit.org/changeset/126970>