Skip to content

[ClickAwayListener] Fix mounting behavior in Portals in React 17#23315

Merged
oliviertassinari merged 4 commits intomui:nextfrom
eps1lon:fix/ClickAwayListener/react-17
Nov 1, 2020
Merged

[ClickAwayListener] Fix mounting behavior in Portals in React 17#23315
oliviertassinari merged 4 commits intomui:nextfrom
eps1lon:fix/ClickAwayListener/react-17

Conversation

@eps1lon
Copy link
Member

@eps1lon eps1lon commented Oct 29, 2020

We actually rely on the useEffect semantics i.e. the click handler should not be attached synchronously. Otherwise, we might handle the event that mounted the component. Unfortunately, react 17 has a bug where effects are flushed synchronously in portals so we have to find a userland workaround.

Benefits over #23313: Since we have many listeners we would need to repeat it whenever we add the event listeners. Touches might behave the same as clicks so we're fixing the same problem for touch events as well. There's an argument to be made to get rid of mountedRef/activatedRef entirely since that's no different from just removing the handler but the "mounted"-pattern helps here since we don't have to repeat this quirky logic everywhere. If the bug is fixed we should be able to get rid of the ref entirely. That IE11 comments is curious. React 17 removes the listener asynchronously anyway so I wonder if there's a realistic chance that we trigger a click-away between unmount and removal of the listener.

I was not able to write a test for it since the synchronous flush only happens on a user-click and not programmatic (element.click) or dispatched (element.dispatchevent) click.

facebook/react#20074 tracks the React bug.

Closes #23215
Closes #23313

Test plan

@eps1lon eps1lon added type: bug It doesn't behave as expected. scope: click away listener This is the name of the generic UI component, not the React module! labels Oct 29, 2020
@mui-pr-bot
Copy link

mui-pr-bot commented Oct 30, 2020

Details of bundle changes

Generated by 🚫 dangerJS against 0edbb25

@eps1lon eps1lon marked this pull request as ready for review October 30, 2020 13:30
@eps1lon eps1lon added the on hold There is a blocker, we need to wait. label Oct 31, 2020
@eps1lon
Copy link
Member Author

eps1lon commented Oct 31, 2020

Should have a test for it now.

@eps1lon eps1lon force-pushed the fix/ClickAwayListener/react-17 branch from 802f929 to 6638a8e Compare October 31, 2020 10:43
@eps1lon eps1lon marked this pull request as draft October 31, 2020 10:43
@eps1lon eps1lon removed the on hold There is a blocker, we need to wait. label Oct 31, 2020
@eps1lon eps1lon force-pushed the fix/ClickAwayListener/react-17 branch from 6638a8e to d52e2b4 Compare October 31, 2020 10:57
@eps1lon eps1lon marked this pull request as ready for review October 31, 2020 12:27
@oliviertassinari oliviertassinari merged commit 2ab6acd into mui:next Nov 1, 2020
@oliviertassinari
Copy link
Member

@eps1lon Great job!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

scope: click away listener This is the name of the generic UI component, not the React module! type: bug It doesn't behave as expected.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[ClickAwayListener] Is being fired without an onClick event after upgrading react and react-dom to v17

3 participants