Bug 166794 - AX: Speak Selection does not work in an iframe
Summary: AX: Speak Selection does not work in an iframe
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: Safari 10
Hardware: iPhone / iPad iOS 10
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-01-06 19:13 PST by Eric Steinlauf
Modified: 2018-04-02 08:49 PDT (History)
14 users (show)

See Also:


Attachments
Screenshot of bad share (244.78 KB, image/png)
2017-01-06 19:31 PST, Eric Steinlauf
no flags Details
Patch (2.26 KB, patch)
2017-01-16 09:10 PST, Eric Steinlauf
no flags Details | Formatted Diff | Diff
patch with test (17.32 KB, patch)
2017-01-20 12:45 PST, Nan Wang
no flags Details | Formatted Diff | Diff
patch with test (18.17 KB, patch)
2017-01-24 10:49 PST, Nan Wang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Steinlauf 2017-01-06 19:13:59 PST
To reproduce
- Go to iPhone settings > General > Accessibility > Speech > toggle on "Speak Selection"
- Go to http://output.jsbin.com/zakocoh/quiet
- Select the text inside the iframe "This is in an iframe"
- Tap "Speak" in the mneu

Expect result:
It reads "This is an an iframe"

Actual result:
It reads the text outside the iframe: "This is before the iframe. This is after the iframe."
Comment 1 Radar WebKit Bug Importer 2017-01-06 19:14:21 PST
<rdar://problem/29913013>
Comment 2 Eric Steinlauf 2017-01-06 19:31:48 PST
Created attachment 298249 [details]
Screenshot of bad share

The problem also occurs when sharing.

- Go to http://output.jsbin.com/zakocoh/quiet
- Select the text inside the iframe "This is in an iframe"
- Tap "Share"
- Tap "New Note"

Expected result:
Shares "This is in an iframe"

Actual result:
Shares "This is before the iframe. This is after the iframe." See screenshot.
Comment 3 Eric Steinlauf 2017-01-09 12:38:10 PST
One other note, "Copy" and "Look up" use the correct text so my naive assumption is that it should be easy to make it work correctly for "Speak" and "Share".
Comment 4 Eric Steinlauf 2017-01-16 09:10:45 PST
Created attachment 298967 [details]
Patch

Change getSelectionOrContentsAsString() to getSelectionContext() so that selection in an iframe returns the correct text.
Comment 5 Eric Steinlauf 2017-01-16 09:20:00 PST
I've never made a change to WebKit before. I was curious so I decided to give it a shot, but I'd be happy to leave this to someone who actually knows what they're doing.

I saw that getSelectionContext() was used in -[WKContentView _lookup:] so it was simple enough to try it in -[WKContentView canPerformAction:withSender:]. Not knowing the code, I don't know if this is reasonable or not. I also wasn't sure about tests.
Comment 6 chris fleizach 2017-01-16 10:31:40 PST
Comment on attachment 298967 [details]
Patch

probably want to leave the _share method alone. there might be a reason to use that method
we can probably figure out how to add a test for this
Comment 7 Eric Steinlauf 2017-01-16 14:41:26 PST
I can leave out _share, though as noted in Comment 2, it seems to have to same problem. Should I file a separate issue for that?

I think I'll need some guidance on tests. I don't any for the existing code.
Comment 8 Simon Fraser (smfr) 2017-01-17 10:18:48 PST
We should also question why WebPage::getSelectionOrContentsAsString() doesn't get iframe content.
Comment 9 Nan Wang 2017-01-20 12:45:01 PST
Created attachment 299368 [details]
patch with test

- Fixed WebPage::getSelectionOrContentsAsString() to get the selection content in the right frame
- Added layout test.
Comment 10 chris fleizach 2017-01-20 12:53:10 PST
Comment on attachment 299368 [details]
patch with test

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

> Source/WebKit2/ChangeLog:8
> +

will this also handle the case of just text nodes highlighted (I ask because I don't know if focus moves to a frame if you just highlight text in that frame)

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:2333
> +{

can you put a comment here about who uses this

> Tools/WebKitTestRunner/ios/UIScriptControllerIOS.mm:116
> +    [webView accessibilityRetrieveSpeakSelectionContentWithCompletionHandler:^(void) {

(void) probably not necessary to include

^{ probably will work

> LayoutTests/accessibility/ios-simulator/speak-selection-content.html:17
> +<input type="text" id="myText" value="Text outside iframe">

let's also test selecting text that's not inside a input type

> LayoutTests/platform/ios-simulator-wk1/TestExpectations:108
> +# Not support on WK1

Not supported
Comment 11 Nan Wang 2017-01-20 13:00:05 PST
Comment on attachment 299368 [details]
patch with test

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

>> Source/WebKit2/ChangeLog:8
>> +
> 
> will this also handle the case of just text nodes highlighted (I ask because I don't know if focus moves to a frame if you just highlight text in that frame)

That's how getSelectionContext() works. So I would say yes.

>> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:2333
>> +{
> 
> can you put a comment here about who uses this

Ok, but currently only the test uses it.

>> Tools/WebKitTestRunner/ios/UIScriptControllerIOS.mm:116
>> +    [webView accessibilityRetrieveSpeakSelectionContentWithCompletionHandler:^(void) {
> 
> (void) probably not necessary to include
> 
> ^{ probably will work

ok

>> LayoutTests/accessibility/ios-simulator/speak-selection-content.html:17
>> +<input type="text" id="myText" value="Text outside iframe">
> 
> let's also test selecting text that's not inside a input type

Ok.
Comment 12 Simon Fraser (smfr) 2017-01-20 13:30:53 PST
Comment on attachment 299368 [details]
patch with test

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

> Source/WebKit2/UIProcess/ios/WKContentViewInteraction.h:234
> +- (void)accessibilityRetrieveSpeakSelectionContent;

Please prefix with an underscore.

> Tools/WebKitTestRunner/cocoa/TestRunnerWKWebView.mm:29
> +#import <JavaScriptCore/JSStringRefCF.h>

Is this really required?
Comment 13 Nan Wang 2017-01-20 13:54:51 PST
Comment on attachment 299368 [details]
patch with test

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

>> Source/WebKit2/UIProcess/ios/WKContentViewInteraction.h:234
>> +- (void)accessibilityRetrieveSpeakSelectionContent;
> 
> Please prefix with an underscore.

This function has been there for years, changing the name would make Speak Selection not work at all. Do you think we should still change it?
Comment 14 Nan Wang 2017-01-24 10:49:58 PST
Created attachment 299612 [details]
patch with test

- updated from review
- Added test case that covers selecting text that's not inside an input type
Comment 15 WebKit Commit Bot 2017-01-24 12:45:55 PST
Comment on attachment 299612 [details]
patch with test

Clearing flags on attachment: 299612

Committed r211095: <http://trac.webkit.org/changeset/211095>
Comment 16 Frédéric Wang (:fredw) 2017-01-27 04:56:41 PST
(In reply to comment #15)
> Comment on attachment 299612 [details]
> patch with test
> 
> Clearing flags on attachment: 299612
> 
> Committed r211095: <http://trac.webkit.org/changeset/211095>

Is this bug fixed?
Comment 17 Nan Wang 2017-01-27 09:31:59 PST
(In reply to comment #16)
> (In reply to comment #15)
> > Comment on attachment 299612 [details]
> > patch with test
> > 
> > Clearing flags on attachment: 299612
> > 
> > Committed r211095: <http://trac.webkit.org/changeset/211095>
> 
> Is this bug fixed?

Yes
Comment 18 Eric Steinlauf 2017-01-30 06:15:49 PST
Great. Thank you.
Comment 19 Frédéric Wang (:fredw) 2017-01-31 02:36:18 PST
(In reply to comment #17)
> > Is this bug fixed?
> 
> Yes

Then I think we can clear the review flag and resolve the bug as FIXED.
Comment 20 Ali G 2017-05-22 09:51:04 PDT
Hi all,

We are still experiencing it with iOS 10.3.2 and 10.3.3. Could we know what version of iOS is planned to contain this fix?

Thanks!
Comment 21 chris fleizach 2017-05-22 09:53:57 PDT
(In reply to Ali G from comment #20)
> Hi all,
> 
> We are still experiencing it with iOS 10.3.2 and 10.3.3. Could we know what
> version of iOS is planned to contain this fix?
> 
> Thanks!

A future version of iOS is all we can say
Comment 22 Frédéric Wang (:fredw) 2018-04-02 08:49:32 PDT
(In reply to Ali G from comment #20)
> Hi all,
> 
> We are still experiencing it with iOS 10.3.2 and 10.3.3. Could we know what
> version of iOS is planned to contain this fix?
> 
> Thanks!

This is fixed in 11.3