Summary: | AX: Speak Selection does not work in an iframe | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Eric Steinlauf <ericfs> | ||||||||||
Component: | Accessibility | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | aboxhall, ali.ghassemi, apinheiro, cfleizach, commit-queue, dmazzoni, fred.wang, jcraig, jdiggs, mario, n_wang, samuel_white, simon.fraser, webkit-bug-importer | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | Safari 10 | ||||||||||||
Hardware: | iPhone / iPad | ||||||||||||
OS: | iOS 10 | ||||||||||||
Bug Depends on: | |||||||||||||
Bug Blocks: | 210645 | ||||||||||||
Attachments: |
|
Description
Eric Steinlauf
2017-01-06 19:13:59 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. 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". Created attachment 298967 [details]
Patch
Change getSelectionOrContentsAsString() to getSelectionContext() so that selection in an iframe returns the correct text.
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 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
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. We should also question why WebPage::getSelectionOrContentsAsString() doesn't get iframe content. Created attachment 299368 [details]
patch with test
- Fixed WebPage::getSelectionOrContentsAsString() to get the selection content in the right frame
- Added layout test.
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 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 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 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? 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 on attachment 299612 [details] patch with test Clearing flags on attachment: 299612 Committed r211095: <http://trac.webkit.org/changeset/211095> (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? (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 Great. Thank you. (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. 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! (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 (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 |