Bug 260190 - AX: aria-labelledby-on-password-input fails in ITM
Summary: AX: aria-labelledby-on-password-input fails in ITM
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-14 19:27 PDT by Joshua Hoffman
Modified: 2023-08-21 07:20 PDT (History)
10 users (show)

See Also:


Attachments
Patch (3.85 KB, patch)
2023-08-15 11:19 PDT, Joshua Hoffman
no flags Details | Formatted Diff | Diff
Patch (7.61 KB, patch)
2023-08-17 12:03 PDT, Joshua Hoffman
no flags Details | Formatted Diff | Diff
Patch (8.30 KB, patch)
2023-08-17 15:14 PDT, Joshua Hoffman
no flags Details | Formatted Diff | Diff
Patch (8.05 KB, patch)
2023-08-18 11:08 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-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].