Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WP6.1.1] FormTokenField: Revert to event.keyCode to fix IME composition issue #45703

Merged
merged 3 commits into from
Nov 10, 2022

Conversation

t-hamano
Copy link
Contributor

@t-hamano t-hamano commented Nov 10, 2022

What?

This PR is backported to WP6.1.1 and reverts event.code to event.keyCode, one of the changes made to FormTokenField in #43442.

This separate PR # 45607 will be applied to Gutenberg's latest trunk.

Why?

The background behind the creation of this PR can be found after the comments here.

@codesandbox
Copy link

codesandbox bot commented Nov 10, 2022

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

@t-hamano t-hamano changed the title [WP6.1.1] FormTokenField: Revert to event.keyCode to fix IME compos… [WP6.1.1] FormTokenField: Revert to event.keyCode to fix IME composition issue Nov 10, 2022
@t-hamano t-hamano marked this pull request as ready for review November 10, 2022 14:01
@t-hamano t-hamano requested a review from ajitbohra as a code owner November 10, 2022 14:01
@Mamaduka
Copy link
Member

I think unit tests are failing because event.keyCode doesn't work with RTL.

@tyxla, any suggestions on how we can fix this?

@tyxla
Copy link
Member

tyxla commented Nov 10, 2022

I think unit tests are failing because event.keyCode doesn't work with RTL.

@tyxla, any suggestions on how we can fix this?

@testing-library also supports key if code doesn't work for us. That would certainly fix the tests. Let me push a commit.

@t-hamano
Copy link
Contributor Author

@tyxla
Thank you so much for your help 🙏

@tyxla
Copy link
Member

tyxla commented Nov 10, 2022

Pushed 3146412 - let me know if it works well.

Alternatively, if that doesn't work, we can just fall back to using fireEvent for tests in these particular instances.

@t-hamano
Copy link
Contributor Author

@tyxla
For this PR, we need to use event.keyCode, not event.key.
Please see comments beginning here.

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 🙂

Therefore, I believe that the test may need to be rewritten using fireEvent.

@Mamaduka
Copy link
Member

@tyxla, we decided to revert to the deprecated event.keyCode to fix the IME bug. The update implementation of event.key or event.code isn't tested across different OS and languages.

See the discussion at #45607 (comment) for more details.

Alternatively, if that doesn't work, we can just fall back to using fireEvent for tests in these particular instances.

I think we might need this change to get tests working with event.keyCode.

@tyxla
Copy link
Member

tyxla commented Nov 10, 2022

Cool, that makes sense. I'm happy to help with that migration FWIW.

@t-hamano
Copy link
Contributor Author

We need to revert the refactoring from event.keyCode to event.code done in #43442. But this PR includes a rewrite to the RTL test.

Should we revert this entire PR?

@tyxla
Copy link
Member

tyxla commented Nov 10, 2022

It would also revert the TS migration done there FWIW. cc @ciampo

Reverting the full PR might make sense in the short term, we can do a take 2 afterwards.

@Mamaduka
Copy link
Member

It should be a "hotfix" for WP 6.1.1, so let's do what you think would be the simplest solution.

@t-hamano
Copy link
Contributor Author

I am trying to refactor the following code to make it pass the tests.

fireEvent.keyDown( input, {
	keyCode: BACKSPACE,
} );

However, it fails for some tests, so it may be badly written

@t-hamano t-hamano force-pushed the fix-6.1.1/formtokenfield-ime branch from 72019c7 to 450b9a4 Compare November 10, 2022 16:44
@t-hamano
Copy link
Contributor Author

t-hamano commented Nov 10, 2022

I adjusted the tests to pass while keeping the event.keyCode implementation.
This is how it looks:

await user.type( input, 'dragon fruit[Enter]' );

// to...

function triggerEnter( element: Element ) {
	fireEvent.keyDown( element, {
		keyCode: ENTER,
	} );
}

await user.type( input, 'dragon fruit' );
triggerEnter( input );

@t-hamano t-hamano self-assigned this Nov 10, 2022
@t-hamano t-hamano added [Package] Components /packages/components Backport to WP Minor Release Pull request that needs to be backported to a WordPress minor release labels Nov 10, 2022
Copy link
Member

@mirka mirka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code is working as expected, tested on Mac Chrome/Firefox/Safari 👍

@Mamaduka Mamaduka merged commit cfcee1c into wp/6.1 Nov 10, 2022
@Mamaduka Mamaduka deleted the fix-6.1.1/formtokenfield-ime branch November 10, 2022 18:23
@Mamaduka Mamaduka added the [Type] Bug An existing feature does not function as intended label Nov 11, 2022
@Mamaduka Mamaduka removed the Backport to WP Minor Release Pull request that needs to be backported to a WordPress minor release label Mar 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Components /packages/components [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants