Bug 184806 - Fixes for timing out tests associated with switching Text Selection Assistants
Summary: Fixes for timing out tests associated with switching Text Selection Assistants
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Megan Gardner
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-04-19 20:31 PDT by Megan Gardner
Modified: 2018-04-20 13:41 PDT (History)
4 users (show)

See Also:


Attachments
Patch (3.33 KB, patch)
2018-04-19 20:37 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (8.39 KB, patch)
2018-04-20 12:03 PDT, Megan Gardner
bdakin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Megan Gardner 2018-04-19 20:31:49 PDT
Fixes for failing tests associated with switching Text Selection Assistants
Comment 1 Megan Gardner 2018-04-19 20:37:28 PDT
Created attachment 338392 [details]
Patch
Comment 2 Megan Gardner 2018-04-19 20:38:05 PDT
<rdar://problem/39367905>
Comment 3 Beth Dakin 2018-04-20 10:46:09 PDT
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 4 Wenson Hsieh 2018-04-20 10:52:46 PDT
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 5 Wenson Hsieh 2018-04-20 10:56:20 PDT
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;
Comment 6 Beth Dakin 2018-04-20 11:39:46 PDT
Oh you should make your patch update the test expectations for the tests that are fixed now too!!!
Comment 7 Megan Gardner 2018-04-20 12:02:57 PDT
Added a test and addressed comments. The tests were marked as skip in internal, so that will have to be a different patch.
Comment 8 Megan Gardner 2018-04-20 12:03:20 PDT
Created attachment 338447 [details]
Patch
Comment 9 Megan Gardner 2018-04-20 12:04:08 PDT
Oh, and I checked, lookup did/does nothing on older builds, so we should not be showing it on caret selections.
Comment 10 Wenson Hsieh 2018-04-20 12:05:12 PDT
(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 11 Beth Dakin 2018-04-20 12:06:44 PDT
Comment on attachment 338447 [details]
Patch

A test!! That is awesome!
Comment 12 Megan Gardner 2018-04-20 13:41:48 PDT
https://trac.webkit.org/r230850