Bug 260190

Summary: AX: aria-labelledby-on-password-input fails in ITM
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, 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

Description Joshua Hoffman 2023-08-14 19:27:49 PDT
In isolated tree mode, accessibility/aria-labelledby-on-password-input.html fails due to the linked nodes not updating properly.
Comment 1 Radar WebKit Bug Importer 2023-08-14 19:27:56 PDT
<rdar://problem/113885331>
Comment 2 Joshua Hoffman 2023-08-15 11:19:58 PDT
Created attachment 467283 [details]
Patch
Comment 3 Andres Gonzalez 2023-08-16 08:31:22 PDT
(In reply to Joshua Hoffman from comment #2)
> Created attachment 467283 [details]
> Patch

--- a/Source/WebCore/accessibility/AXObjectCache.cpp
+++ b/Source/WebCore/accessibility/AXObjectCache.cpp
@@ -1282,6 +1282,11 @@ void AXObjectCache::handleChildrenChanged(AccessibilityObject& object)
             // Do not let any ancestor of an editable object update its children.
             shouldUpdateParent = false;
         }
+
+        if (auto objects = parent->labelForObjects(); objects.size()) {
+            for (const auto& axObject : objects)
+                postNotification(dynamicDowncast<AccessibilityObject>(axObject.get()), axObject->document(), AXValueChanged);
+        }

handleChildrenChanged doesn't seem the right place to add this notification. Look at

AXObjectCache::handleTextChanged
AXObjectCache::valueChanged


--- a/LayoutTests/accessibility/aria-labelledby-on-password-input.html
+++ b/LayoutTests/accessibility/aria-labelledby-on-password-input.html

-        await waitFor(() => password2.stringValue === "AXValue: \u2022\u2022\u2022\u2022\u2022");
+        await waitFor(() => {
+            return password2.stringValue === "AXValue: \u2022\u2022\u2022\u2022\u2022"
+            && button2.description === "AXDescription: Before password \u2022\u2022\u2022\u2022\u2022 After password";

Indent line starting with &&.
Comment 4 Joshua Hoffman 2023-08-17 12:03:56 PDT
Created attachment 467314 [details]
Patch
Comment 5 Joshua Hoffman 2023-08-17 14:17:25 PDT
(In reply to Andres Gonzalez from comment #3)
> (In reply to Joshua Hoffman from comment #2)
> > Created attachment 467283 [details]
> > Patch
            
> postNotification(dynamicDowncast<AccessibilityObject>(axObject.get()),
> axObject->document(), AXValueChanged);
> +        }
> 
> handleChildrenChanged doesn't seem the right place to add this notification.
> Look at
> 

Placing this codepath inside handleChildrenChanged allows us to update LabelledBy elements regardless of what the change is (since it won't always just be a text change). I collected samples as well and this addition doesn't have an impact on the cost of the method. 

The new test added in the latest patch tests for text changes, to add on to the value change we have covered in the password-input test.
Comment 6 Joshua Hoffman 2023-08-17 15:14:08 PDT
Created attachment 467316 [details]
Patch
Comment 7 Andres Gonzalez 2023-08-18 07:57:09 PDT
(In reply to Joshua Hoffman from comment #6)
> Created attachment 467316 [details]
> Patch

diff --git a/LayoutTests/accessibility/aria-labelledby-text.html b/LayoutTests/accessibility/aria-labelledby-text.html
new file mode 100644
index 000000000000..d453dbdf468b
--- /dev/null
+++ b/LayoutTests/accessibility/aria-labelledby-text.html

+    output += expect("button1.description", "'AXDescription: Label for Button One'");
+    output += expect("button2.description", "'AXDescription: '");
+
+    setTimeout(async function() {
+        await waitFor(() => button1.description === "AXDescription: Label for Button One");
+        output += expect("button1.description", "'AXDescription: Label for Button One'");

No need to repeat this check here since it was already checked in the first expect() above and nothing has changed.
Comment 8 Joshua Hoffman 2023-08-18 11:08:58 PDT
Created attachment 467329 [details]
Patch
Comment 9 EWS 2023-08-21 07:20:34 PDT
Committed 267086@main (d15d7af722dc): <https://commits.webkit.org/267086@main>

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