Bug 170589 – AX: PDF plugin needs to support PDF-DOM Mode

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

[chris fleizach]

PDF is adding support for DOM level access to PDF nodes within the document.

To support that in WebKit, we can't just forward accessibility attributes to the pdfLayerController. 

Now we have to

1) Connect parent/children correctly
2) Support ability to convert outgoing frames and incoming hit points into and out of PDF space
3) Provide ability to connect the PDF annotation elements created in WebKit with their parent PDF annotation nodes

<rdar://problem/31495188>

[chris fleizach]

Created attachment 306472 [details]
patch

[Radar WebKit Bug Importer]

<rdar://problem/31495451>

[Build Bot]

Attachment 306472 [details] did not pass style-queue:


ERROR: Source/WebKit2/WebProcess/Plugins/PDF/PDFPlugin.h:37:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:1692:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebKit2/WebProcess/WebPage/mac/WKAccessibilityWebPageObjectMac.mm:202:  The parameter name "pluginView" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebKit2/WebProcess/Plugins/PDF/PDFPlugin.mm:54:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 4 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[chris fleizach]

Created attachment 306473 [details]
patch

[Build Bot]

Attachment 306473 [details] did not pass style-queue:


ERROR: Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:1692:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 1 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Tim Horton]

Comment on attachment 306473 [details]
patch

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

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:1687
> +- (id)shadowPluginParent

The word "shadow" has a pretty specific meaning in DOMland these days, and PDFPlugin's <input>s are not that. Can we come up with something better?

> Source/WebKit2/WebProcess/Plugins/PDF/PDFPlugin.mm:150
> +- (PDFLayerController *)pdfLayerController

If you're going to implement both, you can drop the @synthesize above, right?

> Source/WebKit2/WebProcess/Plugins/PDF/PDFPlugin.mm:193
> +    if ([attribute isEqualToString:NSAccessibilityChildrenAttribute])
> +        return @[ _pdfLayerController ];
> +    if ([attribute isEqualToString:NSAccessibilityRoleAttribute])
> +        return NSAccessibilityGroupRole;

This seems like a much better plan than the old plan, no idea why we did it that way before.

> Source/WebKit2/WebProcess/Plugins/PDF/PDFPlugin.mm:1763
> +bool PDFPlugin::pluginHandlesContentOffsetForAccessibilityHitTest() const

Why is this specific to the accessibility hit test? Seems like AX + normal hit testing should be the same.

> Source/WebKit2/WebProcess/Plugins/PDF/PDFPlugin.mm:1765
> +    // PDF plugin now handles scroll view and top content inset when convert hit test requests.

Grammar isn't quite right here.

> Source/WebKit2/WebProcess/WebPage/mac/WKAccessibilityWebPageObjectMac.mm:201
> +    BOOL applyContentOffset = true;

Should be a little 'bool'

[chris fleizach]

Comment on attachment 306473 [details]
patch

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

>> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:1687
>> +- (id)shadowPluginParent
> 
> The word "shadow" has a pretty specific meaning in DOMland these days, and PDFPlugin's <input>s are not that. Can we come up with something better?

how about "associatedPluginParent"

>> Source/WebKit2/WebProcess/Plugins/PDF/PDFPlugin.mm:150
>> +- (PDFLayerController *)pdfLayerController
> 
> If you're going to implement both, you can drop the @synthesize above, right?

yes will do

>> Source/WebKit2/WebProcess/Plugins/PDF/PDFPlugin.mm:1763
>> +bool PDFPlugin::pluginHandlesContentOffsetForAccessibilityHitTest() const
> 
> Why is this specific to the accessibility hit test? Seems like AX + normal hit testing should be the same.

its not entirely clear to me, but it appears that the PDFLayer accounts for the scroll offset in its own calculations. sadly this doesn't seem to work the same because of that

>> Source/WebKit2/WebProcess/WebPage/mac/WKAccessibilityWebPageObjectMac.mm:201
>> +    BOOL applyContentOffset = true;
> 
> Should be a little 'bool'

will do

[chris fleizach]

Created attachment 306508 [details]
patch

[chris fleizach]

(In reply to chris fleizach from comment #8)
> Created attachment 306508 [details]
> patch

Updated with review comments

[Build Bot]

Attachment 306508 [details] did not pass style-queue:


ERROR: Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:1692:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 1 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 306508 [details]
patch

Clearing flags on attachment: 306508

Committed r215233: <http://trac.webkit.org/changeset/215233>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.