RESOLVED FIXED 118680
AX: VoiceOver not working with data detection page overlays
https://bugs.webkit.org/show_bug.cgi?id=118680
Summary AX: VoiceOver not working with data detection page overlays
chris fleizach
Reported 2013-07-15 11:07:48 PDT
Now that data detectors are clients of PageOverlays in WK2, VoiceOver is not able to access them
Attachments
patch (14.93 KB, patch)
2013-07-15 11:09 PDT, chris fleizach
no flags
bump the api version (1.61 KB, patch)
2013-07-17 10:32 PDT, Tim Horton
andersca: review+
patch (11.57 KB, patch)
2013-07-19 09:59 PDT, chris fleizach
sam: review-
eflews.bot: commit-queue-
patch (11.35 KB, patch)
2013-07-19 10:11 PDT, chris fleizach
sam: review-
patch (13.96 KB, patch)
2013-07-23 09:28 PDT, chris fleizach
no flags
patch (14.00 KB, patch)
2013-07-24 09:15 PDT, chris fleizach
no flags
patch (13.65 KB, patch)
2013-07-24 09:20 PDT, chris fleizach
sam: review-
patch (13.92 KB, patch)
2013-07-26 10:00 PDT, chris fleizach
no flags
Radar WebKit Bug Importer
Comment 1 2013-07-15 11:08:12 PDT
chris fleizach
Comment 2 2013-07-15 11:09:48 PDT
WebKit Commit Bot
Comment 3 2013-07-17 09:43:27 PDT
Comment on attachment 206677 [details] patch Clearing flags on attachment: 206677 Committed r152786: <http://trac.webkit.org/changeset/152786>
WebKit Commit Bot
Comment 4 2013-07-17 09:43:28 PDT
All reviewed patches have been landed. Closing bug.
Tim Horton
Comment 5 2013-07-17 10:20:43 PDT
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
Comment 6 2013-07-17 10:28:17 PDT
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
Comment 7 2013-07-17 10:29:48 PDT
(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
Comment 8 2013-07-17 10:32:26 PDT
Created attachment 206899 [details] bump the api version
Tim Horton
Comment 9 2013-07-17 10:37:16 PDT
WebKit Commit Bot
Comment 10 2013-07-17 12:27:55 PDT
Re-opened since this is blocked by bug 118807
chris fleizach
Comment 11 2013-07-19 09:59:09 PDT
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 12 2013-07-19 10:00:02 PDT
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
Comment 13 2013-07-19 10:01:58 PDT
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 14 2013-07-19 10:04:46 PDT
chris fleizach
Comment 15 2013-07-19 10:11:26 PDT
Sam Weinig
Comment 16 2013-07-21 13:56:38 PDT
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 17 2013-07-21 13:57:23 PDT
Comment on attachment 207118 [details] patch Seems I review the wrong version of the patch. Same comment as above.
chris fleizach
Comment 18 2013-07-23 09:28:41 PDT
Tim Horton
Comment 19 2013-07-23 18:31:38 PDT
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
Comment 20 2013-07-24 09:15:51 PDT
chris fleizach
Comment 21 2013-07-24 09:20:32 PDT
Created attachment 207400 [details] patch patch to address Tim's comments
Sam Weinig
Comment 22 2013-07-25 20:50:00 PDT
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
Comment 23 2013-07-26 10:00:06 PDT
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 24 2013-07-27 16:43:14 PDT
Comment on attachment 207537 [details] patch Clearing flags on attachment: 207537 Committed r153404: <http://trac.webkit.org/changeset/153404>
WebKit Commit Bot
Comment 25 2013-07-27 16:43:17 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.