Page MenuHomePhabricator

Lookup doesn't close, wipes out text input when selected item is removed on selection
Open, LowPublic

Description

If you create a Lookup component for letting the user find an item to add to a list, and you want to prevent the user from adding the same item twice, you might implement that by doing something like this:

  • Maintain an array with the items the user has already chosen
  • Bind :menu-items to a computed property that takes the lookup results and filters out already-chosen items
  • When an item is selected, add it to the array of chosen items

However, if you do this, the Lookup fails to close when you select an item. If you had typed any text into the input, that text is wiped away as well. Both of these bugs are caused by the fact that the selected item has been removed:

  • The input text is wiped because the watcher on the modelValue sees a new value that is not null (it's the ID of the selected item), but it can't find the corresponding item (since it's been removed), so it sets inputValue to the empty string
  • The menu does close initially on selection (done by the Menu component), but then the watcher on the menuItems prop fires when the selected item is removed. It concludes that the input value is not equal to the label of the selected item (because it can't find that item anymore), and that there are more than 0 menu items, so it (re)opens the menu.

The first of these appears to happen in the "wrong" order, because modelValue is updated before the selected item is removed, but that's because Vue calls watchers and computed property functions asynchronously (on nextTick). So by the time the modelValue watcher runs, the menuItems prop has also been updated.

See this code for a simple example, and try it out here. This is based on a real-life example in WikiLambda, discovered by @JKieserman.

I'm not sure how we should address this. To some extent, this is a reasonable use case, and one of the bugs is triggered by the somewhat tricky logic in the menuItems watcher. On the other hand, removing the selected item right when it's selected is bound to cause problems, because it blows up any logic that might want to look at the properties of the item we just selected.

Some ideas for solving one or both of these issues:

  • If the menuItems watcher only tried to open/close the menu if the number of items changed from zero to nonzero / from nonzero to zero, that would fix the "menu fails to close" issue. However, we might need this behavior or something like it for cases where the user types something, chooses an item, and then types something else.
  • If the menuItems watcher had some other way of knowing that we just made a selection, it could use that instead of checking whether the input matches the selected item. Perhaps on selection we could do something like justSelected = true; nextTick( () => justSelected = false );
  • If we had an update:selected handler on the Menu that, before emitting Lookup's own update:modelValue first updated inputValue, updating the input correctly for the selected item would work even if the selected item then changes or disappears.
  • If the modelValue watcher can't find the selected item, it could leave inputValue alone instead of blanking it

Event Timeline

Change 773367 had a related patch set uploaded (by Catrope; author: Catrope):

[design/codex@main] [WIP] Demonstrate bug in Lookup when selected item is removed

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

Interesting. I wonder if we could resolve this by directly supporting this use case in Lookup/Menu, via a prop that indicates that this is the desired behavior. The components could keep track of which items have been selected and filter them out, but would still know about them, so we could avoid some of these bugs.

This also feels close to multiselect behavior, which we're going to need to support in Menu at some point anyway. So I wonder if it's better for us to take a step back and think about library architecture vs. applying some fixes for these specific bugs, which might be hard to balance with other use cases supported by Lookup.

Here's a video demonstrating this bug in the WikiLambda context. Another interesting aspect of the bug that I didn't realize yesterday is that the menu also reopens when you delete a language from the table (because that adds that language back to the menu items) even though the lookup isn't focused. Similarly, there's a bug with any delayed lookup (one that takes time to fetch its results) where, if you type something, then move the focus away from the lookup before the results are fetched, the menu will still open when the results arrive.

In light of my most recent comment, do you think it would make sense to never open the menu if the input isn't focused?

The only case I can think of where we might want to keep the menu open is a true multiselect component, where the user may be selecting multiple things at once before wanting the menu to close. Otherwise, yes, I don't think the menu ever needs to be open if focus is not on the input.

STH changed the task status from Open to In Progress.Apr 15 2022, 7:51 PM
STH assigned this task to Catrope.
STH triaged this task as High priority.
Catrope changed the task status from In Progress to Open.Apr 15 2022, 9:50 PM
Catrope removed Catrope as the assignee of this task.

Change 785233 had a related patch set uploaded (by Catrope; author: Catrope):

[design/codex@main] Lookup: Use pending and focus states to decide whether to open the menu

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

You can test this using the demo here.

I also tested this with the WikiLambda patch that this was originally discovered in, and it fixes the bug there:

Change 785233 merged by jenkins-bot:

[design/codex@main] Lookup: Use pending and focus states to decide whether to open the menu

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

Change 773367 abandoned by Catrope:

[design/codex@main] [WIP] Demonstrate bug in Lookup when selected item is removed

Reason:

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

Does this need to be reviewed by a Designer on DST?

No, this should be moved to functional testing. QTE + Engineering can pair on final testing, since we do not have any demos set up that cover this use case.

ldelench_wmf lowered the priority of this task from High to Low.Tue, Jul 5, 4:13 PM

Hey @JKieserman, this issue that you found with the Lookup component was fixed a few releases ago. Could you please confirm whether the Lookup component is now working for your use case, where items are removed once selected?