Bug 224582

<rdar://problem/76670072>

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.

Created attachment 464203 [details] Patch

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.

(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

Created attachment 464211 [details] Patch

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?

Created attachment 464221 [details] Patch

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

Created attachment 464329 [details] Patch

Created attachment 464330 [details] Patch

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