Bug 260278 - AX: aria-selected-menu-items test is flaky in ITM mode
Summary: AX: aria-selected-menu-items test is flaky in ITM mode
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2023-08-16 11:30 PDT by Joshua Hoffman
Modified: 2023-08-24 01:47 PDT (History)
11 users (show)

See Also:


Attachments
Patch (6.73 KB, patch)
2023-08-16 20:37 PDT, Joshua Hoffman
no flags Details | Formatted Diff | Diff
Patch (8.03 KB, patch)
2023-08-18 17:44 PDT, Joshua Hoffman
no flags Details | Formatted Diff | Diff
Patch (8.04 KB, patch)
2023-08-21 17:28 PDT, Joshua Hoffman
no flags Details | Formatted Diff | Diff
Patch (9.24 KB, patch)
2023-08-22 13:41 PDT, Joshua Hoffman
no flags Details | Formatted Diff | Diff
Patch (9.24 KB, patch)
2023-08-23 13:03 PDT, Joshua Hoffman
no flags Details | Formatted Diff | Diff
Patch (9.20 KB, patch)
2023-08-23 21:30 PDT, Joshua Hoffman
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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].