WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
bump the api version
(1.61 KB, patch)
2013-07-17 10:32 PDT
,
Tim Horton
andersca
: review+
Details
Formatted Diff
Diff
patch
(11.57 KB, patch)
2013-07-19 09:59 PDT
,
chris fleizach
sam
: review-
eflews.bot
: commit-queue-
Details
Formatted Diff
Diff
patch
(11.35 KB, patch)
2013-07-19 10:11 PDT
,
chris fleizach
sam
: review-
Details
Formatted Diff
Diff
patch
(13.96 KB, patch)
2013-07-23 09:28 PDT
,
chris fleizach
no flags
Details
Formatted Diff
Diff
patch
(14.00 KB, patch)
2013-07-24 09:15 PDT
,
chris fleizach
no flags
Details
Formatted Diff
Diff
patch
(13.65 KB, patch)
2013-07-24 09:20 PDT
,
chris fleizach
sam
: review-
Details
Formatted Diff
Diff
patch
(13.92 KB, patch)
2013-07-26 10:00 PDT
,
chris fleizach
no flags
Details
Formatted Diff
Diff
Show Obsolete
(7)
View All
Add attachment
proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1
2013-07-15 11:08:12 PDT
<
rdar://problem/14445270
>
chris fleizach
Comment 2
2013-07-15 11:09:48 PDT
Created
attachment 206677
[details]
patch
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
http://trac.webkit.org/changeset/152789
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
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
Comment 15
2013-07-19 10:11:26 PDT
Created
attachment 207118
[details]
patch
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
Created
attachment 207333
[details]
patch
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
Created
attachment 207399
[details]
patch
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.
Top of Page
Format For Printing
XML
Clone This Bug