The aria-controls-with-tabs.html test fails when run in isolated tree mode.
<rdar://problem/114334241>
Created attachment 467398 [details] Patch
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?
(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.
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.
(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
(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!
(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 ?
Created attachment 467419 [details] Patch
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).
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.
(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.
(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.
(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.
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!
(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!
Created attachment 467434 [details] Patch
Created attachment 467463 [details] Patch
Created attachment 467467 [details] Patch
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].
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.
Comment on attachment 467467 [details] Patch Thanks, Darin! I will make these updates in a follow-up