Bug 118680 – AX: VoiceOver not working with data detection page overlays

• Component: Accessibility
• Status: RESOLVED FIXED

[chris fleizach]

Now that data detectors are clients of PageOverlays in WK2, VoiceOver is not able to access them

[Radar WebKit Bug Importer]

<rdar://problem/14445270>

[chris fleizach]

Created attachment 206677 [details]
patch

[WebKit Commit Bot]

Comment on attachment 206677 [details]
patch

Clearing flags on attachment: 206677

Committed r152786: <http://trac.webkit.org/changeset/152786>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Tim Horton]

Ahg, you need to bump the API version and put the new one in its own section (with a version comment). Please do that ASAP (or if you can't let me know so I can).

[chris fleizach]

Alright looking into. If you're in a better position to do that let me know. It will take at least 30 minutes to get webkit build and updated and figure out what i need to do

[Tim Horton]

(In reply to comment #6)
> Alright looking into. If you're in a better position to do that let me know. It will take at least 30 minutes to get webkit build and updated and figure out what i need to do

OK I'll do it.

[Tim Horton]

Created attachment 206899 [details]
bump the api version

[Tim Horton]

http://trac.webkit.org/changeset/152789

[WebKit Commit Bot]

Re-opened since this is blocked by bug 118807

[chris fleizach]

Created attachment 207112 [details]
patch

Updated patch to be more generic for page overlays -- now they're just asked in the general case if they have answers for accessibility attributes.

[chris fleizach]

Comment on attachment 207112 [details]
patch

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

> Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePageOverlay.cpp:-128
> -    if (wkClient && wkClient->version)

Is this the correct removal? It seemed like it since the version number is now 1

[WebKit Commit Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePageOverlay.cpp', u'Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePageOverlay.h', u'Source/WebKit2/WebProcess/WebPage/PageOverlay.cpp', u'Source/WebKit2/WebProcess/WebPage/PageOverlay.h', u'Source/WebKit2/WebProcess/WebPage/WebPage.cpp', u'Source/WebKit2/WebProcess/WebPage/WebPage.h', u'Source/WebKit2/WebProcess/WebPage/mac/WKAccessibilityWebPageObject.mm']" exit_code: 1
Source/WebKit2/WebProcess/WebPage/PageOverlay.cpp:84:  More than one command on the same line in if  [whitespace/parens] [4]
Source/WebKit2/WebProcess/WebPage/PageOverlay.cpp:84:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 2 in 8 files


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

[EFL EWS Bot]

Comment on attachment 207112 [details]
patch

Attachment 207112 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/1098985

[chris fleizach]

Created attachment 207118 [details]
patch

[Sam Weinig]

Comment on attachment 207112 [details]
patch

I would prefer if we could could separate things here.  I think there should be a a WKBundlePageOverlayAccessibilityClient that this new callback gets added to.

[Sam Weinig]

Comment on attachment 207118 [details]
patch

Seems I review the wrong version of the patch.  Same comment as above.

[chris fleizach]

Created attachment 207333 [details]
patch

[Tim Horton]

Comment on attachment 207333 [details]
patch

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

> Source/WebKit2/WebProcess/WebPage/mac/WKAccessibilityWebPageObject.mm:49
> +static NSString *NSAccessibilityDataDetectorExistsAtPoint        = @"AXDataDetectorExistsAtPoint";
> +static NSString *NSAccessibilityDidShowDataDetectorMenuAtPoint   = @"AXDidShowDataDetectorMenuAtPoint";
> +static NSString *NSAccessibilityDataDetectorTypeAtPoint          = @"AXDataDetectorTypeAtPoint";

Shouldn't these have a suffix "Key" or "Attribute" or something?

> Source/WebKit2/WebProcess/WebPage/mac/WKAccessibilityWebPageObject.mm:201
> +        pageOverlayParameter = WKPointCreate(WKPointMake(point.x, point.y));

Who owns/releases this WKPoint?

[chris fleizach]

Created attachment 207399 [details]
patch

[chris fleizach]

Created attachment 207400 [details]
patch

patch to address Tim's comments

[Sam Weinig]

Comment on attachment 207400 [details]
patch

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

> WebProcess/InjectedBundle/API/c/WKBundlePageOverlay.h:51
> +typedef WKTypeRef (*WKAccessibilityAttributeValueCallback)(WKBundlePageOverlayRef pageOverlay, WKStringRef attribute, WKTypeRef parameter, const void* clientInfo);

This should go right above the WKBundlePageOverlayAccessibilityClient.

> WebProcess/InjectedBundle/API/c/WKBundlePageOverlay.h:65
>  };
> +
>  typedef struct WKBundlePageOverlayClient WKBundlePageOverlayClient;

This space is not necessary..

> WebProcess/InjectedBundle/API/c/WKBundlePageOverlay.h:75
>  enum { kWKBundlePageOverlayClientCurrentVersion = 0 };
> +enum { kWKBundlePageOverlayAccessibilityClientCurrentVersion = 0 };

The enum with kWKBundlePageOverlayClientCurrentVersion should go next to the WKBundlePageOverlayClient.

> WebProcess/InjectedBundle/API/c/WKBundlePageOverlay.h:80
>  WK_EXPORT WKBundlePageOverlayRef WKBundlePageOverlayCreate(WKBundlePageOverlayClient* client);
> +WK_EXPORT WKBundlePageOverlayRef WKBundlePageOverlayCreateWithAccessibilityClient(WKBundlePageOverlayClient* client, WKBundlePageOverlayAccessibilityClient* accessibilityClient);

I would prefer if this was a WKBundlePageOverlaySetAccessibilityClient() function.

> WebProcess/WebPage/mac/WKAccessibilityWebPageObject.mm:49
> +static NSString *NSAccessibilityDataDetectorExistsAtPointKey        = @"AXDataDetectorExistsAtPoint";
> +static NSString *NSAccessibilityDidShowDataDetectorMenuAtPointKey   = @"AXDidShowDataDetectorMenuAtPoint";
> +static NSString *NSAccessibilityDataDetectorTypeAtPointKey          = @"AXDataDetectorTypeAtPoint";

Please put a FIXME here, about removing knowledge of data detectors in the future. 

 I am not clear why this is necessary, can we structure this in a way the client, the only party that knows the overlay is being used for data detectors, is the one with this knowledge?

[chris fleizach]

Created attachment 207537 [details]
patch

Patch to address Sam's comments
   - Remove all detection of "data detectors" from WK2
   - Make the accessibility client callbacks get set in their own method (instead of in the initializer) - (Required exposing some back pointers in PageOverlay)

[WebKit Commit Bot]

Comment on attachment 207537 [details]
patch

Clearing flags on attachment: 207537

Committed r153404: <http://trac.webkit.org/changeset/153404>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.