Bug 260621 – AX: aria-controls-with-tabs fails in ITM

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

[Joshua Hoffman]

The aria-controls-with-tabs.html test fails when run in isolated tree mode.

[Radar WebKit Bug Importer]

<rdar://problem/114334241>

[Joshua Hoffman]

Created attachment 467398 [details]
Patch

[chris fleizach]

Comment on attachment 467398 [details]
Patch

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

> Source/WebCore/accessibility/AXObjectCache.cpp:1537
> +void AXObjectCache::handleTabOrControlledTabPanelSelected(Node* node)

feel like our naming has been moving to

onTabSelectionChange()

(that may be enough rather than tabOrControlledTab which is pretty wordy)

> Source/WebCore/accessibility/AXObjectCache.cpp:1647
> +    handleTabOrControlledTabPanelSelected(newNode);

can we make this method take old and new and do the work once?

> Source/WebCore/accessibility/AXObjectCache.cpp:4081
> +            if (notification.first->isTabList())

in this tabList case, we're not actually updating isSelected right? we just want SelectedChildren to change?

[Joshua Hoffman]

(In reply to chris fleizach from comment #3)
> Comment on attachment 467398 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=467398&action=review
> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:1647
> > +    handleTabOrControlledTabPanelSelected(newNode);
> 
> can we make this method take old and new and do the work once?

Yes, however we will have to do the traversal twice inside the method regardless in case the tab panels are controlled by different tabs. 

> > Source/WebCore/accessibility/AXObjectCache.cpp:4081
> > +            if (notification.first->isTabList())
> 
> in this tabList case, we're not actually updating isSelected right? we just
> want SelectedChildren to change?

Yes—I can handle AXChildrenChanged to update that property then (right now, we just do a full updateNode whenever that notification is received.

[Tyler Wilcock]

Comment on attachment 467398 [details]
Patch

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

> Source/WebCore/accessibility/AXObjectCache.cpp:1542
> +    RefPtr<AccessibilityObject> object = getOrCreate(node);

I think this can just be RefPtr object = ... (no <> necessary)

> Source/WebCore/accessibility/AXObjectCache.cpp:1544
> +    if (object->isTabItem()) {

Do we need to null-check `object` before dereferencing here?

> Source/WebCore/accessibility/AXObjectCache.cpp:1558
> +    auto controllers = focusedControlledPanelItem->controllers();
> +
> +    for (auto& axObject : controllers)

Optional nit: this newline feels unnecessary to me.

> Source/WebCore/accessibility/AXObjectCache.cpp:1645
> +    // FIXME: optimize with ancestor flags to determine if an element is in a tabpanel

I feel like this could be written more clearly for future us...isn't the idea more like "FIXME: Consider creating a new ancestor flag to only do this work when |oldNode| or |newNode| have a tab panel ancestor (as that's the only time it is necessary)"? Or is that not accurate?

> Source/WebCore/accessibility/AXObjectCache.cpp:1685
>      else if (is<HTMLOptionElement>(node))
>          postNotification(node, AXSelectedStateChanged);
> +    else if (nodeHasRole(node, "tab"_s))
> +        postNotification(node, AXSelectedStateChanged);

Can we combine this with the previous else if rather than adding a new case?

else if (is<HTMLOptionElement>(node) || nodeHasRole(node, "tab"_s))

> Source/WebCore/accessibility/AXObjectCache.h:570
> +    void handleTabOrControlledTabPanelSelected(Node*);

Any time we add a method here, we need to make sure we add a no-op method to the !ENABLE(ACCESSIBILITY) section of this file to avoid breaking the Playstation build. See `handleRoleChanged` as an example.

[Tyler Wilcock]

(In reply to chris fleizach from comment #3)
> Comment on attachment 467398 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=467398&action=review
> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:1537
> > +void AXObjectCache::handleTabOrControlledTabPanelSelected(Node* node)
> 
> feel like our naming has been moving to
> 
> onTabSelectionChange()
> 
> (that may be enough rather than tabOrControlledTab which is pretty wordy)
The "onFooBar" naming scheme is only for events triggered from the DOM or render tree, so it doesn't fit this case. e.g. onTitleChange, onPopoverTargetToggle

[Joshua Hoffman]

(In reply to Tyler Wilcock from comment #5)
> Comment on attachment 467398 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=467398&action=review
> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:1645
> > +    // FIXME: optimize with ancestor flags to determine if an element is in a tabpanel
> 
> I feel like this could be written more clearly for future us...isn't the
> idea more like "FIXME: Consider creating a new ancestor flag to only do this
> work when |oldNode| or |newNode| have a tab panel ancestor (as that's the
> only time it is necessary)"? Or is that not accurate?
> 

Yes, that wording is more clear—I'll update that. 

> > Source/WebCore/accessibility/AXObjectCache.cpp:1685
> >      else if (is<HTMLOptionElement>(node))
> >          postNotification(node, AXSelectedStateChanged);
> > +    else if (nodeHasRole(node, "tab"_s))
> > +        postNotification(node, AXSelectedStateChanged);
> 
> Can we combine this with the previous else if rather than adding a new case?

Yep.
 
> else if (is<HTMLOptionElement>(node) || nodeHasRole(node, "tab"_s))
> 
> > Source/WebCore/accessibility/AXObjectCache.h:570
> > +    void handleTabOrControlledTabPanelSelected(Node*);
> 
> Any time we add a method here, we need to make sure we add a no-op method to
> the !ENABLE(ACCESSIBILITY) section of this file to avoid breaking the
> Playstation build. See `handleRoleChanged` as an example.

Got it!

[Joshua Hoffman]

(In reply to Tyler Wilcock from comment #6)
> (In reply to chris fleizach from comment #3)
> > Comment on attachment 467398 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=467398&action=review

> The "onFooBar" naming scheme is only for events triggered from the DOM or render tree, so it doesn't fit this case. e.g. onTitleChange, onPopoverTargetToggle

I could remove the word "controlled" from the method name to make it less wordy: handleTabOrTabPanelSelected ?

[Joshua Hoffman]

Created attachment 467419 [details]
Patch

[Tyler Wilcock]

Comment on attachment 467419 [details]
Patch

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

> Source/WebCore/accessibility/AXObjectCache.cpp:-4112
> -        case AXSelectedChildrenChanged:

Sorry to jump on this so late after Chris' comment, but the reason that AXSelectedChildrenChanged didn't just do an updateNodeProperty(*notification.first, AXPropertyName::SelectedChildren) is because selected children determines the `accessibleNameForNode` for listboxes:

// The Accname specification states that if the name is being calculated for a combobox
// or listbox inside a labeling element, return the text alternative of the chosen option.
AccessibilityObject::AccessibilityChildrenVector selectedChildren;
if (axObject->isListBox())
    selectedChildren = axObject->selectedChildren();

And in turn, the accessibleNameForNode is a transitive component of several other properties. And whenever a property's dependency graph gets complex (as in this case), we fallback to updateNode to make sure everything is up-to-date.

I'm guessing we don't have a testcase for this specific condition (we should add one if not).

[Tyler Wilcock]

Comment on attachment 467419 [details]
Patch

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

> Source/WebCore/accessibility/AXObjectCache.cpp:1559
> +                for (auto& axObject : oldControllers) {

Optional nit, but I think `for (auto& oldController : oldControllers) {` would read better here.

> Source/WebCore/accessibility/AXObjectCache.cpp:1561
> +                    if (auto* parent = axObject->parentObject(); parent->isTabList())

You need to null-check this here when using multi-conditional if-statements. So it should be:

parent && parent->isTabList()

> Source/WebCore/accessibility/AXObjectCache.cpp:1571
> +    if (!newNode)
> +        return;
> +
> +    RefPtr newObject = get(newNode);

I think we can rely on AXObjectCache::get returning nullptr if given a nullptr newNode, so can probably remove the newNode null-check.

> Source/WebCore/accessibility/AXObjectCache.cpp:1588
> +    for (auto& axObject : newControllers) {

Optional nit, but I think `for (auto& newController : newControllers) {` would read better here.

> Source/WebCore/accessibility/AXObjectCache.cpp:1590
> +        if (auto* parent = axObject->parentObject(); parent->isTabList())

Also need a null-check here.

[Andres Gonzalez]

(In reply to Tyler Wilcock from comment #5)
> Comment on attachment 467398 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=467398&action=review
> 
> 
> > Source/WebCore/accessibility/AXObjectCache.h:570
> > +    void handleTabOrControlledTabPanelSelected(Node*);
> 
> Any time we add a method here, we need to make sure we add a no-op method to
> the !ENABLE(ACCESSIBILITY) section of this file to avoid breaking the
> Playstation build. See `handleRoleChanged` as an example.

Only for those methods that are actually used when !ENABLE(ACCESSIBILITY), i.e., calls from outside accessibility. No need to have no-op stubs for private or methods that are only used inside accessibility.

[Andres Gonzalez]

(In reply to Joshua Hoffman from comment #7)
> (In reply to Tyler Wilcock from comment #5)
> > Comment on attachment 467398 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=467398&action=review
> > 
> > > Source/WebCore/accessibility/AXObjectCache.cpp:1645
> > > +    // FIXME: optimize with ancestor flags to determine if an element is in a tabpanel
> > 
> > I feel like this could be written more clearly for future us...isn't the
> > idea more like "FIXME: Consider creating a new ancestor flag to only do this
> > work when |oldNode| or |newNode| have a tab panel ancestor (as that's the
> > only time it is necessary)"? Or is that not accurate?
> > 
> 
> Yes, that wording is more clear—I'll update that. 
> 
> > > Source/WebCore/accessibility/AXObjectCache.cpp:1685
> > >      else if (is<HTMLOptionElement>(node))
> > >          postNotification(node, AXSelectedStateChanged);
> > > +    else if (nodeHasRole(node, "tab"_s))
> > > +        postNotification(node, AXSelectedStateChanged);
> > 
> > Can we combine this with the previous else if rather than adding a new case?
> 
> Yep.
>  
> > else if (is<HTMLOptionElement>(node) || nodeHasRole(node, "tab"_s))
> > 
> > > Source/WebCore/accessibility/AXObjectCache.h:570
> > > +    void handleTabOrControlledTabPanelSelected(Node*);
> > 
> > Any time we add a method here, we need to make sure we add a no-op method to
> > the !ENABLE(ACCESSIBILITY) section of this file to avoid breaking the
> > Playstation build. See `handleRoleChanged` as an example.
> 
> Got it!

See my comment above. Only do that when needed.

[Andres Gonzalez]

(In reply to Joshua Hoffman from comment #9)
> Created attachment 467419 [details]
> Patch

--- a/Source/WebCore/accessibility/AXObjectCache.cpp
+++ b/Source/WebCore/accessibility/AXObjectCache.cpp

+        else if (oldObject) {
+            RefPtr oldFocusedControlledPanelItem = Accessibility::findAncestor<AccessibilityObject>(*oldObject, false, [] (auto& axObject) {

oldFocusedControlledPanelItem --> oldFocusedControlledPanel
axObject --> ancestor

+                return axObject.roleValue() == AccessibilityRole::TabPanel && axObject.controllers().size();
+            });
+
+            if (oldFocusedControlledPanelItem) {
+                auto oldControllers = oldFocusedControlledPanelItem->controllers();

I think that in findAncestor, you only need to find the closest panel ancestor, not a panel with controllers. Then if you do find a panel, you can check the controllers and update them.

The rest of the function does the same thing for the new focus. Avoid the code duplication, maybe using a lambda. Also account for the case where the old panel and the new panel are the same, in which case you only need to check for controllers and do updates and notifications only once.

--- a/Source/WebCore/accessibility/AXObjectCache.h
+++ b/Source/WebCore/accessibility/AXObjectCache.h

+    void handleTabOrTabPanelSelected(Node*, Node*);

What about updateTabs / updateTabPanels for name?

+inline void AXObjectCache::handleTabOrTabPanelSelected(Node*, Node*) { }

No needed since this is a private method.

[Joshua Hoffman]

Comment on attachment 467419 [details]
Patch

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

>> Source/WebCore/accessibility/AXObjectCache.cpp:-4112
>> -        case AXSelectedChildrenChanged:
> 
> Sorry to jump on this so late after Chris' comment, but the reason that AXSelectedChildrenChanged didn't just do an updateNodeProperty(*notification.first, AXPropertyName::SelectedChildren) is because selected children determines the `accessibleNameForNode` for listboxes:
> 
> // The Accname specification states that if the name is being calculated for a combobox
> // or listbox inside a labeling element, return the text alternative of the chosen option.
> AccessibilityObject::AccessibilityChildrenVector selectedChildren;
> if (axObject->isListBox())
>     selectedChildren = axObject->selectedChildren();
> 
> And in turn, the accessibleNameForNode is a transitive component of several other properties. And whenever a property's dependency graph gets complex (as in this case), we fallback to updateNode to make sure everything is up-to-date.
> 
> I'm guessing we don't have a testcase for this specific condition (we should add one if not).

Ah I see—will revert this to use updateNode. Let's definitely add that case in a follow-up. Thanks!

[Joshua Hoffman]

(In reply to Andres Gonzalez from comment #14)
> (In reply to Joshua Hoffman from comment #9)
> > Created attachment 467419 [details]
> > Patch
> 
> +                return axObject.roleValue() == AccessibilityRole::TabPanel
> && axObject.controllers().size();
> +            });
> +
> +            if (oldFocusedControlledPanelItem) {
> +                auto oldControllers =
> oldFocusedControlledPanelItem->controllers();
> 
> I think that in findAncestor, you only need to find the closest panel
> ancestor, not a panel with controllers. Then if you do find a panel, you can
> check the controllers and update them.
> 
> The rest of the function does the same thing for the new focus. Avoid the
> code duplication, maybe using a lambda. Also account for the case where the
> old panel and the new panel are the same, in which case you only need to
> check for controllers and do updates and notifications only once.
> 

I will refactor that method, thanks for the suggestions!

[Joshua Hoffman]

Created attachment 467434 [details]
Patch

[Joshua Hoffman]

Created attachment 467463 [details]
Patch

[Joshua Hoffman]

Created attachment 467467 [details]
Patch

[EWS]

Committed 267391@main (b1049697ce8b): <https://commits.webkit.org/267391@main>

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

[Darin Adler]

Comment on attachment 467467 [details]
Patch

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

Some post-landing style comments

> Source/WebCore/accessibility/AXObjectCache.cpp:1559
> +    RefPtr oldObject = get(oldNode);

Slight better style:

    if (RefPtr oldObject = get(oldNode))

> Source/WebCore/accessibility/AXObjectCache.cpp:1564
> +        oldFocusedControlledPanel = Accessibility::findAncestor<AccessibilityObject>(*oldObject, false, [] (auto& ancestor) {
> +            return ancestor.roleValue() == AccessibilityRole::TabPanel;
> +        });

Would be nice to share this "find panel" code too, just as we share the "update tab" code.

[Joshua Hoffman]

Comment on attachment 467467 [details]
Patch

Thanks, Darin! I will make these updates in a follow-up