Bug 182834 – Switch to UIWKTextInteractionAssistant for non-editable text

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

[Megan Gardner]

We are switching to UIWKTextInteractionAssistant / WKSelectionGranularityCharacter, as we have removed block mode, so UIWKSelectionAssistant is no longer needed.

[Megan Gardner]

Created attachment 333924 [details]
Trial Patch

[Megan Gardner]

Created attachment 333925 [details]
Test Patch2

[EWS Watchlist]

Comment on attachment 333925 [details]
Test Patch2

Attachment 333925 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/6521812

New failing tests:
imported/blink/editing/selection/deleteFromDocument-crash.html
editing/pasteboard/copy-element-with-conflicting-background-color-from-rule.html
fast/forms/textfield-no-linebreak.html
fast/css-grid-layout/grid-simplified-layout-positioned.html
editing/execCommand/enabling-and-selection-2.html
editing/pasteboard/copy-standalone-image.html
imported/w3c/web-platform-tests/css/selectors/focus-within-004.html
fast/forms/textarea/textarea-placeholder-paint-order-2.html
fast/forms/input-placeholder-paint-order-2.html
fast/css/caret-color-auto.html

[EWS Watchlist]

Created attachment 333933 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6

[Megan Gardner]

Created attachment 334091 [details]
test patch w/extra fix

[Megan Gardner]

Created attachment 334189 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/37687026>

[Wenson Hsieh]

Comment on attachment 334189 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=334189&action=review

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:3995
> +        shouldShowKeyboard = userIsInteracting || _isChangingFocus || changingActivityState;

_isChangingFocus will be true if a <select> is focused — do we want to set `shouldShowKeyboard` to true in that case? It's possible this behavior change is actually desirable (allowing programmatic focus to bring up the keyboard if a select is focused)...perhaps we should get some more opinions on this.

Currently, we only show the keyboard when we're currently focusing an element that also presents a keyboard (see the following code snippet):

    switch (information.elementType) {
    case InputType::Select:
    case InputType::DateTimeLocal:
    case InputType::Time:
    case InputType::Month:
    case InputType::Date:
        break;
    default:
        [self _startAssistingKeyboard];
        break;
    }

...maybe we could factor this out into a helper method and call it here if our existing behavior is something we want to maintain.

[Wenson Hsieh]

Comment on attachment 334189 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=334189&action=review

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:-1865
> -            _webSelectionAssistant = adoptNS([[UIWKSelectionAssistant alloc] initWithView:self]);

I think we can also remove _webSelectionAssistant from WKContentView altogether, since WKContentView's _webSelectionAssistant will always be nil after this change.

[Megan Gardner]

I talked with Tim, and we're ok with slightly expending the situations in which the keyboard will be allowed to be up. 

I will will be removing _webSelectionAssistant in a later patch. This is about getting the switch in so we can make sure everything is going well before ripping everything out.

[Wenson Hsieh]

(In reply to Megan Gardner from comment #10)
> I talked with Tim, and we're ok with slightly expending the situations in
> which the keyboard will be allowed to be up. 
> 
> I will will be removing _webSelectionAssistant in a later patch. This is
> about getting the switch in so we can make sure everything is going well
> before ripping everything out.

Ok! Sounds reasonable.

[Wenson Hsieh]

Comment on attachment 334189 [details]
Patch

r=me

(we may need a WK2 owner to rubber stamp, though)

[Tim Horton]

wk2r=me

[Megan Gardner]

https://trac.webkit.org/r228829

[Matt Lewis]

The API test WebKit.InteractionDeadlockAfterCrash started to fail on iOS Simulator after this revision.

Failure:

FAIL WebKit.InteractionDeadlockAfterCrash

/Volumes/Data/slave/ios-simulator-11-release/build/Tools/TestWebKitAPI/Tests/WebKitCocoa/InteractionDeadlockAfterCrash.mm:74
Value of: shouldBegin
  Actual: false
Expected: true

The test doesn't seem to be crashing, but is failing.

[Matt Lewis]

I was able to reproduce the failure after the revision but not before with:

run-api-tests --debug --ios-simulator WebKit.InteractionDeadlockAfterCrash

[Matt Lewis]

Reverted r228829 for reason:

This caused a consistent failure in the API test WebKit.InteractionDeadlockAfterCrash on iOS Simulator

Committed r228845: <https://trac.webkit.org/changeset/228845>

[Megan Gardner]

Created attachment 334323 [details]
Patch

[Tim Horton]

Comment on attachment 334323 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=334323&action=review

> Source/WebKit/ChangeLog:4
> +        https://bugs.webkit.org/show_bug.cgi?id=182834

rdar here

> Source/WebKit/ChangeLog:7
> +

Did you lose overall patch words from before?

> Source/WebKit/ChangeLog:12
> +        (-[WKContentView gestureRecognizerShouldBegin:]):

Might write some words here about this one (since this is the only new one that isn't obvious).

[EWS Watchlist]

Comment on attachment 334323 [details]
Patch

Attachment 334323 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/6597785

New failing tests:
editing/selection/character-granularity-rect.html

[EWS Watchlist]

Created attachment 334332 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 334323 [details]
Patch

Attachment 334323 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/6598296

New failing tests:
http/tests/preload/download_resources.html
http/tests/preload/onerror_event.html

[EWS Watchlist]

Created attachment 334335 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Megan Gardner]

Created attachment 337215 [details]
Patch

[Andy Estes]

Comment on attachment 337215 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=337215&action=review

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:1906
> +    if (_webSelectionAssistant)
> +        _webSelectionAssistant = nil;

Why not set _webSelectionAssistant to nil unconditionally?

[Megan Gardner]

Created attachment 337509 [details]
Patch

[Wenson Hsieh]

Comment on attachment 337509 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=337509&action=review

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:-1914
> -            _webSelectionAssistant = adoptNS([[UIWKSelectionAssistant alloc] initWithView:self]);

Nit - it seems that after this patch, WKContentView's _webSelectionAssistant will always be nil (since it is only initialized here), so we could consider removing this variable entirely.

[Megan Gardner]

Wenson, Yeah, I will be removing _webSelectionAssistant and a whole lot more, but for now I just want to turn it off and start fixing issues that come up. In case something goes super sideways, I want to make it easy to turn back on, at least for a little while

[Megan Gardner]

https://trac.webkit.org/changeset/230447/webkit

[Ryan Haddad]

I think this change may be responsible for the image failures seen here:

fast/flexbox/flexbox-fail-to-select-same-line.html
fast/text/international/hebrew-selection.html
imported/blink/editing/style/justify-left-crash.html
imported/blink/fast/css/user-select-none.html

https://build.webkit.org/results/Apple%20iOS%2011%20Simulator%20Release%20WK2%20(Tests)/r230644%20(4346)/results.html

All of these tests are flaky failures on iOS.

[Ryan Haddad]

Reverted r230447 for reason:

Caused flaky selection test failures on iOS

Committed r230652: <https://trac.webkit.org/changeset/230652>

[Megan Gardner]

Created attachment 338044 [details]
Patch

[Beth Dakin]

Comment on attachment 338044 [details]
Patch

Make sure to file a follow up bug for the test failures, and otherwise r=me

[Beth Dakin]

Comment on attachment 338044 [details]
Patch

Make sure to file a follow up bug for the test failures, and otherwise r=me

[Megan Gardner]

https://trac.webkit.org/changeset/230686/webkit