ComboboxControl: SuggestionsList loses focus on DOWN key press

[mahnunchik]

Description

Whole block including ComboboxControl loses focus on DOWN key press.

Step-by-step reproduction instructions

1. Create custom block with ComboboxControl
2. Start typing to have 2+ options to be shown.
3. Press DOWN

Expected behavior: selected next option.

Actual behavior: block loses focus, options hidden.

Screenshots, screen recording, code snippet

https://developer.wordpress.org/block-editor/reference-guides/components/combobox-control/

import { ComboboxControl } from '@wordpress/components';
import { useState } from '@wordpress/element';
 
const options = [
    {
        value: 'small',
        label: 'Small',
    },
    {
        value: 'normal',
        label: 'Normal',
    },
    {
        value: 'large',
        label: 'Large',
    },
    {
        value: 'huge',
        label: 'Huge',
    },
];
 
function MyComboboxControl() {
    const [ fontSize, setFontSize ] = useState();
    const [ filteredOptions, setFilteredOptions ] = useState( options );
    return (
        <ComboboxControl
            label="Font Size"
            value={ fontSize }
            onChange={ setFontSize }
            options={ filteredOptions }
            onFilterValueChange={ ( inputValue ) =>
                setFilteredOptions(
                    options.filter( ( option ) =>
                        option.label
                            .toLowerCase()
                            .startsWith( inputValue.toLowerCase() )
                    )
                )
            }
        />
    );
}

Keys should be handled properly, source:

gutenberg/packages/components/src/combobox-control/index.js

Lines 107 to 141 in 39f883e

	const onKeyDown = ( event ) => {
	let preventDefault = false;
	if ( event.defaultPrevented ) {
	return;
	}
	switch ( event.keyCode ) {
	case ENTER:
	if ( selectedSuggestion ) {
	onSuggestionSelected( selectedSuggestion );
	preventDefault = true;
	}
	break;
	case UP:
	handleArrowNavigation( -1 );
	preventDefault = true;
	break;
	case DOWN:
	handleArrowNavigation( 1 );
	preventDefault = true;
	break;
	case ESCAPE:
	setIsExpanded( false );
	setSelectedSuggestion( null );
	preventDefault = true;
	break;
	default:
	break;
	}
	if ( preventDefault ) {
	event.preventDefault();
	}
	};

Environment info

• WordPress 5.9
• Chrome 97.0.4692.99
• macOS 12.2

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

Yes

[mahnunchik]

Key UP works as expected.

[t-hamano]

I have tested it in a similar environment and confirmed that the same symptom occurs with UP key.

The option does not change and it loses focus if there is a block or title element before / after the ComboboxControl.

For some reason, I think the keyboard events are not working.

Please see:

05e9b68efa61a5aa0ba1622777ce4bcf.mp4

• 00:00～: There is a block before ComboboxControl.
• 00:17～: There is a block after ComboboxControl.
• 00:30～: There is a block before and after ComboboxControl.

Environment info

• WordPress version: 5.9
• Gutenberg version: 12.5.3
• Chrome 97.0.4692.99
• OS: Windows 11

[t-hamano]

I don't know the details, but I removed one of the event handlers registered in .components-combobox-control__suggestions-container, and it seems to work correctly.

[mahnunchik]

Just for the information, FormTokenField component has almost the same implementation of key handler:

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

Lines 141 to 189 in 5c0b338

	onKeyDown( event ) {
	let preventDefault = false;
	if ( event.defaultPrevented ) {
	return;
	}
	switch ( event.keyCode ) {
	case BACKSPACE:
	preventDefault = this.handleDeleteKey(
	this.deleteTokenBeforeInput
	);
	break;
	case ENTER:
	preventDefault = this.addCurrentToken();
	break;
	case LEFT:
	preventDefault = this.handleLeftArrowKey();
	break;
	case UP:
	preventDefault = this.handleUpArrowKey();
	break;
	case RIGHT:
	preventDefault = this.handleRightArrowKey();
	break;
	case DOWN:
	preventDefault = this.handleDownArrowKey();
	break;
	case DELETE:
	preventDefault = this.handleDeleteKey(
	this.deleteTokenAfterInput
	);
	break;
	case SPACE:
	if ( this.props.tokenizeOnSpace ) {
	preventDefault = this.addCurrentToken();
	}
	break;
	case ESCAPE:
	preventDefault = this.handleEscapeKey( event );
	break;
	default:
	break;
	}
	if ( preventDefault ) {
	event.preventDefault();
	}
	}

I'm unable to check it now, but maybe (a) it has the same issue (b) the issue is already fixed there.

[stefanfisk]

I this caused by some parts of the code attaching the event listener directly to the DOM instead of handling the event via React?

Doing this and passing ref instead of onKeyDown to <input> fixes the issue for me:

const ref = useRefEffect((element) => {
    element.addEventListener('keydown', onInputKeyDown);

    return () => {
        element.removeEventListener('keydown', onInputKeyDown);
    };
});

[t-hamano]

I am investigating, but have created code to temporarily enable the block so that anyone can reproduce this issue.
If you run this code in your browser console, a custom block will be added and you should be able to reproduce this issue.

Code

( function ( blocks, components, element, blockEditor ) {
	const el = element.createElement;
	blocks.registerBlockType( 'create-block/test-block', {
		title: '#38548 Test Block',
		apiVersion: 2,
		icon: 'smiley',
		category: 'text',
		edit: function () {
			return el(
				'div',
				blockEditor.useBlockProps({
					style: {
						backgroundColor: '#eee',
						padding: '16px',
						border: '1px solid #aaa',
					}
				}),
				el( components.ComboboxControl, {
					label: 'Combobox Control',
					options: [
						{
							value: 'small',
							label: 'Small',
						},
						{
							value: 'normal',
							label: 'Normal',
						},
						{
							value: 'large',
							label: 'Large',
						},
						{
							value: 'huge',
							label: 'Huge',
						},
					],
				} )
			);
		},
		save: function () {
			return null;
		},
	} );
} )(
	window.wp.blocks,
	window.wp.components,
	window.wp.element,
	window.wp.blockEditor
);

18ae6210fa1bf77e7857bef0028df11c.mp4

[t-hamano]

I have not been able to solve this problem, but I will describe what I have learned from my research so far in the hope that someone else will be able to solve this problem.

First, in the ComboboxControl component, the keyboard operations are tracked here:

gutenberg/packages/components/src/combobox-control/index.js

Lines 121 to 128 in eeeccf6

	const onKeyDown = ( event ) => {
	let preventDefault = false;
	if ( event.defaultPrevented ) {
	return;
	}
	switch ( event.code ) {

If there is a focusable element (block or title) above or below, starting from the block containing the ComboboxControl component, event.defaultPrevented will be true and no movement within the dropdown will occur.

What occurs instead is the vertical movement of the caret (i.e., the movement of the block in focus) in the useArrowNav hook:

gutenberg/packages/block-editor/src/components/writing-flow/use-arrow-nav.js

Lines 263 to 270 in eeeccf6

	if ( closestTabbable ) {
	placeCaretAtVerticalEdge(
	closestTabbable,
	isReverse,
	verticalRect
	);
	event.preventDefault();
	}

If we were to comment out the placeCaretAtVerticalEdge, the cursor movement within the dropdown would work correctly.

However, in the ComboboxControl component, if the cursor up/down operation is caught, event.preventDefault(); should be performed here:

gutenberg/packages/components/src/combobox-control/index.js

Lines 135 to 154 in eeeccf6

	case 'ArrowUp':
	handleArrowNavigation( -1 );
	preventDefault = true;
	break;
	case 'ArrowDown':
	handleArrowNavigation( 1 );
	preventDefault = true;
	break;
	case 'Escape':
	setIsExpanded( false );
	setSelectedSuggestion( null );
	preventDefault = true;
	break;
	default:
	break;
	}
	if ( preventDefault ) {
	event.preventDefault();
	}

And in the useArrowNav hook, the processing should be canceled at this point:

gutenberg/packages/block-editor/src/components/writing-flow/use-arrow-nav.js

Lines 221 to 224 in eeeccf6

	// Abort if navigation has already been handled (e.g. RichText
	// inline boundaries).
	if ( event.defaultPrevented ) {
	return;

However, for some reason event.defaultPrevented is always false, so the vertical movement of focus to the block takes precedence.

I think we need to add the appropriate treatment to either ComboboxControl or useArrowNav, but for the life of me, I could not pinpoint the cause.

I have given it a hoge label and hope someone familiar with this issue can solve it.

[stefanfisk]

@t-hamano did you look into this #38548 (comment)?

From what I can tell, event handlers that are registered this way take precedence over those registered via React, which makes the event bubbling super confusing.

[t-hamano]

So, if we register an event handler via React in ComboboxControl as well, it will solve this problem?
If so, I would like to try to fix it.
Of course, the PR from you would be greatly appreciated 👍

[stefanfisk]

If I remember correctly, and assuming that my findings were correct, the problem is not in <ComboboxControl>. Instead it is useArrowNav() (or some other similar hook) which registers its keydown handler via addEventListener(), and therefore handles the event before <ComboboxControl> has had a chance to call event.preventDefault().

So a quick and dirty fix is to make <ComboboxControl> also use addEventListener(). But IMHO that's just kicking the can down the road, what is really needed is to stop using addEventListener an instead let React handle all events.

This was all a few months back though, and right now I've got a two month old baby taking up all of my time, so I don't have the time to confirm any of this.

[stefanfisk]

But if you find the code that changes the selection and set a breakpoint there as well as in <ComboxboxControl>s onKeyDown, you should be able to verify if this is the root cause.

[t-hamano]

Thanks for the informative info!
Since addEventListener is used everywhere, I don't know if it is possible to completely replace them.
Therefore, I would like to create a PR using the approach you suggested and ask other members for their input.

[stefanfisk]

Yeah, that's such a fundamental architectural change that I just backed away slowly once I got that far 😕

[t-hamano]

I have confirmed that the approach described in this comment solves this problem on the block editor.

However, there is no longer a proper way to "escape" from the ComboboxControl on the block editor.
The up/down keys change the choices. The left and right keys move the cursor in the text box. In other words, the arrow keys alone will not allow us to escape from the ComboboxControl and focus on other blocks. The only way is to press the tab key to give focus once to the reset button and then press the up/down keys.

I believe this is inappropriate in terms of accessibility and would like to explore what keystrokes would be appropriate.

[mirka]

However, there is no longer a proper way to "escape" from the ComboboxControl on the block editor.
The up/down keys change the choices. The left and right keys move the cursor in the text box. In other words, the arrow keys alone will not allow us to escape from the ComboboxControl and focus on other blocks. The only way is to press the tab key to give focus once to the reset button and then press the up/down keys.

There are already cases where the special arrow key handling of the Writing Flow is just not compatible with a control's own standard arrow key handling. For example, a standard <input type="number"> or <input type="time"> where the up/down keys are supposed to change the value of the input. Edge navigation is already disabled in some of those known cases, requiring a Tab to navigate out.

I would argue that we should first honor the standard arrow controls of the underlying component (combobox in this case), rather than prioritize the Writing Flow edge navigation, as this is more of an enhancement on top of the standard controls.

In that vein, one possible way to address might be to add role="listbox" to the opt-outs in isNavigationCandidate().