Page MenuHomePhabricator

MenuSelectWidget: updateItemVisibility keeps options invisible
Closed, ResolvedPublic

Description

I3f6c916de28f0bf70d9e8a9b7d6f5735131954eb for T190156 optimized the way the first item in the match list is highlighted.
In doing so it also introduced an object state filterQuery that caches the previous run's input value. Highlighting for items is now only executed if the input value changed compared to the last run.

This causes problems when updateItemVisibility is repeatedly run (e.g. because of filterFromInput) or after asynchronous request (e.g. RequestManager mixin) - in subsequent runs this.items is not iterated over and anyVisible never reaches the true state leaving the widget and options in invisible state despite items that should be shown.

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJun 29 2018, 3:10 PM

Change 443101 had a related patch set uploaded (by Pablo Grass (WMDE); owner: Pablo Grass (WMDE)):
[mediawiki/extensions/WikibaseLexeme@master] GrammaticalFeatureListWidget: Show exact results

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

Change 443115 had a related patch set uploaded (by Pablo Grass (WMDE); owner: Pablo Grass (WMDE)):
[mediawiki/extensions/WikibaseLexeme@master] selenium: adding grammatical feature in node

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

Change 443101 merged by jenkins-bot:
[mediawiki/extensions/WikibaseLexeme@master] GrammaticalFeatureListWidget: Show exact results

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

I don't really want to investigate this code. Unless @Mooeypoo (or somebody else) wants to work on a better solution, I would propose that we just revert https://gerrit.wikimedia.org/r/c/oojs/ui/+/439323 (I3f6c916de28f0bf70d9e8a9b7d6f5735131954eb). As I noted while reviewing it, it was mostly a cleanup change:

As far as I can tell, the only part of this patch that changes anything is adding the check for this.filterQuery !== this.$input.val(), which fixes navigation of the menu with arrow keys.

The rest is nice code cleanup (I agree it is saner to put the highlightItem() call at the end), but the old code already worked correctly. The issue in T190156 is caused by AdvancedSearch not passing highlightOnFilter: true at all.

We can fix navigation of the menu with arrow keys in a different way, e.g. by explicitly ignoring the arrow keys here (rather than ignoring any keypress which does not change the input value).

matmarex moved this task from Backlog to Doing on the OOUI board.Jul 2 2018, 11:09 PM

Change 443115 merged by jenkins-bot:
[mediawiki/extensions/WikibaseLexeme@master] selenium: adding grammatical feature in node

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

Change 444738 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[oojs/ui@master] MenuSelectWidget: Remove checks for unchanged input from updateItemVisibility()

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

Change 444738 merged by jenkins-bot:
[oojs/ui@master] MenuSelectWidget: Remove checks for unchanged input from updateItemVisibility()

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

matmarex closed this task as Resolved.Nov 1 2018, 2:00 AM
matmarex claimed this task.

(Whoops, we forgot to close it.)