Bug 192824 - [iOS] Keyups for non-modifier keys identified as "Dead" when not focused in a content-editable element
Summary: [iOS] Keyups for non-modifier keys identified as "Dead" when not focused in a...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Local Build
Hardware: iPhone / iPad iOS 12
: P2 Normal
Assignee: Daniel Bates
URL: https://unixpapa.com/js/testkey.html
Keywords: InRadar
: 193255 (view as bug list)
Depends on:
Blocks: 190571 197745 199807
  Show dependency treegraph
 
Reported: 2018-12-18 12:38 PST by Daniel Bates
Modified: 2019-07-15 14:26 PDT (History)
7 users (show)

See Also:


Attachments
Patch and updated tests (25.16 KB, patch)
2019-01-08 16:25 PST, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch and layout tests (25.65 KB, patch)
2019-01-08 16:45 PST, Daniel Bates
no flags Details | Formatted Diff | Diff
To Land once <rdar://problem/47222115> is fixed (26.36 KB, patch)
2019-01-11 16:30 PST, Daniel Bates
no flags Details | Formatted Diff | Diff
To Land once <rdar://problem/47222115> is fixed (30.76 KB, patch)
2019-01-16 20:37 PST, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch and layout test (30.29 KB, patch)
2019-01-23 17:59 PST, Daniel Bates
no flags Details | Formatted Diff | Diff
To land (29.80 KB, patch)
2019-01-30 15:28 PST, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch and layout tests (31.59 KB, patch)
2019-02-08 16:37 PST, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch and layout test (31.55 KB, patch)
2019-02-08 20:31 PST, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch and layout tests (31.50 KB, patch)
2019-02-10 10:18 PST, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch and layout tests (55.57 KB, patch)
2019-02-10 11:07 PST, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch and layout tests (31.59 KB, patch)
2019-02-10 12:43 PST, Daniel Bates
no flags Details | Formatted Diff | Diff
To land (31.39 KB, patch)
2019-02-18 13:11 PST, Daniel Bates
no flags Details | Formatted Diff | Diff
To land (31.47 KB, patch)
2019-02-18 13:14 PST, Daniel Bates
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Bates 2018-12-18 12:38:11 PST
This bug is about pressing a Control modified key command when not focused inside a content-editable field. Bug #192788 tracks fixing this issue inside content-editable fields.

Steps to reproduce:

1. Visit <https://unixpapa.com/js/testkey.html>.
2. Ensure none of the default handlers are suppressed.
3. Ensure Classic and DOM 3 attributes will be shown.
4. Press some Control modified command, say Control + A.

Then the key property of the keyup will show "Dead" as the name of the key. But it should be "a".
Comment 1 Daniel Bates 2019-01-07 16:10:45 PST
Actually, this bug effects all keys.
Comment 2 Radar WebKit Bug Importer 2019-01-07 16:17:41 PST
<rdar://problem/47100332>
Comment 3 Daniel Bates 2019-01-08 13:52:22 PST
Actually, this bug effects only non-modifier keys.
Comment 4 Daniel Bates 2019-01-08 16:25:27 PST
Created attachment 358651 [details]
Patch and updated tests
Comment 5 Daniel Bates 2019-01-08 16:45:39 PST
Created attachment 358654 [details]
Patch and layout tests
Comment 6 Wenson Hsieh 2019-01-09 14:51:46 PST
Comment on attachment 358654 [details]
Patch and layout tests

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

> Source/WebKit/ChangeLog:33
> +        an intercepted UIEvents for app key commands.

Nit - s/an//

> LayoutTests/fast/events/ios/keypress-keys-in-non-editable-element.html:53
> +// FIXME: Test that keypress is not dispatch for "insert" once <rdar://problem/47128940> is fixed.

Nit - …is not dispatched…
Comment 7 Daniel Bates 2019-01-09 15:02:08 PST
*** Bug 193255 has been marked as a duplicate of this bug. ***
Comment 8 Daniel Bates 2019-01-11 16:30:37 PST
Created attachment 358962 [details]
To Land once <rdar://problem/47222115> is fixed

I will land this once the fix for <rdar://problem/47222115> lands.
Comment 9 Daniel Bates 2019-01-11 16:48:22 PST
Comment on attachment 358962 [details]
To Land once <rdar://problem/47222115> is fixed

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

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:3807
> +    // Do not change traits when dismissing the keyboard.
> +    if (_isBlurringFocusedNode)
> +        return _traits.get();

For completeness, this code is new in this version of the patch. We need to do this to ensure that we do not change the keyboard's appearance as part of its dismissal animation now that the keyboard sticks around (though its UI is hidden) when nothing is focused/a non-editable element is focused.
Comment 10 Daniel Bates 2019-01-16 20:37:08 PST
Created attachment 359345 [details]
To Land once <rdar://problem/47222115> is fixed

Condition return value of _disableAutomaticKeyboardUI on whether we are first responder. Otherwise, we interfere with the Find-in-page feature.
Comment 11 Daniel Bates 2019-01-23 17:59:14 PST
Created attachment 359978 [details]
Patch and layout test

Updated patch to fix tab cycling and type ahead for <select>s.
Comment 12 Daniel Bates 2019-01-28 16:33:07 PST
Comment on attachment 359978 [details]
Patch and layout test

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

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:3819
> +    // Do not change traits when dismissing the keyboard.
> +    if (_isBlurringFocusedNode)
> +        return _traits.get();
> +

I think this is fine as-is, but I'll guard this code with USE(UIKIT_KEYBOARD_ADDITIONS) before landing since this is really only needed now that -_requiresKeyboardWhenFirstResponder always returns YES.

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:4657
> +    [self _endEditing];

I think this is fine as-is, but I'll guard this code with USE(UIKIT_KEYBOARD_ADDITIONS) before landing since this is really only needed now that -_requiresKeyboardWhenFirstResponder always returns YES.

> LayoutTests/ChangeLog:14
> +2019-01-23  Daniel Bates  <dabates@apple.com>
> +
> +        [iOS] Keyups for non-modifier keys identified as "Dead" when not focused in a content-editable element
> +        https://bugs.webkit.org/show_bug.cgi?id=192824
> +        <rdar://problem/47100332>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        * fast/events/ios/keydown-keyup-special-keys-in-non-editable-element-expected.txt:
> +        * fast/events/ios/keydown-keyup-special-keys-in-non-editable-element.html:
> +        * fast/events/ios/keypress-keys-in-non-editable-element-expected.txt:
> +        * fast/events/ios/keypress-keys-in-non-editable-element.html:
> +
> +2019-01-11  Daniel Bates  <dabates@apple.com>

Will fix this double ChangeLog before landing.
Comment 13 Daniel Bates 2019-01-30 15:28:00 PST
Created attachment 360632 [details]
To land
Comment 14 WebKit Commit Bot 2019-01-30 16:54:11 PST
Comment on attachment 360632 [details]
To land

Clearing flags on attachment: 360632

Committed r240742: <https://trac.webkit.org/changeset/240742>
Comment 15 WebKit Commit Bot 2019-01-30 16:54:13 PST
All reviewed patches have been landed.  Closing bug.
Comment 16 Ryan Haddad 2019-02-05 10:39:41 PST
Reverted r240742 for reason:

Causes crashes on iOS simulator.

Committed r240983: <https://trac.webkit.org/changeset/240983>
Comment 17 Daniel Bates 2019-02-08 16:37:02 PST
Created attachment 361561 [details]
Patch and layout tests

Patch -applyAutocorrection and -requestAutocorrectionContextWithCompletionHandler to not perform the replacement or compute autocorrection data when we are not in an editable element.
Comment 18 Daniel Bates 2019-02-08 20:31:38 PST
Created attachment 361590 [details]
Patch and layout test
Comment 19 Daniel Bates 2019-02-10 10:18:59 PST
Created attachment 361632 [details]
Patch and layout tests
Comment 20 Daniel Bates 2019-02-10 11:07:48 PST Comment hidden (obsolete)
Comment 21 Daniel Bates 2019-02-10 12:43:33 PST Comment hidden (obsolete)
Comment 22 Wenson Hsieh 2019-02-11 11:00:23 PST
Comment on attachment 361632 [details]
Patch and layout tests

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

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:3400
> +    if (!_page->editorState().isContentEditable) {

This seems like an acceptable workaround for now, but in general we should not be relying on cached state in the UI process for correctness in asynchronous methods like this.

Is there ever a case in which the editor state is not up to date when UIKit asks us for this? I imagine that in -applyAutocorrection: it's probably okay, but I suspect that when we first focus a field to summon the keyboard, -requestAutocorrectionContextWithCompletionHandler: *might* be invoked before our EditorState has arrived in the UI process.
Comment 23 Daniel Bates 2019-02-11 11:08:46 PST
(In reply to Wenson Hsieh from comment #22)
> Comment on attachment 361632 [details]
> Patch and layout tests
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=361632&action=review
> 
> > Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:3400
> > +    if (!_page->editorState().isContentEditable) {
> 
> This seems like an acceptable workaround for now, but in general we should
> not be relying on cached state in the UI process for correctness in
> asynchronous methods like this.
> 
> Is there ever a case in which the editor state is not up to date when UIKit
> asks us for this? I imagine that in -applyAutocorrection: it's probably
> okay, but I suspect that when we first focus a field to summon the keyboard,
> -requestAutocorrectionContextWithCompletionHandler: *might* be invoked
> before our EditorState has arrived in the UI process.

This if-condition was a last minute change. Originally I conditioned on [self _disableAutomaticKeyboardUI]. I just want to avoid calling this function when we are not focused in an editable element. If nothing is focused and editable OR a non-text field is focused (like a <select>) then no need to request or apply corrections, right? Maybe I should go back to [self _disableAutomaticKeyboardUI]? Or is there a better way to get what I want?
Comment 24 Wenson Hsieh 2019-02-11 11:29:40 PST
Comment on attachment 361632 [details]
Patch and layout tests

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

>>> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:3400
>>> +    if (!_page->editorState().isContentEditable) {
>> 
>> This seems like an acceptable workaround for now, but in general we should not be relying on cached state in the UI process for correctness in asynchronous methods like this.
>> 
>> Is there ever a case in which the editor state is not up to date when UIKit asks us for this? I imagine that in -applyAutocorrection: it's probably okay, but I suspect that when we first focus a field to summon the keyboard, -requestAutocorrectionContextWithCompletionHandler: *might* be invoked before our EditorState has arrived in the UI process.
> 
> This if-condition was a last minute change. Originally I conditioned on [self _disableAutomaticKeyboardUI]. I just want to avoid calling this function when we are not focused in an editable element. If nothing is focused and editable OR a non-text field is focused (like a <select>) then no need to request or apply corrections, right? Maybe I should go back to [self _disableAutomaticKeyboardUI]? Or is there a better way to get what I want?

_disableAutomaticKeyboardUI uses focused element info, which should definitely be up to date by the time UIKit calls into us for an autocorrection context. The editor state will arrive later, possibly after UIKit asks us for an autocorrection context.
Comment 25 Daniel Bates 2019-02-11 12:37:46 PST
(In reply to Wenson Hsieh from comment #24)
> Comment on attachment 361632 [details]
> Patch and layout tests
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=361632&action=review
> 
> >>> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:3400
> >>> +    if (!_page->editorState().isContentEditable) {
> >> 
> >> This seems like an acceptable workaround for now, but in general we should not be relying on cached state in the UI process for correctness in asynchronous methods like this.
> >> 
> >> Is there ever a case in which the editor state is not up to date when UIKit asks us for this? I imagine that in -applyAutocorrection: it's probably okay, but I suspect that when we first focus a field to summon the keyboard, -requestAutocorrectionContextWithCompletionHandler: *might* be invoked before our EditorState has arrived in the UI process.
> > 
> > This if-condition was a last minute change. Originally I conditioned on [self _disableAutomaticKeyboardUI]. I just want to avoid calling this function when we are not focused in an editable element. If nothing is focused and editable OR a non-text field is focused (like a <select>) then no need to request or apply corrections, right? Maybe I should go back to [self _disableAutomaticKeyboardUI]? Or is there a better way to get what I want?
> 
> _disableAutomaticKeyboardUI uses focused element info, which should
> definitely be up to date by the time UIKit calls into us for an
> autocorrection context. The editor state will arrive later, possibly after
> UIKit asks us for an autocorrection context.

Okiedokies. Will change to use that.
Comment 26 Daniel Bates 2019-02-18 13:11:35 PST
Created attachment 362317 [details]
To land
Comment 27 Daniel Bates 2019-02-18 13:14:51 PST
Created attachment 362319 [details]
To land
Comment 28 Daniel Bates 2019-02-18 13:16:27 PST
Committed r241734: <https://trac.webkit.org/changeset/241734>