[iOS] Keyups for non-modifier keys identified as "Dead" when not focused in a content-editable element

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Daniel Bates]

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".

[Daniel Bates]

Actually, this bug effects all keys.

[Radar WebKit Bug Importer]

<rdar://problem/47100332>

[Daniel Bates]

Actually, this bug effects only non-modifier keys.

[Daniel Bates]

Created attachment 358651 [details]
Patch and updated tests

[Daniel Bates]

Created attachment 358654 [details]
Patch and layout tests

[Wenson Hsieh]

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…

[Daniel Bates]

*** Bug 193255 has been marked as a duplicate of this bug. ***

[Daniel Bates]

Created attachment 358962 [details]
To Land once <rdar://problem/47222115> is fixed

I will land this once the fix for <rdar://problem/47222115> lands.

[Daniel Bates]

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.

[Daniel Bates]

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.

[Daniel Bates]

Created attachment 359978 [details]
Patch and layout test

Updated patch to fix tab cycling and type ahead for <select>s.

[Daniel Bates]

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
> [phone]  Daniel Bates  <[e-mail]>
> +
> +        [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:
> +
> [phone]  Daniel Bates  <[e-mail]>

Will fix this double ChangeLog before landing.

[Daniel Bates]

Created attachment 360632 [details]
To land

[WebKit Commit Bot]

Comment on attachment 360632 [details]
To land

Clearing flags on attachment: 360632

Committed r240742: <https://trac.webkit.org/changeset/240742>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryan Haddad]

Reverted r240742 for reason:

Causes crashes on iOS simulator.

Committed r240983: <https://trac.webkit.org/changeset/240983>

[Daniel Bates]

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.

[Daniel Bates]

Created attachment 361590 [details]
Patch and layout test

[Daniel Bates]

Created attachment 361632 [details]
Patch and layout tests

[Daniel Bates]

Created attachment 361634 [details]
Patch and layout tests

[Daniel Bates]

Created attachment 361638 [details]
Patch and layout tests

[Wenson Hsieh]

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.

[Daniel Bates]

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

[Wenson Hsieh]

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.

[Daniel Bates]

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

[Daniel Bates]

Created attachment 362317 [details]
To land

[Daniel Bates]

Created attachment 362319 [details]
To land

[Daniel Bates]

Committed r241734: <https://trac.webkit.org/changeset/241734>