Bug 257377

<rdar://problem/109884868>

Created attachment 466506 [details]
Patch

I suggest the function we add to the header not take any arguments. It can call the internal function that takes and ignores an NSApplication and a SEL.

By the way, the bugs.webkit.org review tool is no longer working for me. I encourage everyone to switch to GitHub pull requests soon.

Comment on attachment 466506 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=466506&action=review

> Source/WebKit/WebProcess/InjectedBundle/API/c/WKBundlePage.cpp:278
> +#if ENABLE(ACCESSIBILITY_ISOLATED_TREE) && USE(APPKIT)
> +    UNUSED_PARAM(pageRef);
> +    return WebKit::WebProcess::accessibilityFocusedUIElement(nil, nil);
> +#else

Prior to this change, accessibility would be enabled (via AXObjectCache::enableAccessibility()), assuming we had a non-null `pageRef`, `page`, and `focusedDocument`. Do we want / need to preserve that behavior somehow?

> Source/WebKit/WebProcess/InjectedBundle/API/c/WKBundlePage.cpp:298
> +#endif

Thoughts on adding // ENABLE(ACCESSIBILITY_ISOLATED_TREE) && USE(APPKIT) after this #endif? This added #endif is sibling to an #else of a different compiler directive, so the extra context on which one is ending here could be nice.

Created attachment 466537 [details]
Patch

(In reply to Darin Adler from comment #3)
> I suggest the function we add to the header not take any arguments. It can
> call the internal function that takes and ignores an NSApplication and a SEL.

Added the method to the header that takes no arguments as you suggested. This static method now implements the functionality. The AppKit override that takes and ignores NSApplication and SEL, now simply calls this method. I think it is much cleaner now. Thanks.

(In reply to Tyler Wilcock from comment #5)
> Comment on attachment 466506 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=466506&action=review
> 
> > Source/WebKit/WebProcess/InjectedBundle/API/c/WKBundlePage.cpp:278
> > +#if ENABLE(ACCESSIBILITY_ISOLATED_TREE) && USE(APPKIT)
> > +    UNUSED_PARAM(pageRef);
> > +    return WebKit::WebProcess::accessibilityFocusedUIElement(nil, nil);
> > +#else
> 
> Prior to this change, accessibility would be enabled (via
> AXObjectCache::enableAccessibility()), assuming we had a non-null `pageRef`,
> `page`, and `focusedDocument`. Do we want / need to preserve that behavior
> somehow?

AXObjectCache::enableAccessibility is called because this path ultimately results in getting the root element, to get the focused element from it, and getting the root does:

- (id)accessibilityRootObjectWrapper
{
#if ENABLE(ACCESSIBILITY_ISOLATED_TREE)
    if (!isMainRunLoop()) {
        if (RefPtr root = m_isolatedTreeRoot.get())
            return root->wrapper();
    }
#endif

    return ax::retrieveAutoreleasedValueFromMainThread<id>([protectedSelf = retainPtr(self)] () -> RetainPtr<id> {
        if (!WebCore::AXObjectCache::accessibilityEnabled())
            WebCore::AXObjectCache::enableAccessibility();


> 
> > Source/WebKit/WebProcess/InjectedBundle/API/c/WKBundlePage.cpp:298
> > +#endif
> 
> Thoughts on adding // ENABLE(ACCESSIBILITY_ISOLATED_TREE) && USE(APPKIT)
> after this #endif? This added #endif is sibling to an #else of a different
> compiler directive, so the extra context on which one is ending here could
> be nice.

Done.

Committed 264706@main (2def5f3d4ef5): <https://commits.webkit.org/264706@main>

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