search

Addepar / ember-widgets

https://opensource.addepar.com/ember-widgets/#/ember-widgets/overview
Other
288 stars 75 forks source link

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

Open cyril-sf opened 4 years ago

cyril-sf commented 4 years ago

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 ( https://github.com/Addepar/ember-widgets/blob/ebcc0e26b3a1bf92b82dfdb2c51e6432237e239b/app/templates/select.hbs#L3 ).

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 commented 4 years ago

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 commented 4 years ago

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

bantic commented 4 years ago

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?

cyril-sf commented 4 years ago

@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).