EuiOutsideClickDetector fires early on overrides value in firefox

[flash1293]

In firefox if EuiOutsideClickDetector is used in the same React component with a select element and the state is updated by the onOutsideClick handler, the value of the select element is reset before the onChange handler of the select is fired which makes it impossible to capture a select change which was triggered by mouse click:

<div>
        <EuiOutsideClickDetector
          onOutsideClick={() => {
            this.setState({ otherKey: this.state.otherKey + 1 });
          }}
        >
          <h1>Counter {this.state.otherKey}</h1>
        </EuiOutsideClickDetector>
        <select
          value={this.state.val}
          onChange={e => {
            // when this is called, e.target.value is already reset to
            // this.state.val, because the onOutsideClick handler above
            // also re-renders the select
            this.setState({ val: e.target.value });
          }}
        >
          <option value="1">Option 1</option>
          <option value="2">Option 2</option>
          <option value="3">Option 3</option>
          <option value="4">Option 4</option>
        </select>
      </div>

https://codesandbox.io/s/m7916933m8

This happens because in firefox a mouseup on an option in a select menu bubbles into the EuiOutsideClickDetector which isn't the case in chrome.

I'm not sure whether this even counts as React bug. It is possible to work around this by delaying the state update from onOutsideClick, but there has to be a better solution to that.

[flash1293]

This problem caused elastic/kibana#35199 - in this case it was possible to work around the issue.

[thompsongl]

Looks like in Chrome it's the mousedown event on the select box that bubbles to EuiOutsideClickDetector and in Firefox it's the mouseup event on the select option item.

This means that the exact same interaction with a select in this scenario will result in a different count number depending on the browser 😕 (e.g., single selection results in 1 in both; open and close without selection results in 1 in Chrome and 0 in Firefox)

[flash1293]

It is possible to prevent this specific event from bubbling up (could be implemented in EuiSelect) to have Firefox behaving like chrome regarding option events - it doesn't fix the underlying problem, but at least we won't run into this specific bug.

onMouseUp={e => {
  if (e.target.nodeName === "OPTION") {
    e.nativeEvent.stopImmediatePropagation();
  }
}}

https://codesandbox.io/s/4q8x16w6r0

[thompsongl]

The issue is actually a bit different than my initial observation. FireFox never fires a mouseup for select box clicks, only a mousedown; the select option fires both mousedown and mouseup.
Chrome immediately fires a mouseup event after the select box opens, even if you still have the mouse pressed. Chrome fires no mouse events for interaction with the select options.

I think you're right that trying to normalize via EuiSelect is the path to go down.

[flash1293]

It's not nice, but probably the best we can do here.

The underlying problem is that the reaction of EuiOutsideClickDetector to a mouse event can "disturb" a change event because it is handled first - if it's possible we should make sure the outside click handler always fires last in the set of all handlers attached - maybe there is some way to enforce this.

[thompsongl]

Also a good idea. Thanks for putting together the codesandbox repros 👍

[thompsongl]

More news: Safari never fires a mouseup event for select elements, box or options.

[thompsongl]

And to complete the cycle: IE11 fires a mousedown and mouseup event for clicks on both the select box and the select option.

[thompsongl]

@chandlerprall I don't see a good way to normalize select mouse eventing via EuiSelect, but this issue does raise a question about the intended timing of the EuiOutsideClickDetector handler.

The originating problem of state update cancellation can be solved by the suggested setTimeout on the EuiOutsideClickDetector callback, but defaulting to that behavior would certainly cause breaking changes where consumers have relied on said callback to prevent updates.

I could see setTimeout being an optional flag, but let's discuss.

[chandlerprall]

I'm hesitant to make any wide spread and/or sweeping changes here, as everything is functioning as expected-ish (to be honest I'm surprised this doesn't happen in other browsers), and is an unfortunate intersection of React development being component-focused, DOM events retaining their ordering, and browser differences.

The provided code sandbox can rectify itself by including a setTimeout in the onOutsideClick callback or by re-structuring the state and moving the EuiOutsideClickDetector usage into a sub-component - the latter is much more difficult or impossible to do if both state values are global e.g. Redux or MobX. However, because this issue manifests through browser differences it does seem ideal to find a solution EUI can internally implement.

This also appears to be very isolated to native select boxes, where React polyfills the value attribute into marking the appropriate option as selected. This issue does not present itself in e.g. checkboxes even though the same onMouseUp -> onClickOutside -> render -> onChange -> render flow happens, as the checkbox's checked virtual DOM attribute doesn't change and the DOM element is left alone. If the stopImmediatePropagation solution works cross-browser for all of our use cases I would be very inclined to stick to that.

[thompsongl]

All good points. Let me look into browser outcomes for stopImmediatePropagation on EuiSelect mouseup

[flash1293]

I’m also not 100% sure whether React guarantees that onChange will be called synchronously or whether this is just an implementation detail with all that fiber stuff going on - stopping propagation in the first place seems more stable