Bug 182834

Summary: Switch to UIWKTextInteractionAssistant for non-editable text
Product: WebKit Reporter: Megan Gardner <megan_gardner>
Component: New BugsAssignee: Megan Gardner <megan_gardner>
Status: RESOLVED FIXED    
Severity: Normal CC: aestes, bdakin, ews-watchlist, jlewis3, ryanhaddad, thorton, timothy, webkit-bug-importer, wenson_hsieh
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Trial Patch
none
Test Patch2
none
Archive of layout-test-results from ews126 for ios-simulator-wk2
none
test patch w/extra fix
none
Patch
none
Patch
none
Archive of layout-test-results from ews126 for ios-simulator-wk2
none
Archive of layout-test-results from ews206 for win-future
none
Patch
none
Patch
none
Patch bdakin: review+

Description Megan Gardner 2018-02-15 11:23:26 PST
We are switching to UIWKTextInteractionAssistant / WKSelectionGranularityCharacter, as we have removed block mode, so UIWKSelectionAssistant is no longer needed.
Comment 1 Megan Gardner 2018-02-15 11:29:04 PST
Created attachment 333924 [details]
Trial Patch
Comment 2 Megan Gardner 2018-02-15 11:34:15 PST
Created attachment 333925 [details]
Test Patch2
Comment 3 EWS Watchlist 2018-02-15 13:10:44 PST
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
Comment 4 EWS Watchlist 2018-02-15 13:10:46 PST
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
Comment 5 Megan Gardner 2018-02-16 18:19:36 PST
Created attachment 334091 [details]
test patch w/extra fix
Comment 6 Megan Gardner 2018-02-19 14:52:13 PST
Created attachment 334189 [details]
Patch
Comment 7 Radar WebKit Bug Importer 2018-02-19 14:53:21 PST
<rdar://problem/37687026>
Comment 8 Wenson Hsieh 2018-02-19 15:02:46 PST
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.
Comment 9 Wenson Hsieh 2018-02-19 15:05:04 PST
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.
Comment 10 Megan Gardner 2018-02-19 16:27:07 PST
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.
Comment 11 Wenson Hsieh 2018-02-19 17:37:43 PST
(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.
Comment 12 Wenson Hsieh 2018-02-20 12:05:02 PST
Comment on attachment 334189 [details]
Patch

r=me

(we may need a WK2 owner to rubber stamp, though)
Comment 13 Tim Horton 2018-02-20 12:05:43 PST
wk2r=me
Comment 14 Megan Gardner 2018-02-20 12:12:57 PST
https://trac.webkit.org/r228829
Comment 15 Matt Lewis 2018-02-20 14:15:21 PST
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.
Comment 16 Matt Lewis 2018-02-20 14:51:54 PST
I was able to reproduce the failure after the revision but not before with:

run-api-tests --debug --ios-simulator WebKit.InteractionDeadlockAfterCrash
Comment 17 Matt Lewis 2018-02-20 15:02:05 PST
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>
Comment 18 Megan Gardner 2018-02-20 17:16:50 PST
Created attachment 334323 [details]
Patch
Comment 19 Tim Horton 2018-02-20 17:19:46 PST
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).
Comment 20 EWS Watchlist 2018-02-20 18:49:55 PST
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
Comment 21 EWS Watchlist 2018-02-20 18:49:56 PST
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
Comment 22 EWS Watchlist 2018-02-20 19:46:10 PST
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
Comment 23 EWS Watchlist 2018-02-20 19:46:20 PST
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
Comment 24 Megan Gardner 2018-04-04 13:11:37 PDT
Created attachment 337215 [details]
Patch
Comment 25 Andy Estes 2018-04-05 14:36:34 PDT
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?
Comment 26 Megan Gardner 2018-04-09 10:57:05 PDT
Created attachment 337509 [details]
Patch
Comment 27 Wenson Hsieh 2018-04-09 12:13:52 PDT
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.
Comment 28 Megan Gardner 2018-04-09 12:27:59 PDT
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
Comment 29 Megan Gardner 2018-04-09 12:31:38 PDT
https://trac.webkit.org/changeset/230447/webkit
Comment 30 Ryan Haddad 2018-04-13 17:35:03 PDT
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.
Comment 31 Ryan Haddad 2018-04-13 18:07:30 PDT
Reverted r230447 for reason:

Caused flaky selection test failures on iOS

Committed r230652: <https://trac.webkit.org/changeset/230652>
Comment 32 Megan Gardner 2018-04-16 15:14:13 PDT
Created attachment 338044 [details]
Patch
Comment 33 Beth Dakin 2018-04-16 15:18:50 PDT
Comment on attachment 338044 [details]
Patch

Make sure to file a follow up bug for the test failures, and otherwise r=me
Comment 34 Beth Dakin 2018-04-16 15:19:18 PDT
Comment on attachment 338044 [details]
Patch

Make sure to file a follow up bug for the test failures, and otherwise r=me
Comment 35 Megan Gardner 2018-04-16 15:48:28 PDT
https://trac.webkit.org/changeset/230686/webkit