Bug 233138 – AX: Stop returning AccessibilityUIElements backed by a null pointer

• Component: Accessibility
• Status: RESOLVED FIXED

[Tyler Wilcock]

In a few places, we call AccessibilityUIElement::create() with a nullptr. This returns a Ref<> backed by a null pointer / object, which is confusing. We should probably return a null RefPtr instead.

One example:

Ref<AccessibilityUIElement> AccessibilityController::focusedElement()
{
     auto page = InjectedBundle::singleton().page()->page();
     auto root = static_cast<PlatformUIElement>(WKAccessibilityRootObject(page));
     auto rootElement = AccessibilityUIElement::create(root);
     if (auto focusedElement = rootElement->focusedElement())
         return *focusedElement;
     return AccessibilityUIElement::create(nullptr);
}

[Radar WebKit Bug Importer]

<rdar://problem/85420387>

[Tyler Wilcock]

Created attachment 444279 [details]
Patch

[Tyler Wilcock]

Created attachment 444287 [details]
Patch

[Tyler Wilcock]

Created attachment 444288 [details]
Patch

[Andres Gonzalez]

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

--- a/Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.cpp
+++ a/Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.cpp

-Ref<AccessibilityUIElement> AccessibilityUIElement::create(PlatformUIElement uiElement)
+RefPtr<AccessibilityUIElement> AccessibilityUIElement::create(PlatformUIElement uiElement)

I think it is odd that a class create method can return null. Haven't seen examples of that in WebKit, but I can be convinced otherwise. Instead I think we shouldn't try to create an AccessibilityUIElement if the platform element is null.

+    if (!uiElement)
+        return { };

If this is the way to go, return nullptr here.

--- a/Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.h
+++ a/Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.h

-    static Ref<AccessibilityUIElement> create(PlatformUIElement);
+    static RefPtr<AccessibilityUIElement> create(PlatformUIElement);

Again, create shouldn't return null IMHO.

[Tyler Wilcock]

Created attachment 444290 [details]
Patch

[Tyler Wilcock]

(In reply to Andres Gonzalez from comment #5)
> (In reply to Tyler Wilcock from comment #2)
> > Created attachment 444279 [details]
> > Patch
> 
> --- a/Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.cpp
> +++ a/Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.cpp
> 
> -Ref<AccessibilityUIElement>
> AccessibilityUIElement::create(PlatformUIElement uiElement)
> +RefPtr<AccessibilityUIElement>
> AccessibilityUIElement::create(PlatformUIElement uiElement)
> 
> I think it is odd that a class create method can return null. Haven't seen
> examples of that in WebKit, but I can be convinced otherwise. Instead I
> think we shouldn't try to create an AccessibilityUIElement if the platform
> element is null.
> 
> +    if (!uiElement)
> +        return { };
> 
> If this is the way to go, return nullptr here.
> 
> --- a/Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.h
> +++ a/Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.h
> 
> -    static Ref<AccessibilityUIElement> create(PlatformUIElement);
> +    static RefPtr<AccessibilityUIElement> create(PlatformUIElement);
> 
> Again, create shouldn't return null IMHO.
Yeah, I think that's reasonable. I checked, and you're right that every create method everywhere returns a Ref. I'll change it.

[Tyler Wilcock]

Created attachment 444304 [details]
Patch

[Tyler Wilcock]

Created attachment 444310 [details]
Patch

[Andres Gonzalez]

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

--- a/Tools/WebKitTestRunner/InjectedBundle/win/AccessibilityControllerWin.cpp
+++ a/Tools/WebKitTestRunner/InjectedBundle/win/AccessibilityControllerWin.cpp

-Ref<AccessibilityUIElement> AccessibilityController::focusedElement()
+RefPtr<AccessibilityUIElement> AccessibilityController::focusedElement()
 {
     notImplemented();
     return AccessibilityUIElement::create(nullptr);

we should return nullptr here for consistency.

[Tyler Wilcock]

Created attachment 444321 [details]
Patch

[EWS]

Committed r285859 (244285@main): <https://commits.webkit.org/244285@main>

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