[Menu] Fix nested menus that can't be clicked issue

[newoga]

• PR has tests / docs demo, and is linted.
• Commit and PR titles begin with [ComponentName], and are in imperative form: "[Component] Fix leaky abstraction".
• Description explains the issue / use-case resolved, and auto-closes the related issue(s) (http://tr.im/vFqem).

This fixes a regression that was introduced in #3360. The previous click-awayable mixin listened on mouseup and touchend events and the new ClickAwayListener was implemented to use mousedown and touchstart events. This commit changes the events to mimick the behavior from the original mixin.

Resolves #3818

[newoga]

Note: I haven't thoroughly tested the impact of this change. It does fix the nested menu issue, but I haven't checked to see if it impacts any of the other components that use ClickAwayListener. Either way, the new behavior in this PR more closely resembles the mixin that it replaced.

If someone else could give this a test, that'd be much appreciated!

[newoga]

Also, I know this wasn't slated for the beta, but this might not be a bad one to get in before releasing.

[alitaheri]

@newoga I agree 👍

[oliviertassinari]

Something is weird. I'm having this warning when selecting a nested menu item (level > 2):

warning.js:45 Warning: setState(...): Can only update a mounted or mounting component. This usually means you called setState() on an unmounted component. This is a no-op. Please check the code for the undefined component.

[nathanmarks]

@newoga do you need a hand looking into this?

[newoga]

@nathanmarks I can continue this later tonight, but feel free to take a stab if it's holding things up (just shoot me a ping and let me know). I've already identified the issue is here with this setState call in Popover.js#148

Edit: More specifically, the setState is getting call after an unmount, probably due to the timeout.

[nathanmarks]

@newoga sweet mother of god, I'll ping you on gitter

[nathanmarks]

BTW, it will not open N levels in this state. I added another level and it has the vanishing bug 😟

No console error either when this bug is triggered.

[nathanmarks]

EDIT:

I just realised that by removing the prop in the PR below, you cannot highlight items on previous menu levels

---

Seems like this is playing a part here, removing it allows me to open all levels with no problem more problems: #2707

However, I still get this issue 😟

warning.js:45 Warning: setState(...): Can only update a mounted or mounting component. This usually means you called setState() on an unmounted component. This is a no-op. Please check the code for the undefined component.

[nathanmarks]

@oliviertassinari

I went back to the PR before the mixin was removed.

Even though the 2nd level works there.... the 3rd doesn't -- @#@!#QDSS*(D&

[nathanmarks]

@newoga @oliviertassinari

Well, N levels doesn't work in 0.14.4 either. 😄

[nathanmarks]

Not to mention that our nested menu doesn't have the right UX anyways. It should support triggering the next level by hover state to mimic application menus (like google docs, which is what the spec examples kind of copy)

[mbrookes]

4 levels of nested menu is bad UX to start with, so if the regression can be fixed at previously supported nesting, that's good enough for now IMHO. 👍

[nathanmarks]

@mbrookes it can, but not without that warning so far 😄

[nathanmarks]

@newoga what's the plan here -- try and fix that error with 2 nested? Feels like punching a brick wall. lol.

[newoga]

Feels like punching a brick wall. lol.

@nathanmarks I'm stumped and my fists are bloody.

[mbrookes]

👊 🔴

[nathanmarks]

We have some deep rooted issues with Popover IMO. There are a number of code smells in the component and it already exhibits undesirable behaviour when it is working as intended.

Case in point: when you scroll a page with a Popover open, the popover moving with scroll is capped at 10fps resulting in absolutely abysmal UX.

Have you guys considered that this bug may just not be fixable with the current component design? That it is inherently flawed?

[mbrookes]

@nathanmarks - I agree on all fronts, however this is a regression, not a bug - i.e. it worked before.

Given that a mixin is just a means of sharing code between components (and merging lifecycle hooks etc), and that whatever code click-awayable added to Menu worked previously (however badly), this should be fixable with the current state of Menu / Popover (granted I haven't tried).

But if someone wants to take on improving Popover (especially that nasty scroll jank), I'm not going to complain! 😄

[nathanmarks]

@mbrookes I went back to 0.14.4 and I cannot use N levels of menu. Which theoretically is how this component is supposed to work.

[mbrookes]

@nathanmarks - no, but that isn't what we're talking about, as you well know. 😄

If we can fix the regression to support 3 levels, as it did at 0.14.4, that's sufficient for 0.15.0, or until Popover / Menu get an overhaul in the future.

[newoga]

Given that a mixin is just a means of sharing code between components (and merging lifecycle hooks etc), and that whatever code click-awayable added to Menu worked previously (however badly), this should be fixable with the current state of Menu / Popover (granted I haven't tried).

Just some quick context to clarify some things because this has gotten really confusing:

• The only regression that was introduced by ClickAwayListener was the bug described in [Core] Remove click-awayable mixin #3360 and it has already been fixed in this PR (d9878fd)
• As @nathanmarks said, the bug of using nested menu at N levels has been broken for a much longer time (since at least 0.14.4, maybe longer, though I'm thinking the break happened in 0.14.2 -- I think it was this PR ([Popover] pass "useLayerForClickAway" property #2821) but haven't confirmed )
• The regression reported by @oliviertassinari in this comment was not introduced by ClickAwayListener, but by some other change anywhere between the ClickAwayListener PR and now. I tested the commit before and after that change (with/without the fix applied in this PR) and could not reproduce the setState warning. I haven't confirmed, but the warning might have been introduced in [Core] Remove isMounted() #3437.

tl;dr As far as I'm concerned, #3360 is fixed by this PR from a regression standpoint (not fixed from a bug standpoint if you consider N levels of nesting).

The setState warning is a new "regression" that was not previously noticeable because #3360 was preventing it from happening. Though that regression is only a warning (noop from behavior standpoint).

[nathanmarks]

Yeah, I think the click behaviour of this menu is not correct for a default anyways so we should not invest too much time in making sure it is perfect right now.

Are you suggesting we just let the warning slide for the time being?

[newoga]

Are you suggesting we just let the warning slide for the time being?

Yeah I was literally writing another comment to address that point. I do think we should seriously consider just letting it slide until the component design can be fixed. IMO, the warning provided by React is there for this very reason, to indicate when something may not be designed properly. Any hacks we implement now to fix a noop warning that doesn't hit production ignores the root issue.

I can detail my (somewhat limited) findings later, but this problem is really not straightforward. It's also appears timing related (primarily as it relates to Popover animations) which makes reproducing in various commits and with various changes applied very difficult.

Add that to @nathanmarks' point that the behavior should change regardless of the implementation, I think the time may be better spend investigating alternative approaches to some of these components.

[nathanmarks]

@newoga Yeah that's what a lot of the new warnings are -- to stop users implementing poor design patterns.

Agree on all fronts dude 👍

Just to add -- combined, @newoga and I have probably already spent ~6-8 hours on this ticket.

[oliviertassinari]

@newoga I haven't found any other issue with this change.
I feel like the best UX would be to keep

  const clickAwayEvents = ['mousedown', 'touchstart'];

as a native <select /> is behaving like this. We could add an option to change it later.