Bug 235246 – AX: Fix disabled-controls-not-focusable.html in isolated tree mode by updating AXPropertyName::CanSetFocusAttribute when the disabled attribute changes

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Tyler Wilcock]

AX: Fix disabled-controls-not-focusable.html in isolated tree mode by updating AXPropertyName::CanSetFocusAttribute when the disabled attribute changes.

[Radar WebKit Bug Importer]

<rdar://problem/87613404>

[Tyler Wilcock]

Created attachment 449207 [details]
Patch

[Andres Gonzalez]

(In reply to Tyler Wilcock from comment #2)
> Created attachment 449207 [details]
> Patch

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

+        case AXDisabledStateChanged:
+            tree->updateNodeProperty(*notification.first, AXPropertyName::CanSetFocusAttribute);
+            break;

Add this same case to the updateIsolatedTree that takes a single notification as opposed to a vector of notifications.

--- a/Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.h
+++ a/Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.h
@@ -139,6 +139,7 @@ public:
     JSRetainPtr<JSStringRef> stringAttributeValue(NSString *attribute) const;
     double numberAttributeValue(NSString *attribute) const;
     RetainPtr<id> attributeValue(NSString *) const;
+    bool isAttributeSettable(NSString *) const;

Can we make this private?

--- a/LayoutTests/accessibility/disabled-controls-not-focusable.html
+++ a/LayoutTests/accessibility/disabled-controls-not-focusable.html

+        async function checkControl(id) {
+            window.element = document.getElementById(id);
+            debug(`Testing #${id}`);
+            element.focus();
+            await waitFor(() => {
+                window.axElement = accessibilityController.focusedElement;
+                return axElement && accessibilityController.accessibleElementById(id);
+            });
+            shouldBeTrue("axElement.isFocusable");
+            debug(`Setting the disabled attribute of element #${id}`);
+            element.disabled = true;
+            await expectAsyncExpression("axElement.isFocusable", "false");
+            debug("");
+        }

The order of operations looks weird to me. I think it should be:
1. Check that an axElement is focusable.
2. Then focus it.
3. Disable the DOM element.
4. Check that now is not focusable.

Not sure that we even have to do 2., because that's not what the test is testing, correct?

[Tyler Wilcock]

Created attachment 449221 [details]
Patch

[Tyler Wilcock]

Thanks for the review -- addressed all of your comments in the latest patch.

[Tyler Wilcock]

(In reply to Andres Gonzalez from comment #3)
> (In reply to Tyler Wilcock from comment #2)
> > Created attachment 449207 [details]
> > Patch
> 
> --- a/Source/WebCore/accessibility/AXObjectCache.cpp
> +++ a/Source/WebCore/accessibility/AXObjectCache.cpp
> 
> +        case AXDisabledStateChanged:
> +            tree->updateNodeProperty(*notification.first,
> AXPropertyName::CanSetFocusAttribute);
> +            break;
> 
> Add this same case to the updateIsolatedTree that takes a single
> notification as opposed to a vector of notifications.
We should refactor this such that we don't have to remember to add new notification handling in two places.

[Tyler Wilcock]

Created attachment 449231 [details]
Patch

[EWS]

Committed r288058 (246078@main): <https://commits.webkit.org/246078@main>

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