Bug 233138 - AX: Stop returning AccessibilityUIElements backed by a null pointer
Summary: AX: Stop returning AccessibilityUIElements backed by a null pointer
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tyler Wilcock
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-11-15 11:03 PST by Tyler Wilcock
Modified: 2021-11-16 06:38 PST (History)
2 users (show)

See Also:


Attachments
Patch (7.82 KB, patch)
2021-11-15 11:18 PST, Tyler Wilcock
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (9.08 KB, patch)
2021-11-15 11:51 PST, Tyler Wilcock
no flags Details | Formatted Diff | Diff
Patch (10.17 KB, patch)
2021-11-15 11:56 PST, Tyler Wilcock
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (12.63 KB, patch)
2021-11-15 12:06 PST, Tyler Wilcock
no flags Details | Formatted Diff | Diff
Patch (12.54 KB, patch)
2021-11-15 14:20 PST, Tyler Wilcock
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (13.51 KB, patch)
2021-11-15 15:08 PST, Tyler Wilcock
no flags Details | Formatted Diff | Diff
Patch (13.60 KB, patch)
2021-11-15 17:13 PST, Tyler Wilcock
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tyler Wilcock 2021-11-15 11:03:54 PST
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);
}
Comment 1 Radar WebKit Bug Importer 2021-11-15 11:04:52 PST
<rdar://problem/85420387>
Comment 2 Tyler Wilcock 2021-11-15 11:18:17 PST
Created attachment 444279 [details]
Patch
Comment 3 Tyler Wilcock 2021-11-15 11:51:50 PST
Created attachment 444287 [details]
Patch
Comment 4 Tyler Wilcock 2021-11-15 11:56:24 PST
Created attachment 444288 [details]
Patch
Comment 5 Andres Gonzalez 2021-11-15 11:57:56 PST
(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.
Comment 6 Tyler Wilcock 2021-11-15 12:06:55 PST
Created attachment 444290 [details]
Patch
Comment 7 Tyler Wilcock 2021-11-15 12:54:00 PST
(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.
Comment 8 Tyler Wilcock 2021-11-15 14:20:15 PST
Created attachment 444304 [details]
Patch
Comment 9 Tyler Wilcock 2021-11-15 15:08:54 PST
Created attachment 444310 [details]
Patch
Comment 10 Andres Gonzalez 2021-11-15 17:06:26 PST
(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.
Comment 11 Tyler Wilcock 2021-11-15 17:13:46 PST
Created attachment 444321 [details]
Patch
Comment 12 EWS 2021-11-16 06:38:26 PST
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].