Picking an emoji via the emoji dialog (Ctrl+Cmd+Space) fires inconsistent beforeinput events.

• Component: DOM
• Status: RESOLVED FIXED

To reproduce this, you'll need to use some kind of keyboard event logger (Eg. https://cdn.rawgit.com/w3c/uievents/gh-pages/tools/key-event-viewer.html).

Case 1:
1. Focus on a text field.
2. Open the emoji picker via Ctrl+Cmd+Space.
3. Select an emoji.

Observe that a beforeinput event was fired with the 'insertReplacementText' type and that the emoji picker has become hidden.

Case 2:
1. Focus on a text field
2. Open the emoji picker via Ctrl+Cmd+Space.
3. Move the emoji picker so that it is no longer docked to the text field.
4. Select an emoji.

Observe that a beforeinput event was fired with the 'insertText' type and that the emoji picker has become hidden.

This is inconsistent with the semantics of these event types (as well as native behavior, where inserting an emoji via the picker doesn't replace the text unless there is a selection). These should both be insertText.

[Radar WebKit Bug Importer]

<rdar://problem/31697653>

[Wenson Hsieh]

Created attachment 319133 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 319133 [details]
Patch

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

> LayoutTests/fast/events/before-input-prevent-insert-replacement.html:40
> +        UIHelper.replaceTextAtRange("b", 0, 1)
> +        .then(() => {
> +            write(`The editor now has text content: ${editable.textContent}`);
> +            write("(3) Inserting 'c' after 'a'...");
> +            return UIHelper.replaceTextAtRange("c", 1, 0);
> +        })
> +        .then(() => {
> +            write(`The editor now has text content: ${editable.textContent}`);
> +            write("(4) Selecting all and preventing replacement with 'd'...");
> +            document.execCommand("SelectAll")
> +            return UIHelper.replaceTextAtRange("d", 0, 2);
> +        })
> +        .then(() => {
> +            write(`The editor now has text content: ${editable.textContent}`);
> +            testRunner.notifyDone();
> +        });

We typically put then after the last line like:
(() => ~).then(() => {
})

But why don't we just wrap this in an async function so that we can just do:
await UIHelper.replaceTextAtRange("c", 1, 0);
instead?

> LayoutTests/resources/ui-helper.js:116
> +    static replaceTextAtRange(text, location, length) {
> +        return new Promise(resolve => {
> +            testRunner.runUIScript(`(() => {
> +                uiController.insertText("${text}", ${location}, ${length});

It's strange to call this function replaceTextAtRange when uiController's method is called insertText
We should stick with one or the other, and I think replaceTextAtRange is a more descriptive of the two.

[Wenson Hsieh]

Comment on attachment 319133 [details]
Patch

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

>> LayoutTests/fast/events/before-input-prevent-insert-replacement.html:40
>> +        });
> 
> We typically put then after the last line like:
> (() => ~).then(() => {
> })
> 
> But why don't we just wrap this in an async function so that we can just do:
> await UIHelper.replaceTextAtRange("c", 1, 0);
> instead?

Sounds good -- adopted async/await in these two tests.

>> LayoutTests/resources/ui-helper.js:116
>> +                uiController.insertText("${text}", ${location}, ${length});
> 
> It's strange to call this function replaceTextAtRange when uiController's method is called insertText
> We should stick with one or the other, and I think replaceTextAtRange is a more descriptive of the two.

Ok! I agree -- replaceTextAtRange is much better than insertText. I think this was originally named insertText() to match the platform AppKit -insertText: API, but there's no reason this needs to be the case.

[Wenson Hsieh]

Created attachment 319164 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 319164 [details]
Patch for landing

Clearing flags on attachment: 319164

Committed r221234: <http://trac.webkit.org/changeset/221234>

I'm still seeing a subset of this issue.

To reproduce this, you'll need to use some kind of keyboard event logger (Eg. https://cdn.rawgit.com/w3c/uievents/gh-pages/tools/key-event-viewer.html).

Setup: Populate the text field with content, eg. "abc"

Case 1:
1. Focus on a text field and select "b"
2. Open the emoji picker via Ctrl+Cmd+Space.
3. Select an emoji.

Observe that a beforeinput event was fired with the 'insertReplacementText' type and that the emoji picker has become hidden.

Case 2:
1. Focus on a text field and select "b"
2. Open the emoji picker via Ctrl+Cmd+Space.
3. Move the emoji picker so that it is no longer docked to the text field.
4. Select an emoji.

Observe that a beforeinput event was fired with the 'insertText' type and that the emoji picker has become hidden.

[Wenson Hsieh]

(In reply to charleyroy from comment #7)
> I'm still seeing a subset of this issue.
> 
> To reproduce this, you'll need to use some kind of keyboard event logger
> (Eg.
> https://cdn.rawgit.com/w3c/uievents/gh-pages/tools/key-event-viewer.html).
> 
> Setup: Populate the text field with content, eg. "abc"
> 
> Case 1:
> 1. Focus on a text field and select "b"
> 2. Open the emoji picker via Ctrl+Cmd+Space.
> 3. Select an emoji.
> 
> Observe that a beforeinput event was fired with the 'insertReplacementText'
> type and that the emoji picker has become hidden.
> 
> Case 2:
> 1. Focus on a text field and select "b"
> 2. Open the emoji picker via Ctrl+Cmd+Space.
> 3. Move the emoji picker so that it is no longer docked to the text field.
> 4. Select an emoji.
> 
> Observe that a beforeinput event was fired with the 'insertText' type and
> that the emoji picker has become hidden.

Ah, I see...so on ToT, the reason case 1 fires an insertReplacementText event is that it's using an input-method-like mechanism to replace an existing range of text, whereas in case 2, undocking the emoji picker makes it behave more like an on-screen keyboard, so replacing a selected range using that UI just fires insertText (this is the same as what happens if you select a range of text and type a key on the keyboard, or if you show the on-screen keyboard and press one of the keys to insert text).

The spec (https://www.w3.org/TR/input-events-2/) is somewhat vague about when `InsertReplacementText` should be fired, as opposed to just `insertText`. According to the spec, the `InsertReplacementText` input type is fired when "[replacing] existing text by means of a spell checker, auto-correct or similar". What's happening currently is that in docked mode, the emoji picker behaves like a spell-checking/auto-correct input method, but when undocked, it behaves more like an onscreen keyboard, so the text insertion codepath changes to match that of on-screen keyboard input.

* * *

However, I would've thought that there shouldn't really be a difference between handling `InsertReplacementText` or `InsertText`, since in both cases, WebKit is replacing a (possibly empty) range in the DOM with some data, so the default behavior in both cases is really:

1. Delete the contents of the target range.
2. Insert the event data.

...and the `InsertReplacementText` really just indicates whether the insertion is happening from a source that the platform considers to be some kind of alternate input method. Is there a reason Google Docs might need to distinguish between the cases of `InsertReplacementText` and `InsertText`?

The main difference we were distinguishing between the two was to correctly handle autocorrect. Even if the user has no text selected, selecting a spell-check correction from the touch bar would fire 'insertReplacementText' and would expect the entire word to be replaced. Not sure if there is some alternative in the spec for this scenario? We were potentially expanding the range of the selection depending on whether the event was insertReplacementText and there was a selection (where as we didn't do any expansion for insertText).

[Wenson Hsieh]

(In reply to charleyroy from comment #9)
> The main difference we were distinguishing between the two was to correctly
> handle autocorrect. Even if the user has no text selected, selecting a
> spell-check correction from the touch bar would fire 'insertReplacementText'
> and would expect the entire word to be replaced. Not sure if there is some
> alternative in the spec for this scenario? We were potentially expanding the
> range of the selection depending on whether the event was
> insertReplacementText and there was a selection (where as we didn't do any
> expansion for insertText).

Interesting! So the idea is that if the inputType is "insertReplacementText", then you would prevent default and instead expand the selection range to the word and replace it?

In that case, it seems the emoji picker should only fire input events with "insertText", and we should instead limit "insertReplacementText" to spellcheck and autocorrect. Otherwise, attempting to insert an emoji in the middle of a word would cause us to replace the word with that emoji (which, I assume, would not be desired?)

That is what we were aiming for, but that very well may be considered straying from the spec :) Just unclear what the best way to handle the autocorrect is otherwise.

[Wenson Hsieh]

This was addressed by a change in another part of the OS, which first landed in macOS Mojave (<rdar://problem/36021755>).

[Lucas Forschler]

Mass move bugs into the DOM component.