Bug 229556 – AX: Make PDFs loaded via <embed> accessible

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

[Tyler Wilcock]

Created attachment 436508 [details]
Testcase

<embed name="plugin" src="file:///path/to/example.pdf" type="application/pdf">

These PDFs are rendered fine, but are not accessible to VoiceOver.

[Radar WebKit Bug Importer]

<rdar://problem/82388249>

[Tyler Wilcock]

Created attachment 436512 [details]
WIP

[Tyler Wilcock]

Created attachment 436624 [details]
WIP

[Tyler Wilcock]

Created attachment 436632 [details]
WIP

[Tyler Wilcock]

Created attachment 437630 [details]
Patch

[Tyler Wilcock]

Created attachment 437632 [details]
Patch

[Andres Gonzalez]

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

--- a/Source/WebCore/accessibility/AccessibilityObject.h
+++ a/Source/WebCore/accessibility/AccessibilityObject.h
     Widget* widgetForAttachmentView() const override { return nullptr; }
+    PluginViewBase* pluginViewBase() const override { return nullptr; }

If PluginViewBase derives from Widget, why we need a separate method to retrieve it? widgetForAttachmentView should be enough, right?

[chris fleizach]

Comment on attachment 437632 [details]
Patch

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

> Source/WebCore/accessibility/AccessibilityObject.h:450
> +    PluginViewBase* pluginViewBase() const override { return nullptr; }

while the type is a pluginViewBase, the name seems like it could be just "pluginView"

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1356
> +    if (pluginViewBase())

if (auto pluginView = pluginViewBase())
   return !pluginView->accessibilityObject()

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2010
> +    auto* thisWidget = widget();

if (auto *widget = this->widget())
   return is<PluginViewBase>(widget) ? downcast<PluginViewBase>(widget) : nullptr;

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:1662
> +        return @[pluginViewBase->accessibilityObject()];

we should verify accessibilityObject() is not null before putting it in an array

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:2951
> +        if (axObject->pluginViewBase())

if (auto *pluginView = axObject->pluginViewBase())
   return pluginView->....

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:2952
> +            return axObject->pluginViewBase()->accessibilityHitTest(IntPoint(point));

do we want to handle the case where the returned object is accessibilityIgnored? should we do the default hit test in that case?

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3705
> +    return axObject->pluginViewBase()

you can do a local cache of pluginViewBase since you check it twice

> Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm:255
> +    if ([attribute isEqualToString:axTestObjectTypeAttributeName])

wonder if we should just add a subrole type for this like "AXPDFPluginSubrole" instead of exposing a testing attribute for all clients

> Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm:3069
> +    if (m_accessibilityObject)

this check not necessary, since if it's nil calling axHitTest on it will return nil anyway

> Source/WebKit/WebProcess/Plugins/Plugin.h:41
> +OBJC_CLASS NSArray;

doesn't look like we need to add NSArray here

> LayoutTests/accessibility/mac/basic-embed-pdf-accessibility.html:28
> +            pdfEmbedElement = body.uiElementForSearchPredicate(embedContainer, true, "AXGroupSearchKey", "", false);

can you also do a hit test into the embedded object to ensure we return an element

> LayoutTests/accessibility/mac/basic-embed-pdf-accessibility.html:33
> +            pdfAxObject = pdfEmbedElement.childAtIndex(0);

can you verify reaching into the PDF document to get a node

> LayoutTests/accessibility/mac/basic-embed-pdf-accessibility.html:36
> +

can you verify the parentage is correct for this object

[Andres Gonzalez]

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

--- a/Source/WebCore/plugins/PluginViewBase.h
+++ a/Source/WebCore/plugins/PluginViewBase.h

--- a/Source/WebCore/plugins/PluginViewBase.h
+++ a/Source/WebCore/plugins/PluginViewBase.h

+    virtual NSObject *accessibilityHitTest(const IntPoint&) const { return 0; }
+    virtual NSObject *accessibilityObject() const { return nullptr; }

In both of these you want to return nil. Do we need the return value to be NSObject *? I think in most cases where we return a platform AX object, the return type is id.

[Andres Gonzalez]

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

--- a/Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm
+++ a/Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm

+@property (assign) WebCore::HTMLPlugInElement* pluginElement;

Shouldn't this be a RetainPtr or WeakPtr?

--- a/Source/WebKit/WebProcess/Plugins/Plugin.h
+++ a/Source/WebKit/WebProcess/Plugins/Plugin.h

+    virtual NSObject *accessibilityHitTest(const WebCore::IntPoint&) const { return 0; }

Should return nil.

[Tyler Wilcock]

> can you verify reaching into the PDF document to get a node
> can you also do a hit test into the embedded object to ensure we return an element
Any time I hit test or descend into the PDF tree and try to get an attribute to use for test output, I get a crash like this:

Crashing on exception: -[PDFLayerController accessibilityAttributeValue:]: unrecognized selector sent to instance 0x1567298f0"

Or:

-[PDFLayerController accessibilityAttributeNames]: unrecognized selector sent to instance 0x10622c560

Here’s how I’m hit testing:

hitTestResult = accessibilityController.elementAtPoint(pdfAxObject.width / 2, pdfAxObject.height / 2)
shouldBe("hitTestResult.stringAttributeValue('AXRole')", "1");

Same result using `childAtIndex()` to descend into the PDF tree.

> Do we need the return value to be NSObject *?


I used NSObject * because that’s what PluginView::accessibilityObject() returns without my patch (this is the accessibilityObject we need to connect to the PDF tree):

https://github.com/WebKit/WebKit/blob/c4af6be06909465afa97c8f3490f13d0c42ad1dd/Source/WebKit/WebProcess/Plugins/PluginView.h#L90

I can try switching that to `id`, but I’d have to change downstream callers of that, too.

[Tyler Wilcock]

> I can try switching that to `id`

Nevermind, this turned out to be easy. I'll include it in the next version of the patch.

[Tyler Wilcock]

Created attachment 437808 [details]
Patch

[chris fleizach]

Comment on attachment 437808 [details]
Patch

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

> Source/WebCore/ChangeLog:16
> +        This patch only implements `<embed>` PDF support for Mac â iOS support will

some unicode character here Mac â iOS su

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3833
> +        if (isPluginMatchingCriteria(backingObject, criteria)) {

in this case I feel like it won't return the right data. the search should enumerate all results (like below with findMatchingObject) and then return the count.

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3846
> +        if (isPluginMatchingCriteria(backingObject, criteria)) {

ditto with this. I think for the searching what we need to do is during the findMatchingObjects, including plugins. then here, after we get the results, we need to enumerate them and see if the renderWidgetChildren, and their sub-children and down the tree, match any results, then insert them into the final array

[Andres Gonzalez]

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

--- a/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm
+++ a/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm

@@ -2944,6 +2948,25 @@ ALLOW_DEPRECATED_IMPLEMENTATIONS_END
     if (axObject) {
         if (axObject->isAttachment() && [axObject->wrapper() attachmentView])
             return [axObject->wrapper() attachmentView];
+        auto* widget = axObject->widget();
+        if (is<PluginViewBase>(widget)) {
+            id hitTestResult = widget->accessibilityHitTest(IntPoint(point));
+            BOOL accessibilityIsIgnored = YES;
+            BOOL isAccessibilityElement = NO;
+
+            ALLOW_DEPRECATED_DECLARATIONS_BEGIN
+            if ([hitTestResult respondsToSelector:@selector(accessibilityIsIgnored)])
+                accessibilityIsIgnored = [hitTestResult accessibilityIsIgnored];
+            ALLOW_DEPRECATED_DECLARATIONS_END
+
+            if ([hitTestResult respondsToSelector:@selector(isAccessibilityElement)])
+                isAccessibilityElement = [hitTestResult isAccessibilityElement];
+
+            // Check if either selector exposes the element, because some classes
+            // have conflicting implementations (such as PDFLayerController which returns YES in both cases).
+            if (!accessibilityIsIgnored || isAccessibilityElement)
+                return hitTestResult;
+        }
         return NSAccessibilityUnignoredAncestor(axObject->wrapper());
     }
     return NSAccessibilityUnignoredAncestor(self);

I would suggest getting rid of the bools and changing this logic to:

if (![hitTestResult respondsToSelector:@selector(isAccessibilityElement)]
    || ![hitTestResult isAccessibilityElement])
    return NSAccessibilityUnignoredAncestor(axObject->wrapper());
// isAccessibilityElement is a required selector for all platform accessible objects.

ALLOW_DEPRECATED_DECLARATIONS_BEGIN
if ([hitTestResult respondsToSelector:@selector(accessibilityIsIgnored)])
    && [hitTestResult accessibilityIsIgnored];
    return NSAccessibilityUnignoredAncestor(axObject->wrapper());
ALLOW_DEPRECATED_DECLARATIONS_END
// accessibilityIsIgnored is optional, implemented by web objects, but not necessarily by plugins.

return hitTestResult;

@@ -3694,6 +3717,16 @@ enum class TextUnit {
     });
 }

+static BOOL isPluginMatchingCriteria(AXCoreObject* axObject, const AccessibilitySearchCriteria& criteria)

What is the purpose of this function? Searches can start at any object, plug-in or not, and the results should include all matching objects plug-in or not.

@@ -3797,6 +3830,10 @@ ALLOW_DEPRECATED_IMPLEMENTATIONS_END

     if ([attribute isEqualToString:NSAccessibilityUIElementCountForSearchPredicateParameterizedAttribute]) {
         AccessibilitySearchCriteria criteria = accessibilitySearchCriteriaForSearchPredicateParameterizedAttribute(dictionary);
+        if (isPluginMatchingCriteria(backingObject, criteria)) {
+            auto* widgetChildren = [self renderWidgetChildren];
+            return @(std::min([widgetChildren count], NSUInteger(criteria.resultsLimit)));
+        }

There is no matching performed here, we don't want to return all the children of the starting object.

@@ -3804,6 +3841,13 @@ ALLOW_DEPRECATED_IMPLEMENTATIONS_END

     if ([attribute isEqualToString:NSAccessibilityUIElementsForSearchPredicateParameterizedAttribute]) {
         AccessibilitySearchCriteria criteria = accessibilitySearchCriteriaForSearchPredicateParameterizedAttribute(dictionary);
+        // If our object is a plugin, it won't have non-widget children to search through.
+        // If the criteria allows, search through `renderWidgetChildren` instead.
+        if (isPluginMatchingCriteria(backingObject, criteria)) {
+            auto* widgetChildren = [self renderWidgetChildren];
+            NSUInteger matchingChildrenCount = std::min([widgetChildren count], NSUInteger(criteria.resultsLimit));
+            return [widgetChildren subarrayWithRange:NSMakeRange(0, matchingChildrenCount)];
+        }

Same here, there is no matching done, we don't want to return just the children of the start object.

[Andres Gonzalez]

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

--- a/Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm
+++ a/Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm

+- (id)accessibilityHitTestIntPoint:(const WebCore::IntPoint&)point
+{
+    auto convertedPoint = _pdfPlugin->convertFromRootViewToPDFView(point);
+    return [_pdfLayerController accessibilityHitTest:convertedPoint];
+}
+
 - (id)accessibilityHitTest:(NSPoint)point
 {
-    point = _pdfPlugin->convertFromRootViewToPDFView(WebCore::IntPoint(point));
-    return [_pdfLayerController accessibilityHitTest:point];
+    return [self accessibilityHitTestIntPoint:WebCore::IntPoint(point)];
 }

Why we need to split this method into two?

[Andres Gonzalez]

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

+    if ([attribute isEqual:NSAccessibilityChildrenAttribute] && [element respondsToSelector:@selector(accessibilityChildren)])
+        value = [element accessibilityChildren];
+    else if ([attribute isEqual:NSAccessibilityDescriptionAttribute] && [element respondsToSelector:@selector(accessibilityLabel)])
+        value = [element accessibilityLabel];
+    else if ([attribute isEqual:NSAccessibilityParentAttribute] && [element respondsToSelector:@selector(accessibilityParent)])
+        value = [element accessibilityParent];
+    else if ([attribute isEqual:NSAccessibilityRoleAttribute] && [element respondsToSelector:@selector(accessibilityRole)])
+        value = [element accessibilityRole];
+    else if ([attribute isEqual:NSAccessibilityValueAttribute] && [element respondsToSelector:@selector(accessibilityValue)])
+        value = [element accessibilityValue];

All of this needs to be done inside executeOnAXThreadAndWait for isolated tree mode testing.

[Andres Gonzalez]

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

--- a/LayoutTests/accessibility/mac/basic-embed-pdf-accessibility.html
+++ a/LayoutTests/accessibility/mac/basic-embed-pdf-accessibility.html

+        for (var index of traversalPath) {

better practice is let index instead of var index, because var bumps up the scope of index.

+            await waitFor(() => {
+                pdfEmbedElement = accessibilityController.accessibleElementById("pdfEmbed");
+                return pdfEmbedElement.children.length >= 1;
+            });

return pdfEmbedElement && pdfEmbedElement.children.length >= 1;

wouldn't crash otherwise if pdfEmbedElement is null?

+            await waitFor(() => {
+                pdfAxObject = pdfEmbedElement.childAtIndex(0);
+                return pdfAxObject.children.length >= 1;
+            });

Same as above for pdfAxObject.

+            await waitFor(() => {
+                pdfLayerController = pdfAxObject.childAtIndex(0);
+                return pdfLayerController.children.length >= 1;
+            });

and for pdfLayerController.

[Tyler Wilcock]

> I would suggest getting rid of the bools and changing this logic to:
While your suggestion is fair, it wouldn’t work for the thing we care most about in this bug, which is the PDFLayerController. It has conflicting implementations for `accessibilityIsIgnored` and `isAccessibilityElement` that would cause it to get ignored with your suggested change:



// We need this here as well until rdar://31454793 is resolved
- (BOOL)accessibilityIsIgnored
{
	return YES;
}

- (BOOL)isAccessibilityElement
{
	return YES;
}

[Andres Gonzalez]

(In reply to Tyler Wilcock from comment #19)
> > I would suggest getting rid of the bools and changing this logic to:
> While your suggestion is fair, it wouldn’t work for the thing we care most
> about in this bug, which is the PDFLayerController. It has conflicting
> implementations for `accessibilityIsIgnored` and `isAccessibilityElement`
> that would cause it to get ignored with your suggested change:
> 
> 

// We need this here as well until rdar://31454793 is resolved
> - (BOOL)accessibilityIsIgnored
> {
> 	return YES;
> }
> 
> - (BOOL)isAccessibilityElement
> {
> 	return YES;
> }

It is not necessarily a conflict that an AXElment is also ignored. Why is it a problem that PDFLayerController is ignored?

[Tyler Wilcock]

(In reply to Andres Gonzalez from comment #20)
> (In reply to Tyler Wilcock from comment #19)
> > > I would suggest getting rid of the bools and changing this logic to:
> > While your suggestion is fair, it wouldn’t work for the thing we care most
> > about in this bug, which is the PDFLayerController. It has conflicting
> > implementations for `accessibilityIsIgnored` and `isAccessibilityElement`
> > that would cause it to get ignored with your suggested change:
> > 
> > 

// We need this here as well until rdar://31454793 is resolved
> > - (BOOL)accessibilityIsIgnored
> > {
> > 	return YES;
> > }
> > 
> > - (BOOL)isAccessibilityElement
> > {
> > 	return YES;
> > }
> 
> It is not necessarily a conflict that an AXElment is also ignored. Why is it
> a problem that PDFLayerController is ignored?

It may not a problem in some cases, but it seems to be here. I tested your suggestion which results in the PDFLayerController being ignored, which in turn makes the PDF unable to be hit by hit test via accesstool, the Accessibility Inspector, or my layout test.

Here's the hierarchy, if it's useful:

Embed element renderWidgetChildren
> WKPDFPluginAccessibilityObject
>> PDFLayerController
>>> PDFAccessibilityNodePage
>>>> more PDF nodes all the way down

[Tyler Wilcock]

> Why we need to split this method into two?
I split it into two so that we can handle hit tests of both NSPoint's and WebCore::IntPoints. The NSPoint hit test just converts the point to a WebCore::IntPoint and calls the IntPoint hit test method.

We could probably work entirely in NSPoints, but I'm having issues with the usual NSPoint forward declaration not being sufficient in Widget.h, so I'm inclined to leave it as-is unless you have a suggestion here.

/Users/twilco/projects/web/OpenSource/Source/WebCore/platform/Widget.h:180:44: error: variable has incomplete type 'NSPoint' (aka '_NSPoint')
  virtual id accessibilityHitTest(NSPoint) const { return nil; }

/Users/twilco/projects/web/OpenSource/Source/WebCore/platform/graphics/IntPoint.h:40:16: note: forward declaration of '_NSPoint'
typedef struct _NSPoint NSPoint;

I tried #include <Foundation/NSGeometry.h>, but that causes more issues:

error: expected unqualified-id @class NSString, Protocol;

error: unknown type name 'NSString'

And ~20 more similar errors.

[chris fleizach]

(In reply to Tyler Wilcock from comment #22)
> > Why we need to split this method into two?
> I split it into two so that we can handle hit tests of both NSPoint's and
> WebCore::IntPoints. The NSPoint hit test just converts the point to a
> WebCore::IntPoint and calls the IntPoint hit test method.
> 
> We could probably work entirely in NSPoints, but I'm having issues with the
> usual NSPoint forward declaration not being sufficient in Widget.h, so I'm
> inclined to leave it as-is unless you have a suggestion here.
> 
> /Users/twilco/projects/web/OpenSource/Source/WebCore/platform/Widget.h:180:
> 44: error: variable has incomplete type 'NSPoint' (aka '_NSPoint')
>   virtual id accessibilityHitTest(NSPoint) const { return nil; }
> 
> /Users/twilco/projects/web/OpenSource/Source/WebCore/platform/graphics/
> IntPoint.h:40:16: note: forward declaration of '_NSPoint'
> typedef struct _NSPoint NSPoint;
> 
> I tried #include <Foundation/NSGeometry.h>, but that causes more issues:
> 
> error: expected unqualified-id @class NSString, Protocol;
> 
> error: unknown type name 'NSString'
> 
> And ~20 more similar errors.

We can't use Foundation things inside WebCore code, only the wrappers

[Tyler Wilcock]

Created attachment 437905 [details]
Patch

[chris fleizach]

Comment on attachment 437905 [details]
Patch

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

> Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm:158
>      __unsafe_unretained NSObject *_parent;

can we make this into a WeakObjCPtr now?

[Tyler Wilcock]

Created attachment 437917 [details]
Patch

[Tyler Wilcock]

Created attachment 437930 [details]
Patch

[Tyler Wilcock]

Created attachment 437998 [details]
Patch

[Andres Gonzalez]

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

--- a/Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityUIElementMac.mm
+++ a/Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityUIElementMac.mm
@@ -143,10 +143,34 @@ id attributeValue(id element, NSString *attribute)
 {
     RetainPtr<id> value;

+    // The given `element` may not respond to `accessibilityAttributeValue`, so first check to see if it responds to the attribute-specific selector.
     BEGIN_AX_OBJC_EXCEPTIONS
-    AccessibilityUIElement::s_controller->executeOnAXThreadAndWait([&element, &attribute, &value] {
-        value = [element accessibilityAttributeValue:attribute];
-    });
+    if ([attribute isEqual:NSAccessibilityChildrenAttribute] && [element respondsToSelector:@selector(accessibilityChildren)]) {
+        AccessibilityUIElement::s_controller->executeOnAXThreadAndWait([&element, &value] {
+            value = [element accessibilityChildren];
+        });
+    } else if ([attribute isEqual:NSAccessibilityDescriptionAttribute] && [element respondsToSelector:@selector(accessibilityLabel)]) {
+        AccessibilityUIElement::s_controller->executeOnAXThreadAndWait([&element, &value] {
+            value = [element accessibilityLabel];
+        });
+    } else if ([attribute isEqual:NSAccessibilityParentAttribute] && [element respondsToSelector:@selector(accessibilityParent)]) {
+        AccessibilityUIElement::s_controller->executeOnAXThreadAndWait([&element, &value] {
+            value = [element accessibilityParent];
+        });
+    } else if ([attribute isEqual:NSAccessibilityRoleAttribute] && [element respondsToSelector:@selector(accessibilityRole)]) {
+        AccessibilityUIElement::s_controller->executeOnAXThreadAndWait([&element, &value] {
+            value = [element accessibilityRole];
+        });
+    } else if ([attribute isEqual:NSAccessibilityValueAttribute] && [element respondsToSelector:@selector(accessibilityValue)]) {
+        AccessibilityUIElement::s_controller->executeOnAXThreadAndWait([&element, &value] {
+            value = [element accessibilityValue];
+        });
+    } else {
+        // Fallback to calling `accessibilityAttributeValue`.
+        AccessibilityUIElement::s_controller->executeOnAXThreadAndWait([&element, &attribute, &value] {
+            value = [element accessibilityAttributeValue:attribute];
+        });
+    }

I think you can have just one executeOnAXThreadAndWait and do all the selector checks in one lambda. Also move the comment inside the lambda.

[Tyler Wilcock]

Created attachment 438039 [details]
Patch

[EWS]

Committed r282358 (241621@main): <https://commits.webkit.org/241621@main>

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