Hm, probably an unintended consequence of T317682: Make new Vector search navigate to search result URL when selecting search result using keyboard – in TypeaheadSearch’s onKeydown(), we need to know if the menu.value.getHighlightedMenuItem() was highlighted by keyboard input or via the mouse?

Edit: No, I don’t think that last part is right, I don’t understand the interaction between onKeydown() and onSubmit() at the moment. (But it probably is related to that task.)

(Tagging Codex, the problem can also be reproduced on the demo page and I assume it should be fixed generally.)

menu.value.getHighlightedMenuItem()

This also seems like we've confused highlighting (focus/pre-selection) with the actual selection..

I believe this happens because we wanted to support keyboard navigation in menus. It makes sense that pressing the up/down arrow keys to navigate to a menu item and then pressing enter should select that menu item. And this already works TypeaheadSearch (the Vector search), because navigating to a different item with the keyboard also changes the contents of the input. So for TypeaheadSearch, I think we should change this behavior and make the enter key submit what's in the input, not what's highlighted.

For other menus, it's less clear. In menus that are not attached to a TextInput (e.g. the Select component), it's probably fine to keep the current behavior where pressing enter selects the highlighted option. In menus that are attached to a TextInput (Combobox and Lookup), it makes sense for the enter key to select the highlighted item if you got there by pressing the arrow keys, but not if you just hovered it with the mouse. TypeaheadSearch makes that distinction by changing what's in the input when the arrow keys are pressed. We could do that in Combobox/Lookup, but I don't think it would work that well there.

Instead, we could track (either in those components or in Menu itself) whether the currently highlighted item was arrived at using the keyboard, and only selecting on enter if that is the case. I'm not sure what pressing enter would otherwise do in those components though; in TypeaheadSearch there is a form to submit, but for Combobox/Lookup that depends on the surrounding context. Maybe we just don't prevent the event, let it bubble up, and see if the parent component wants to do anything with it? I could see Combobox/Lookup being used in form contexts in the future where enter might trigger a submission action, maybe we should just keep the door open to that.

The problem isn't that pressing Enter selects the highlighted option. It's that the creation/showing of the menu alone highlights the item that happens to be under the cursor even before the mouse has moved.

Change 888795 had a related patch set uploaded (by Anne Tomasevich; author: Anne Tomasevich):

[design/codex@main] Menu, TypeaheadSearch: Don't select item highlighted via mouse

https://gerrit.wikimedia.org/r/888795

This patch changes the behavior of the Menu component to only select a highlighted item if it was highlighted via the keyboard. Here's the demo.

A follow-up patch also removes the MenuItem's highlighted styles for the :hover state, and only applies them when the item has the cdx-menu-item--highlighted class. This fixes the issue of double-highlighting when you highlight an item with the mouse then use the arrow keys to highlight another item. See a demo here.

@Sarai-WMDE, would you mind taking a look at this, especially the second demo which incorporates both changes, to confirm if this is the correct UX? cc @bmartinezcalvo and @Volker_E as well

In T327499#8611593, @AnneT wrote:

In T327499#8601057, @Nardog wrote:

The problem isn't that pressing Enter selects the highlighted option. It's that the creation/showing of the menu alone highlights the item that happens to be under the cursor even before the mouse has moved.

As far as I can tell, this is pretty typical behavior for stylized dropdown menu components (i.e. dropdowns that do not use the native <select> element). It would likely be difficult or maybe impossible to change this behavior in a way that would work consistently, especially across browsers where mouse events can differ. That said, it does seem very clear that we need to avoid selecting the highlighted item on an enter keypress unless the user navigated to that item via the keyboard.

It seems to work consistently in the old search suggestions in legacy Vector. This seems to be achieved by a combination of using mousemove events instead of mouseenter, and explicitly tracking whether a selection was done by mouse or by keyboard arrow keys. See uses of selectedWithMouse in the code: https://gerrit.wikimedia.org/g/mediawiki/core/+/855004747a995408c5499b39b62b7535232f1ddc/resources/src/jquery/jquery.suggestions.js. I don't know if that works for Codex, but maybe it will at least serve as inspiration.

@matmarex thanks for the suggestions! I had forgotten about the mousemove event.

mousemove does indeed provide the ideal UX here, in that it avoids highlighting an item that happens to be under the cursor when the menu opens, but starts highlighting items as soon as the mouse moves. My only concern is that it gets triggered with each mouse move—I wonder if there's a performance concern with the mousemove handler being called over and over if the mouse is moved a lot? cc @Catrope

Change 888795 merged by jenkins-bot:

[design/codex@main] Menu, TypeaheadSearch: Don't select item highlighted via mouse

https://gerrit.wikimedia.org/r/888795

Change 889230 had a related patch set uploaded (by Anne Tomasevich; author: Anne Tomasevich):

[mediawiki/core@master] Update Codex from v0.5.0 to v0.6.0

https://gerrit.wikimedia.org/r/889230

Test wiki created on Patch demo by ATomasevich (WMF) using patch(es) linked to this task:
https://patchdemo.wmflabs.org/wikis/66d018802f/w

The initial patch, which fixes the main part of this bug, is part of today's Codex release, which will ride the train next week. This will change the behavior of the search component to only select a highlighted menu item on Enter keypress when that item was highlighted via keyboard navigation.

We'll keep working on further improvements, like avoiding highlight on menu open and getting rid of the duplicate highlighting situation (i.e. you can currently highlight two items at once, one with your mouse and one with the keyboard). Hopefully, these changes will go out with the next Codex release in 2 weeks.

Change 889230 merged by jenkins-bot:

[mediawiki/core@master] Update Codex from v0.5.0 to v0.6.0

https://gerrit.wikimedia.org/r/889230

In T327499#8614601, @AnneT wrote:

@matmarex thanks for the suggestions! I had forgotten about the mousemove event.

mousemove does indeed provide the ideal UX here, in that it avoids highlighting an item that happens to be under the cursor when the menu opens, but starts highlighting items as soon as the mouse moves. My only concern is that it gets triggered with each mouse move—I wonder if there's a performance concern with the mousemove handler being called over and over if the mouse is moved a lot? cc @Catrope

Sorry for missing this comment last week! Performance-wise using mousemove is less good than using mouseenter (because it fires so often), but we can mitigate that by returning early if the item is already highlighted. Right now we don't do this, the MenuItem emits a change event to its parent menu even if the state didn't actually change, and that could be an issue if we do it every time mousemove fires.

Thanks @Catrope, that all makes sense to me!

I've opened T330665 to cover further improvements to menu item highlighting behavior.

Test wiki on Patch demo by ATomasevich (WMF) using patch(es) linked to this task was deleted:

https://patchdemo.wmflabs.org/wikis/66d018802f/w/