URLInput: Skip search requests while an IME composition is in progress - #80602
Conversation
|
Size Change: +49 B (0%) Total Size: 7.81 MB 📦 View Changed
|
|
Warning: Type of PR label mismatch To merge this PR, it requires exactly 1 label indicating the type of PR. Other labels are optional and not being checked here.
Read more about Type labels in Gutenberg. Don't worry if you don't have the required permissions to add labels; the PR reviewer should be able to help with the task. |
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
d42da7e to
f155272
Compare
|
Flaky tests detected in 629cfc5. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/30402410551
|
629cfc5 to
eb0af2b
Compare
| // Whether characters are currently being composed with an IME. Composition | ||
| // state is tracked with explicit `compositionstart` and `compositionend` | ||
| // listeners because the `isComposing` property of native events is not | ||
| // reliable across browsers (e.g. Safari). | ||
| const isComposingRef = useRef( false ); |
There was a problem hiding this comment.
Using state is probably better for this case, then fetch effect remains in a single place that handles suggestions, making handleCompositionEnd a simple callback.
I would also drop the comment.
function handleCompositionEnd() {
setIsComposing( false );
}There was a problem hiding this comment.
Good point, with the composition state as a dependency the fetch logic stays in a single place. The code was updated as suggested. Let me know if this looks good 👍
| } ); | ||
| } ); | ||
|
|
||
| describe( 'IME composition', () => { |
There was a problem hiding this comment.
A bit of refactored tests, should cover most cases and compliment the e2e test.
Tests snippet
it( 'should not fetch suggestions for the intermediate values of an IME composition', async () => {
const { input } = renderURLInput();
fireEvent.compositionStart( input );
fireEvent.change( input, { target: { value: 'ほ' } } );
fireEvent.change( input, { target: { value: 'ほん' } } );
fireEvent.change( input, { target: { value: 'ほんだ' } } );
await flushDebounce();
expect( fetchLinkSuggestions ).not.toHaveBeenCalled();
expect( screen.queryByRole( 'listbox' ) ).not.toBeInTheDocument();
} );
it( 'should not fetch suggestions for a value superseded by an IME composition', async () => {
const { user, input } = renderURLInput();
await user.type( input, 'ab' );
fireEvent.compositionStart( input );
fireEvent.change( input, { target: { value: 'abほ' } } );
await flushDebounce();
expect( fetchLinkSuggestions ).not.toHaveBeenCalled();
} );
// Firefox reports the confirmed value of a composition after
// `compositionend`, Chrome and Safari before it.
// See: https://bugzilla.mozilla.org/show_bug.cgi?id=1305387
it.each( [ 'before', 'after' ] )(
'should fetch suggestions for a composed value reported %s the composition ends',
async ( order ) => {
const { input } = renderURLInput();
fireEvent.compositionStart( input );
fireEvent.change( input, { target: { value: 'ほんだ' } } );
// Compositions outlast the debounce, so the confirmed value is
// the only one a request is made for.
await flushDebounce();
if ( order === 'before' ) {
fireEvent.change( input, { target: { value: 'ホンダ' } } );
fireEvent.compositionEnd( input );
} else {
fireEvent.compositionEnd( input );
fireEvent.change( input, { target: { value: 'ホンダ' } } );
}
expect( await screen.findByRole( 'listbox' ) ).toBeVisible();
expect( fetchLinkSuggestions ).toHaveBeenCalledTimes( 1 );
expect( fetchLinkSuggestions ).toHaveBeenCalledWith( 'ホンダ', {
isInitialSuggestions: false,
} );
}
);There was a problem hiding this comment.
Thank you for the tests! They were applied as suggested, I think covering both event orders in a single parameterized test is better 👍
Co-authored-by: George Mamadashvili <georgemamadashvili@gmail.com>
99a49ba to
418ec3d
Compare
Co-authored-by: George Mamadashvili <georgemamadashvili@gmail.com>
Mamaduka
left a comment
There was a problem hiding this comment.
Thanks for the follow-ups, @jorgefilipecosta!
What?
Fixes #72364.
URLInputvia explicitcompositionstart/compositionendhandlers on the input.compositionend, fetch suggestions once with the confirmed value.URLInputcomposition handling and a Chromium (CDP-driven) IME regression test to the Links e2e suite.Why?
When typing with an IME (Japanese, Chinese, Korean, …), every composition update fires a change event, so
URLInput(and the Link UI built on it) sends/wp/v2/searchrequests for intermediate values while the user is still composing. Requests should wait until the composition is confirmed. This mirrors the composition handling already used by rich text (packages/rich-text/src/hook/event-listeners/input-and-selection.js).Testing Instructions
Automated
To see the failure without the fix:
git checkout trunk -- packages/block-editor/src/components/url-input/index.js npm run test:unit packages/block-editor/src/components/url-input # fails git checkout HEAD -- packages/block-editor/src/components/url-input/index.jsManual
search.honda(ほんだ) in the link field WITHOUT confirming the composition: on trunk each keystroke fires search requests; on this branch nothing fires while the underlined composition text is active.AI usage disclosure: fix and description drafted with AI assistance and reviewed by me.