[BUG] Prevent rendering issue with the SelectComponent when it has focus

[cyril-sf]

This fix aims to remove the following deprecation:

You modified concat(....) twice in a single render. This was unreliable in Ember 1.x and will be removed in Ember 3.0

The SelectComponent sets a specific class when it is focused (

ember-widgets/app/templates/select.hbs

Line 3 in ebcc0e2

{{if hasFocus 'ember-select-focus'}}">

).

It's failing in our application, but I haven't been successful at writing a test to cover the case in the library. Based on the deprecated state of the library and the fact that the problem was caught somewhere else, I decided to not invest more time on it.

[mixonic]

Anything in afterRender can't be causing the double-render error. By definition that error requires something is synchronously changes during the rendering phase (which is in the render queue)

The other two places here that actions queue is used look like they could be a more relevant fix though, under the presumption a focus event is triggered synchronously during rendering. They seem more germane?

[bantic]

@cyril-sf Can you point out (privately if need be) where are the issues in our application that this fixes?

[bantic]

I spent some time trying to better understand when the focusin/focusout events are fired (in general, not in Ember specifically). It looks like it is possible for these to interleave if the DOM changes in a way that modifies focus, for instance by removing the focused element from the DOM, or calling element.focus() — new focusin/out events will synchronously happen. I was able to recreate the double-render issue using a select component that renders a button, where the button's action hides the dropdown. Clicking the button triggers multiple synchronous calls to focusOut and focusIn: the button gets focus itself, which triggers focusOut and then focusIn on the select-component, and then the dropdown is hidden (due to the button's action), which removes the button from DOM, so it loses focus which then triggers another synchronous focusOut as it is removed.
Scheduling these into actions seems like a good call.

I'm not clear on what moving the popover's didInsertElement work from afterRender to actions fixes — were you able to figure out what the underlying issue there is?

[bantic]

Seems good. I wish we had a better understanding of why the popover's didInsertElement work needs to have its queue changed to actions, but not a blocking concern.

Perhaps in the future we can remove the hasFocus property altogether in favor of CSS that uses the :focus/:focus-within selector.

[cyril-sf]

@bantic Agreed. What seems to not make possible to rely on CSS at the moment is the way we decide if the SelectComponent has the focus

https://github.com/Addepar/ember-widgets/blob/master/addon/components/select-component.js#L543-L552

@mixonic reading the setFocus method, it makes it look like the fix we work on checking if an input as the target is correct (thinking about a button in the footer as we saw).

[mixonic]

Awesome, thanks for taking the time to work on this with me @cyril-sf. I think the result is only a few lines different but massively better for comprehension.

[mixonic]

I like this approach!

[mixonic]

When does setFocus not have an argument?