Vuetify: Combobox Is Not Closing When It Has Focus And :items Is Changed

[Flamenco]

Versions and Environment

Vuetify: 1.1.5
Vue: 2.5.17
Browsers: chrome
OS: osx

Steps to reproduce

select item 2
press button (with cursor still in combobox)

Expected Behavior

menu should not open, and then should close

Actual Behavior

menu opens when changing items and will not close

Reproduction Link

https://codepen.io/Flamenco/pen/pZEqGo?editors=1111

[sh7dm]

Is it platform specific issue? It works normally on Firefox.

[johnleider]

This is being caused by this line:
https://github.com/vuetifyjs/vuetify/blob/dev/src/components/VAutocomplete/VAutocomplete.js#L179

This happens because the updateItems event is called and triggers the menu. Because you are clicking on the button, the intended action is to blur the input and then update the items. Because this happens simultaneously, the input thinks that it is still focused and tries to open the menu.

https://codepen.io/johnjleider/pen/KBaqdN?editors=1111

[Flamenco]

@johnleider I think your solution is a workaround and the issue should be reopened and fixed in the product. It is neglegent to assume an end user will simply know that a delay is needed.in this case.

The more I use Vuetify, the more I come across these race conditions, and hours are spent dealing with them; It’s really starting to eat up my development budget.

[ekoeditaa]

Hi @johnleider, I would like to take this as my first issue. Can you help to tell me where to start? Thank you in advance :)

[Flamenco]

A few ideas. Pick one.

• find a way to make the select lose focus, as it should
• do not open the dropdown if it was not already open
• always update the menu in $nextTick, possibly checking some state flag to optimize

[ekoeditaa]

I understand the idea but I'm not entirely sure about the implementation. Since the problem is caused by the methods inherited from VAutocomplete component but it shouldn't be changed because it may be used in others component. In the VCombobox component, I don't know what should I change there. Thanks for the help.

[Flamenco]

There are 2 issues here.

• The first one is that the focus should not be in the combobox when the button is processing. The button should have the focus at that time.
• The second is that the combo should not show the popup if it is not already opened when the content is changed.

Let's start with the first, as it will solve many other issues as well.
@johnleider Do you see major problems with running ALL button handlers in the nextTick? If this solves the problem, then maybe we add a next-tick directive to buttons, default it to false, and if there are no performance or other issues, eventually make it default behavior and remove the directive.

[ekoeditaa]

The first issue is caused by the watched items in the VAutocomplete component, specifically, the line of code @johnleider talked about. Will there be problems if we change this part? Is there any other way to fix this?

[johnkingsley]

Here is a simple test case which someone may find useful: https://codepen.io/anon/pen/djJrpx

[johnkingsley]

This change works for me:

--- a/src/components/VAutocomplete/VAutocomplete.js
+++ b/src/components/VAutocomplete/VAutocomplete.js
@@ -179,6 +179,7 @@ export default {
       // User is probably async loading
       // items, try to activate the menu
       if (
+        this.hideNoData &&
         this.isFocused &&
         !this.isMenuActive &&
         val.length

I think the comment in the code "User is probably async loading items" makes it clear there is currently no way in the API to know _definitely_ if the user is using async loading. By adding a check for hideNoData I think it makes a better guess.

[Kasheftin]

I don't think that fixing focus (removing nextTick from button handlers etc) will fix this issue.
I personally met it in the following scenario:

• autocomplete results loaded through ajax accordingly to the input-search value;
• the initial options should be shown even if input-search is empty;
• return-object option isset;
  This case selecting a value from a list will update input-search to empty, the last will trigger an initial options list to show, items will be replaced and the new list will be mistakenly shown. The autocomplete is still focused, the value itself triggers updateItems event.

The other scenario also causes this issue:

• assume we have to paste some external current value to the autocomplete;
• autocomplete items loaded through ajax, and the current external value is not necessary present in the items list. The initial items array is usually empty;
• But we have to show this value somehow, that's why we inject it into the items list if it's not there;
• Also assume that the other search results go from the store, we have to prepare them somehow without mutations;
• Then we use the code like this:

computed: {
  items () {
    const items = [...this.searchResults]
    applySomePrepareMutation(items)
    if (this.value && items.findIndex(rw => rw.id === this.value.id) !== -1) {
      items.unshift(this.value)
    }
    return items
  }
}

This case selecting a value will trigger computed items to update. The last will not actually change, but from js point of view the new items will be a different array, and this will trigger updateItems event as well.

[johnleider]

The scenario you are describing sounds like autocomplete, not combobox @Kasheftin .

[dkichler]

I believe this all comes down to the assumption that the update is from an async call, and that the menu should always be invoked in that case - as pointed out by @johnleider. I proposed in #4878 a simple prop to control this behaviour, a solution similar to that suggested by @johnkingsley.

Given the number of reports of this behaviour as a defect, perhaps it should even default to off, and the prop would need to be used to toggle it on, in cases where it is preferred.

[johnleider]

I completely agree and made that comment on your PR. Let's get it in next release!

[KaelWD]

The original commit d4732ec0b8b5d80578c4c6d8ce0cc4bf3447a307 and unit test referred to hide-no-data, but the actual implementation doesn't use it. I think @johnkingsley's suggestion is probably the simplest way to solve this, maybe with the addition of !(oldVal && oldVal.length).

[KaelWD]

Actually that watcher can probably be removed entirely, there's also a computed property that handles this much more elegantly: https://github.com/vuetifyjs/vuetify/blob/0a2e9cabe52ade10e4e372425ad92e1aff6f2d55/src/components/VAutocomplete/VAutocomplete.js#L116-L120

[dkichler]

The hide-no-data prop was introduced as a way of controlling visibility of the menu when no results are available (as a result of filtering/querying by search) https://github.com/vuetifyjs/vuetify/commit/20228a5ece31745e2553de1898b9aef3585de753. Using it to also control whether the menu should open upon update I think is confusing because it doesn't allow independent control over distinct behaviours. There may be an argument for combining the two behaviours into a single 'feature', controlled by one prop, but I think that path involves making more assumptions about how such a feature should work/will be used.

If the distinct feature of opening the menu upon external items update is to be maintained, I think the watcher must also stay, and that it's behaviour should be controlled by a new prop as proposed in my PR.

Both the hide-no-data and open-on-items-changed props cater to those doing server side querying/filtering, which I think warrant fine control over configuration. The hide-no-data prop alone does not cover the intent behind this original ticket https://github.com/vuetifyjs/vuetify/issues/2613, but using hide-no-data, open-on-items-changed and no-data-text together, a dev can craft whatever custom server-side managed behaviour they wish.

I understand the desire to avoid new props as generally more props equals more complexity, but I believe the tradeoff is warranted in this case since these props are really intended for cases where a little complexity is to be expected (ie, server-side querying/filtering), and flexibility appreciated.

[KaelWD]

The items watcher is there because hide-no-data would prevent the menu from opening, instead of just hiding it when there was nothing to display. I don't see what use open-on-items-changed really has apart from that.

[dkichler]

As far as I can tell, hide-no-data's only function is to control visibility of the menu when no results exist, which seems like a relevant feature outside the context of an async call. I say this with only the code to go on.

The items watcher is there because hide-no-data would prevent the menu from opening, instead of just hiding it when there was nothing to display.

I was under the impression that the items watcher is there to open the menu in cases where an async call has completed, the menu is closed, and the app wishes to draw attention to the results. Demonstrated by this codepen, type 'm' into autocomplete, then press esc to close the menu. When the simulated call finishes, the menu is opened. Feels like legitimate feature whether or not the user also wants to hide the menu when no results exist (via hide-no-data). This does not apply for non-async cases though, and leads to unwanted menu invocations. Hence the proposal for the open-on-items-change prop to enable this specifically for the async use-case rather than having it be default.

If you're advocating for coupling the above behaviour to hide-no-data as well, I see some risk in that approach of more reports of unexpected behaviour. However, I am willing to draft that solution if that's what it takes to change the existing default (the main motivator behind this ticket).

[KaelWD]

You might be right, this is why john said we should get some use cases together for #4879.

Demonstrated by this codepen

'Off' doesn't seem like it should be used there, typically it'd be a loading bar while waiting for the response instead of jumping straight to saying there's nothing there.
Honestly there's so many ways this could be used that maybe it is better to be explicit. I'm not sure if open-on-items-changed is the right way to go with that though, maybe it should be #5036 plus the ability to control the menu manually so you can decide for yourself if it should be open or not. (#2443)

[dkichler]

'Off' doesn't seem like it should be used there

Indeed, the on/off selector was only there to demonstrate that the opening of the menu is independent (and I think should remain so) of hide-no-data.

maybe it should be #5036 plus the ability to control the menu manually so you can decide for yourself

This codepen demonstrates the async use-case that would rely on the current default behaviour of opening upon items changed, as well as making use of hide-no-data and no-data-text to customize user feedback. It also demonstrates a case that would become either untenable or overly confusing if hide-no-data were also responsible for controlling the behaviour of opening the menu. Finally, it also demonstrates how explicit opening of the menu is already possible via the component API.

Some solution options:

1. Drop the items watcher all together, put the burden on the developer if they wish to build a custom async solution that opens the menu when a call has finished (this would give the greatest flexibility since the client would determine the conditions entirely, rather than relying on the conditions assumed by the component)
2. Keep items watcher, but also gate it with the hide-no-data prop, as proposed in #5036
3. Introduce open-on-items-changed to gate the items watcher and allow developers to turn 'on' the assumptions made by the component about when the menu should item, as proposed in #4879

My order of preference would be:

3 - A good balance: provides optional assumed behaviour (open the menu when items change, is not already open, has focus, and there are some items in the new list). Assumed behaviour must be enabled explicitly, component API is still available for more complex cases.
1 - The most flexible, but puts a bit more onus on the developer. Existing behaviour is lost and must be reimplemented as custom code via the component API
2 - Doubles down on assumptions and additionally couples hide-no-data to the behaviour of opening the menu. Menu opening behaviour is no longer enabled by default, but when it is enabled via hide-no-data, the developer must live with the assumptions in place.