Bug 170589 - AX: PDF plugin needs to support PDF-DOM Mode
Summary: AX: PDF plugin needs to support PDF-DOM Mode
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: chris fleizach
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-04-06 23:49 PDT by chris fleizach
Modified: 2017-04-11 09:26 PDT (History)
13 users (show)

See Also:


Attachments
patch (28.15 KB, patch)
2017-04-06 23:58 PDT, chris fleizach
no flags Details | Formatted Diff | Diff
patch (28.14 KB, patch)
2017-04-07 00:03 PDT, chris fleizach
no flags Details | Formatted Diff | Diff
patch (28.39 KB, patch)
2017-04-07 08:44 PDT, chris fleizach
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description chris fleizach 2017-04-06 23:49:36 PDT
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>
Comment 1 chris fleizach 2017-04-06 23:58:40 PDT
Created attachment 306472 [details]
patch
Comment 2 Radar WebKit Bug Importer 2017-04-06 23:59:17 PDT
<rdar://problem/31495451>
Comment 3 Build Bot 2017-04-07 00:00:50 PDT
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.
Comment 4 chris fleizach 2017-04-07 00:03:48 PDT
Created attachment 306473 [details]
patch
Comment 5 Build Bot 2017-04-07 00:05:48 PDT
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.
Comment 6 Tim Horton 2017-04-07 00:22:21 PDT
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'
Comment 7 chris fleizach 2017-04-07 00:25:40 PDT
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
Comment 8 chris fleizach 2017-04-07 08:44:00 PDT
Created attachment 306508 [details]
patch
Comment 9 chris fleizach 2017-04-07 08:44:21 PDT
(In reply to chris fleizach from comment #8)
> Created attachment 306508 [details]
> patch

Updated with review comments
Comment 10 Build Bot 2017-04-07 08:45:53 PDT
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.
Comment 11 WebKit Commit Bot 2017-04-11 09:26:24 PDT
Comment on attachment 306508 [details]
patch

Clearing flags on attachment: 306508

Committed r215233: <http://trac.webkit.org/changeset/215233>
Comment 12 WebKit Commit Bot 2017-04-11 09:26:26 PDT
All reviewed patches have been landed.  Closing bug.