Bug 166794 – AX: Speak Selection does not work in an iframe

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Eric Steinlauf]

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."

[Radar WebKit Bug Importer]

<rdar://problem/29913013>

[Eric Steinlauf]

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.

[Eric Steinlauf]

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".

[Eric Steinlauf]

Created attachment 298967 [details]
Patch

Change getSelectionOrContentsAsString() to getSelectionContext() so that selection in an iframe returns the correct text.

[Eric Steinlauf]

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.

[chris fleizach]

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

[Eric Steinlauf]

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.

[Simon Fraser (smfr)]

We should also question why WebPage::getSelectionOrContentsAsString() doesn't get iframe content.

[Nan Wang]

Created attachment 299368 [details]
patch with test

- Fixed WebPage::getSelectionOrContentsAsString() to get the selection content in the right frame
- Added layout test.

[chris fleizach]

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

[Nan Wang]

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.

[Simon Fraser (smfr)]

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?

[Nan Wang]

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?

[Nan Wang]

Created attachment 299612 [details]
patch with test

- updated from review
- Added test case that covers selecting text that's not inside an input type

[WebKit Commit Bot]

Comment on attachment 299612 [details]
patch with test

Clearing flags on attachment: 299612

Committed r211095: <http://trac.webkit.org/changeset/211095>

[Frédéric Wang (:fredw)]

(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?

[Nan Wang]

(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

[Eric Steinlauf]

Great. Thank you.

[Frédéric Wang (:fredw)]

(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.

[Ali G]

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!

[chris fleizach]

(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

[Frédéric Wang (:fredw)]

(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