Page MenuHomePhabricator

MenuTagMultiselectWidget: The first item should be automatically highlighted+selected when the menu opens
Closed, ResolvedPublic

Description

Since the menu also allows moving downwards with the mouse, and also allows the "enter" key to automatically add the selected item, we should have the widget automatically select the top/first item in the list when the menu is first opened, especially when filtering.

This will allow the user to either add the first item in the list with 'enter' key or to navigate through the available items with the arrow-down/up keys, starting from the top item.

We are already doing this in RCFilters, and it is requested again in T173741: When typing a namespace, the top option should be highlighted and be able to be added with enter

Event Timeline

Change 413287 had a related patch set uploaded (by Mooeypoo; owner: Mooeypoo):
[oojs/ui@master] MenuTagMultiselectWidget: Highlight first item when filtering

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

@matmarex this was a little harder than I thought, and exposed another potential bug in MenuSelectWidget with the config variable "highlightOnFilter". Supposedly, "highlightOnFilter" should do exactly this behavior, but it doesn't work right now. I tried to fix it in MenuSelectWidget, but the cross-events that trigger 'updateItemVisibility' (on key strokes, on changing of input, etc) interfere with this behavior, and make the selection jump up and be unsynchronized with the arrow movement.

The 'highlightOnFilter' behavior does work on mixin.LookupElement, so I tried to follow the same process, but LookupElement calls the highlighting functionality from outside MenuSelectWidget not inside it, which means that it bypasses all the potential cross-events that are triggered (or rather, it waits until they're all over). This is why in this case I also put the behavior in MenuTagMultiselectWidget rather than inside MenuSelectWidget itself.

While this works, there are a few questions to follow up or decide on with this patch:

  • Should this behavior be cancellable with a config variable? It seems at the very least it should be default to MenuTagMultiselectWidget; in RCFilters we override the entire item-visibility-related events completely, since we're using a model/controller to set it up, so RCFilter is really an edge case here, and would have overriden these methods regardless.
  • Whatever happens in this fix, 'highlightOnFilter' on MenuSelectWidget is broken right now. I couldn't manage to directly fix it without completely changing the way the events work, and almost rewriting the widget. My approach to a fix would have been separating, at the very least, the event response to editing the input with the events related to keystroke, but that change is much more nuanced, since some of those are not really distinct as we want them to be. I didn't want to get too much into this without consulting first, though. So -- do we cancel the 'highlightOnFilter' config as non working, or should we rewrite the functionality to try and decouple events and make it work?
  • Whatever happens in this fix, 'highlightOnFilter' on MenuSelectWidget is broken right now. I couldn't manage to directly fix it without completely changing the way the events work, and almost rewriting the widget. My approach to a fix would have been separating, at the very least, the event response to editing the input with the events related to keystroke, but that change is much more nuanced, since some of those are not really distinct as we want them to be. I didn't want to get too much into this without consulting first, though. So -- do we cancel the 'highlightOnFilter' config as non working, or should we rewrite the functionality to try and decouple events and make it work?

To be more specific, calling updateItemVisibility on keystroke "up" / "down" / "left" or "right" seems redundant and unnecessary. Item visibility only changes if the filtering changed, so it should really be only called on actual change of the input -- either 'change' event and/or detecting copy/paste. But currently, the updateItemVisibility seems to conflate those two actions (keyboard action that changes input and hence changes the visible items, and keyboard action that doesn't) and so attaching any sort of highlighting action is also conflated; it means that the code tries to "highlight first item" both when the items changed ( = good) and when the user hits the arrow buttons ( = bad)

This conflation of events, in my opinion, should be untangled regardless of having this feature. It will also allow any widget that extends MenuSelectWidget to choose whether they extend the editing of the input functionality, or the arrowkey / escape/etc functionality separately. I almost insisted on doing that because RCFilters needed that difference, but then RCFilters started using its entire filtering (and display and highlight) logic through the data model and controller, so it made the internal widget operation change moot -- but I think it's still very confusing and should be untangled regardless.

Volker_E triaged this task as Medium priority.Mar 1 2018, 7:17 PM
Volker_E removed a project: Patch-For-Review.

Change 413287 merged by jenkins-bot:
[oojs/ui@master] MenuTagMultiselectWidget: Highlight first item when filtering

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