Bug 224582 – AX: `aria-activedescendant` doesn't work with a standard "listbox" pattern

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

[Ben Cronin]

With VoiceOver running, go to the example at https://jsfiddle.net/upy3m6n7/3/ and place the cursor inside the "Editable text" field. Use the arrow keys to move through the options (the selected option will get a "selected" class and become `aria-activedescendant` of the "editor" textbox element).

Expected: VoiceOver will identify each option as it is selected and becomes the `aria-activedescendant` of the "editor" element (this happens in Chrome and Firefox).

Observed: VoiceOver reads out nothing as each option becomes selected; there is no indication that there is an active descendant. 

The spec clearly indicates that elements with "textbox" role and correct markup should roles should accommodate `aria-activedescendant` (see https://www.w3.org/TR/wai-aria/#aria-activedescendant, particularly item 2). FWIW, though, as far as I can tell, the above behavior occurs with any role applied to the "editor" node.

[Radar WebKit Bug Importer]

<rdar://problem/76670072>

[Dhawal]

Same issue here. Here's our implementation https://jsfiddle.net/f5Larbnk/19/

Click on button and hover on the options. Note that voiceover in the above fiddle works fine in Firefox, Chrome when active-descendant is updated.

[chris fleizach]

Created attachment 464203 [details]
Patch

[Tyler Wilcock]

Comment on attachment 464203 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=464203&action=review

> Source/WebCore/accessibility/AXObjectCache.cpp:1898
> +    if (!element.document().frame()->selection().isFocusedAndActive())

Do you think it would be useful to have a comment stating why it's important to use the element document and not the cache document? Perhaps that will help prevent us from making the same mistake again.

> Source/WebCore/accessibility/AXObjectCache.cpp:1932
> +        target = Accessibility::findAncestor(*activeDescendant, true, [&object] (const auto& ancestor) {
> +            return object->relatedObjects(AXRelationType::ControllerFor).contains(&ancestor);
> +        });

Could you explain what the intention of this loop is? It's confusing to me because:

  1. In the first iteration of the loop, `const auto& ancestor` is actually not an ancestor, it's a descendant (*activeDescendant)
  2. Because we start iterating from a descendant, eventually we will iterate to `object`, and then above `object`, checking to see if it aria-controls itself, and then any of its ancestors. Does this make sense?

I think this code works for the simple listbox example in this bug because we return true in the first iteration. But I don't understand what this loop accomplishes generally, which might just be my lack of understanding. Can you explain what the intention is here?

> Source/WebCore/accessibility/AXObjectCache.cpp:4104
> +        if (is<HTMLFrameOwnerElement>(element))
> +            updateRelationsForTree(*downcast<HTMLFrameOwnerElement>(element).contentDocument());

Nit: this could arguably be simpler with dynamicDowncast.

if (auto* frameOwnerElement = dynamicDowncast<HTMLFrameOwnerElement>(element))
    updateRelationsForTree(frameOwnerElement->contentDocument());

> LayoutTests/accessibility/mac/relationships-in-frames.html:35
> +	    debug(output);

The indentation level of this debug doesn't match its siblings.

[chris fleizach]

(In reply to Tyler Wilcock from comment #4)
> Comment on attachment 464203 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=464203&action=review
> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:1898
> > +    if (!element.document().frame()->selection().isFocusedAndActive())
> 
> Do you think it would be useful to have a comment stating why it's important
> to use the element document and not the cache document? Perhaps that will
> help prevent us from making the same mistake again.
> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:1932
> > +        target = Accessibility::findAncestor(*activeDescendant, true, [&object] (const auto& ancestor) {
> > +            return object->relatedObjects(AXRelationType::ControllerFor).contains(&ancestor);
> > +        });
> 
> Could you explain what the intention of this loop is? It's confusing to me
> because:
> 
>   1. In the first iteration of the loop, `const auto& ancestor` is actually
> not an ancestor, it's a descendant (*activeDescendant)
>   2. Because we start iterating from a descendant, eventually we will
> iterate to `object`, and then above `object`, checking to see if it
> aria-controls itself, and then any of its ancestors. Does this make sense?
> 
> I think this code works for the simple listbox example in this bug because
> we return true in the first iteration. But I don't understand what this loop
> accomplishes generally, which might just be my lack of understanding. Can
> you explain what the intention is here?
> 
Added better comments in the code.
Basically, the target needs to be thing we send out the notification for. The active descendant needs to be a descendant of that, but the one generating the notification is the controller.

So we go up the active-descendant hierarchy to see if we find the object that is being controlled by the "controller". If we find that object than we use it as the target.

> > Source/WebCore/accessibility/AXObjectCache.cpp:4104
> > +        if (is<HTMLFrameOwnerElement>(element))
> > +            updateRelationsForTree(*downcast<HTMLFrameOwnerElement>(element).contentDocument());
> 
> Nit: this could arguably be simpler with dynamicDowncast.
> 
> if (auto* frameOwnerElement =
> dynamicDowncast<HTMLFrameOwnerElement>(element))
>     updateRelationsForTree(frameOwnerElement->contentDocument());
> 
> > LayoutTests/accessibility/mac/relationships-in-frames.html:35
> > +	    debug(output);
> 
> The indentation level of this debug doesn't match its siblings.

Will make these changes

[chris fleizach]

Created attachment 464211 [details]
Patch

[Tyler Wilcock]

Comment on attachment 464211 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=464211&action=review

> Source/WebCore/accessibility/AXObjectCache.cpp:1935
> +        target = Accessibility::findAncestor(*activeDescendant, false, [&object] (const auto& activeDescendantAncestor) {
> +            return object->relatedObjects(AXRelationType::ControllerFor).contains(&activeDescendantAncestor);
> +        });

Thanks for clarifying. One last question: rather than re-computing object->relatedObjects(AXRelationType::ControllerFor) every iteration, can we extract it into a local variable?

[chris fleizach]

Created attachment 464221 [details]
Patch

[Andres Gonzalez]

(In reply to chris fleizach from comment #8)
> Created attachment 464221 [details]
> Patch

--- /dev/null
+++ b/LayoutTests/accessibility/mac/active-descendant-with-aria-controls.html

+var output = "This test ensures objects that control objects with active descendants generate the correct notifications, while also being inside frames.\n\n";

No iframe involved in this test.

Not sure that this isn't flaky because the order of the calls to debug() in the notificationCallback and main is not deterministic. There may be differences in timing between ITM on and off. I would run it with --iterations=100 to ensure.

[chris fleizach]

Created attachment 464329 [details]
Patch

[chris fleizach]

Created attachment 464330 [details]
Patch

[EWS]

Committed 258478@main (f78fd4847fdb): <https://commits.webkit.org/258478@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 464330 [details].