Fixes for failing tests associated with switching Text Selection Assistants
Created attachment 338392 [details] Patch
<rdar://problem/39367905>
Comment on attachment 338392 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=338392&action=review I don't think this should block committing this change since this change will fix some tests, but is there any way to write a test to explicitly test that we are allowing double tap at the right times? We should add a mechanism for that kind of test if we don't have one already. > Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:1661 > + if (gesture == UIWKGestureDoubleTap && !(self.isAssistingNode)) Do you need !(self.isAssistingNode) here? There is the early return just about when self.isAssistingNode is true, so I wouldn't think you need it.
Comment on attachment 338392 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=338392&action=review > Source/WebKit/ChangeLog:8 > + The major fix is the disabling the double tap gesture. The other fixes are small I think "…disabling the double tap non-editable text selection gesture." would be more accurate. We still want the double tap gesture in general! > Source/WebKit/ChangeLog:15 > + We should not be allowing a double tap text gestures in non-editable web content. s/allowing a/allowing/ > Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:1660 > + // Don't allow double tap text gestures in Noneditable content Nit - comments should end with a period (https://webkit.org/code-style-guidelines/#comments-sentences). Also, probably s/Noneditable/noneditable/ > Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:1661 > + if (gesture == UIWKGestureDoubleTap && !(self.isAssistingNode)) Nit - s/(self.isAssistingNode)/self.isAssistingNode > Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:2253 > + return hasWebSelection || _page->editorState().selectionIsRange; Note that this would also disable lookup when you have a caret selection when editing text, which isn't what we do now. That being said, pressing "Look up" when you have a caret selection when editing text does nothing in shipping iOS 11.3, so it seems not allowing the lookup action in this case is somewhat of a progression. Not necessarily for this patch, but it would be interesting to know if "Look up" with a caret in editable content ever worked (it would've expected it to look up the word near the current selection). > Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:3152 > + // UIKit does not expect caret selections in noneditable content Period at the end! (https://webkit.org/code-style-guidelines/#comments-sentences)
Comment on attachment 338392 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=338392&action=review >>> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:1661 >>> + if (gesture == UIWKGestureDoubleTap && !(self.isAssistingNode)) >> >> Do you need !(self.isAssistingNode) here? There is the early return just about when self.isAssistingNode is true, so I wouldn't think you need it. > > Nit - s/(self.isAssistingNode)/self.isAssistingNode ^ Actually, what Beth said :) I think this can be simplified to something like: if (gesture == UIWKGestureDoubleTap) return self.isAssistingNode && _positionInformation.nodeAtPositionIsAssistedNode;
Oh you should make your patch update the test expectations for the tests that are fixed now too!!!
Added a test and addressed comments. The tests were marked as skip in internal, so that will have to be a different patch.
Created attachment 338447 [details] Patch
Oh, and I checked, lookup did/does nothing on older builds, so we should not be showing it on caret selections.
(In reply to Megan Gardner from comment #9) > Oh, and I checked, lookup did/does nothing on older builds, so we should not > be showing it on caret selections. 👍 Sounds good!
Comment on attachment 338447 [details] Patch A test!! That is awesome!
https://trac.webkit.org/r230850