Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Uses React's onClick handler instead of OutsideClickHandler with withPortal implementations #421

Merged
merged 1 commit into from
Apr 4, 2017

Conversation

majapw
Copy link
Collaborator

@majapw majapw commented Apr 3, 2017

Fix for #340

This fixes an issue with Firefox specifically where the order of operations of the events caused the withPortal DayPicker to immediately close upon opening.

to: @lencioni
fyi: @phamcharles

…ithPortal` implementations

This fixes an issue with Firefox specifically where the order of operations of the events caused the `withPortal` DayPicker to immediately close upon opening.
@ljharb
Copy link
Member

ljharb commented Apr 3, 2017

Do we want to do this unconditionally, or only when withPortal is false?

@majapw
Copy link
Collaborator Author

majapw commented Apr 3, 2017

@ljharb If you look at where onOutsideClick the variable is defined, we do it conditionally:

const onOutsideClick = (!withFullScreenPortal && withPortal) ? this.onClearFocus : undefined;

which is what we want.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 80.059% when pulling a548fbc on maja-fix-firefox-outside-click-with-portal-woes into 5bdcdc4 on master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 80.059% when pulling a548fbc on maja-fix-firefox-outside-click-with-portal-woes into 5bdcdc4 on master.

@ljharb
Copy link
Member

ljharb commented Apr 3, 2017

Is this an issue with OutsideClickHandler itself; or with our event handler not checking the event target properly? In other words, is this something we could solve while keeping OutsideClickHandler (even if this is a good hotfix in the interim)?

@majapw
Copy link
Collaborator Author

majapw commented Apr 3, 2017

@ljharb So I don't think it is specifically an issue with the OutsideClickHandler. It def does check event target properly, but the issue is with the way the native event system plays with the react event system/render cycle.

Basically, here is what happens on Chrome:

  1. input takes focus event when you click on the input
  2. focus event moves up the tree and triggers onFocusChange
  3. onFocusChange updates state.focusedInput
  4. click event happens somewhere between 2 and here
  5. render cycle begins, rendering the withPortal calendar to the page

Conversely, here is what happens in Firefox

  1. input takes focus event when you click on the input
  2. focus event moves up the tree and triggers onFocusChange
  3. onFocusChange updates state.focusedInput
  4. render cycle begins, rendering the withPortal calendar to the page
  5. click event gets triggered
  6. because the click is now outside the root node (the visible calendar area) of the now renderedOutsideClickHandler, onOutsideClick gets triggered
  7. the calendar closes

Because the initial event is a (1) a focus event and (2) a react event, and the outside click event is (1) a click event and (2) a native JS event, I think there is some conflict between making them play nice together in this one weird edge case. I discussed this with @lencioni and I think this is the solution and not just a hotfix.

@majapw
Copy link
Collaborator Author

majapw commented Apr 4, 2017

Do you want to :stamp: me @ljharb? :D

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I kind of get your explanation, and I'm really nervous about adding an onClick to a div instead of using our nice abstracted component; but your changes look fine.

@majapw majapw merged commit 878eb51 into master Apr 4, 2017
@majapw majapw deleted the maja-fix-firefox-outside-click-with-portal-woes branch April 4, 2017 03:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants