Summary: | Switch to UIWKTextInteractionAssistant for non-editable text | ||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Megan Gardner <megan_gardner> | ||||||||||||||||||||||||
Component: | New Bugs | Assignee: | 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
Megan Gardner
2018-02-15 11:23:26 PST
Created attachment 333924 [details]
Trial Patch
Created attachment 333925 [details]
Test Patch2
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 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
Created attachment 334091 [details]
test patch w/extra fix
Created attachment 334189 [details]
Patch
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 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. 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. (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 on attachment 334189 [details]
Patch
r=me
(we may need a WK2 owner to rubber stamp, though)
wk2r=me 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. I was able to reproduce the failure after the revision but not before with: run-api-tests --debug --ios-simulator WebKit.InteractionDeadlockAfterCrash 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> Created attachment 334323 [details]
Patch
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 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 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 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 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
Created attachment 337215 [details]
Patch
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? Created attachment 337509 [details]
Patch
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. 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 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. Reverted r230447 for reason: Caused flaky selection test failures on iOS Committed r230652: <https://trac.webkit.org/changeset/230652> Created attachment 338044 [details]
Patch
Comment on attachment 338044 [details]
Patch
Make sure to file a follow up bug for the test failures, and otherwise r=me
Comment on attachment 338044 [details]
Patch
Make sure to file a follow up bug for the test failures, and otherwise r=me
|