Bug 260278

Summary: AX: aria-selected-menu-items test is flaky in ITM mode
Product: WebKit Reporter: Joshua Hoffman <jhoffman23>
Component: AccessibilityAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: aboxhall, andresg_22, apinheiro, cfleizach, dmazzoni, ews-watchlist, jcraig, jdiggs, samuel_white, tyler_w, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Joshua Hoffman 2023-08-16 11:30:34 PDT
The accessibility/aria-selected-menu-items test is flaky when run in ITM mode.
Comment 1 Radar WebKit Bug Importer 2023-08-16 11:30:46 PDT
<rdar://problem/113979223>
Comment 2 Joshua Hoffman 2023-08-16 20:37:33 PDT
Created attachment 467304 [details]
Patch
Comment 3 Joshua Hoffman 2023-08-18 17:44:41 PDT
Created attachment 467336 [details]
Patch
Comment 4 Andres Gonzalez 2023-08-21 07:45:57 PDT
(In reply to Joshua Hoffman from comment #3)
> Created attachment 467336 [details]
> Patch

"""
This change fixes aria-selected-menu-items in isolated tree mode by adding a traversal up for the newly selected child so that its ancestor also updates.
"""

so that it's ancestor also updates. -> so that its ancestor also updates.

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

+        case AXMenuListItemSelected:
+            if (RefPtr ancestor = Accessibility::findAncestor<AccessibilityObject>(*notification.first, false, [] (const auto& object) {
+                return object.canHaveSelectedChildren();
+            })) {

I think that declaring ancestor = Accessibility::findAncestor above the if statement is more legible.

--- a/LayoutTests/accessibility/aria-selected-menu-items.html
+++ b/LayoutTests/accessibility/aria-selected-menu-items.html

-        selectedChildInfo(window.accessibilityController.accessibleElementById("menu1"));
-        selectedChildInfo(window.accessibilityController.accessibleElementById("menu2"));
+        testOutput += await selectedChildInfo(window.accessibilityController.accessibleElementById("menu1"), "item2");
+        testOutput += await selectedChildInfo(window.accessibilityController.accessibleElementById("menu2"), "item5");

You can get rid of the window. in window.accessibilityController.
Comment 5 Joshua Hoffman 2023-08-21 17:28:05 PDT
Created attachment 467376 [details]
Patch
Comment 6 chris fleizach 2023-08-21 22:30:12 PDT
Comment on attachment 467376 [details]
Patch

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

> Source/WebCore/accessibility/AXObjectCache.cpp:4019
> +                return object.canHaveSelectedChildren();

should this try to find a matching role instead of canHaveSelectedChildren? there may possibly be a mismatch in what is found

> Source/WebCore/accessibility/AXObjectCache.cpp:4022
> +                updateNode(ancestor);

are we able to update just a single property for the ancestor? (I'm not sure what's happening in updateNode)
Comment 7 Joshua Hoffman 2023-08-22 13:37:01 PDT
(In reply to chris fleizach from comment #6)
> Comment on attachment 467376 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=467376&action=review
> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:4019
> > +                return object.canHaveSelectedChildren();
> 
> should this try to find a matching role instead of canHaveSelectedChildren?
> there may possibly be a mismatch in what is found

Definitely—we can check for menu or menubar roles instead.  
 
> > Source/WebCore/accessibility/AXObjectCache.cpp:4022
> > +                updateNode(ancestor);
> 
> are we able to update just a single property for the ancestor? (I'm not sure
> what's happening in updateNode)

After some more investigation we can do this via an update to AXPropertyName::SelectedChildren.
Comment 8 Joshua Hoffman 2023-08-22 13:41:25 PDT
Created attachment 467389 [details]
Patch
Comment 9 Joshua Hoffman 2023-08-23 13:03:32 PDT
Created attachment 467400 [details]
Patch
Comment 10 EWS 2023-08-23 20:56:17 PDT
Commit message contains (OOPS!) and no reviewer found, blocking PR #None
Comment 11 Joshua Hoffman 2023-08-23 21:30:02 PDT
Created attachment 467409 [details]
Patch
Comment 12 EWS 2023-08-24 01:47:34 PDT
Committed 267221@main (5e54c0b0e654): <https://commits.webkit.org/267221@main>

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