Bug 184806 – Fixes for timing out tests associated with switching Text Selection Assistants

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Megan Gardner]

Fixes for failing tests associated with switching Text Selection Assistants

[Megan Gardner]

Created attachment 338392 [details]
Patch

[Megan Gardner]

<rdar://problem/39367905>

[Beth Dakin]

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.

[Wenson Hsieh]

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)

[Wenson Hsieh]

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;

[Beth Dakin]

Oh you should make your patch update the test expectations for the tests that are fixed now too!!!

[Megan Gardner]

Added a test and addressed comments. The tests were marked as skip in internal, so that will have to be a different patch.

[Megan Gardner]

Created attachment 338447 [details]
Patch

[Megan Gardner]

Oh, and I checked, lookup did/does nothing on older builds, so we should not be showing it on caret selections.

[Wenson Hsieh]

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

[Beth Dakin]

Comment on attachment 338447 [details]
Patch

A test!! That is awesome!

[Megan Gardner]

https://trac.webkit.org/r230850