FormTokenField: Fix duplicate input in IME composition

[t-hamano]

• Fix FormTokenField: ADD NEW TAG is entered twice in Japanese #45599
• Part of Components: Resolving unintended key events in IME composition #45605

What?

This PR fixes a problem where duplicate values are set when characters are entered into FormTokenField in languages that use IME composition.

Why?

This is because the enter key, which is used to confirm the entry of a character, is considered a token decision.
For more context, see here.

How?

Replaced event.code with event.key.

Testing Instructions

1. Create some post tags.
2. Open the post Editor.
3. Enter text in IME composition mode in the "ADD TAG" sidebar.
4. Confrim that the Enter key to decide your input does NOT insert a token.
5. Confirm that pressing the Enter key again sets the token.

Screenshots or screencast

195cc8d319b16958d11e6fead9ac1a47.mp4

[codesandbox]

  Open in CodeSandbox Web Editor | VS Code | VS Code Insiders

[t-hamano]

Since there was a TODO for key events here, this was also addressed.
If it is unnecessary, I would like to revert it.

[Mamaduka]

I think the event.key for the comma is ,. See: https://www.toptal.com/developers/keycode

[t-hamano]

Thanks for noticing.
You are correct, I have fixed it in 16190d6.

But it is strange that all tests have passed even though the value was wrong. Perhaps we should check for any missing tests as a follow-up to this PR.

[ciampo]

The "should add a token with the input's value when pressing the comma key" test should check for comma keypresses, maybe you could look a bit more into it and why it didn't flag a regression?

[t-hamano]

@ciampo

I have tried various inputs to clarify this issue and have been unable to find a case where onKeyPress occurs.
Also, the determination of the token by comma key appears to be controlled by this other event.
Is there any way to clarify what this event is provided for?

[mirka]

Agreed, tokenization is also done with regex so the comma handling seems redundant.

gutenberg/packages/components/src/form-token-field/index.tsx

Line 258 in 8c9af51

const separator = tokenizeOnSpace ? /[ ,\t]+/ : /[,\t]+/;

So it turns out the test is correct and it isn't a regression! We could bypass this entire onKeyPress handler and it wouldn't matter 😇

gutenberg/packages/components/src/form-token-field/index.tsx

Lines 214 to 223 in 8c9af51

	function onKeyPress( event: KeyboardEvent ) {
	let preventDefault = false;
	// TODO: replace to event.code;
	switch ( event.charCode ) {
	case 44: // Comma.
	preventDefault = handleCommaKey();
	break;
	default:
	break;
	}

Digging into the history, apparently the original code was copied from wp-calypso, and we can see that the regex was added after the keyPress comma handling was already in place, in the context of supporting copy-pasted strings. From that, I would guess that the redundancy is just an oversight. If anything, relying on the regex for commas is probably more reliable given that there was some weirdness with keyCode/charCode differences.

[t-hamano]

I accessed the local site where I built this PR from my iPhone and it seems to work as expected in Chrome on my iPhone.

iphone.mp4

[Mamaduka]

Tested on macOS using Chrome, Safari, and Firefox with English/Georgian keyboards. I couldn't spot any regressions, FormTokenField works as before.

It would be great to get confirmations from folks using other OS to backport this into a minor release.

[benridane]

Tested on Windows 10 using Chrome and Firefox with Japanese. It looks like work fine.

[Mamaduka]

@t-hamano, @mirka, do we have a solution that we can ship with WP 6.1.1?

The RC is scheduled for tomorrow, and I plan to release packages later today.

[mirka]

To play it safe, I'm wondering if we should ship the hotfix simply as a revert to the event.keyCode way. I'm worried about the lack of testing on this new implementation, namely on Android and Linux. This is one of the most unpredictable parts of the browser, and it could potentially cause even more problems.

Unfortunately I don't have a Virtualbox setup anymore and can't easily test for those environments. We might not be able to get testers before the deadline.

[t-hamano]

I committed this suggestion because I thought the method suggested in this comment was the best so far.
But it may indeed not be well tested.

Considering that there is not much time left before the RC release, it may be safe to revert to the previous implementation.

This is also true for #45626 regarding AutoComplete component.

[mirka]

Cool, let's keep this PR to merge into trunk, and open a separate event.keyCode PR for cherry-picking into 6.1.1. @t-hamano Are you still available today to do this? If not, I'd be happy to do it 🙂

[Mamaduka]

Thank you both for the feedback! I agree; that's probably the safest fix we can ship with the minor release.

@t-hamano, do you have time to create a revert to the keyCode hotfix for both components?

[t-hamano]

Yes, I'll work on them 👍

[t-hamano]

I had something to check.
Should PR for WP6.1.1 be checked out from the wp/6.1 branch? (or trunk ?)

[Mamaduka]

6.1 will work as well. Thanks!

[t-hamano]

FormTokenField PR for WP6.1.1 is here: #45703

[t-hamano]

AutoComplete PR for WP6.1.1 is here: #45704

As for the FormTokenField, I will continue to look into this as the unit test is failing.

[ciampo]

Thank you all for working on a solution for this, really appreciate the collaboration. Excuses also for not having tested my keyCode to code PRs properly, I was definitely not considering IME composition.

[mirka]

Retested on Mac Chrome/Firefox/Safari, and it works as expected.

I'm ok to merge this into trunk so we can get more field testing in the plugin track 👍