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

Alternate solution for timeout blur #598

Merged
merged 7 commits into from Mar 4, 2018

Conversation

Projects
None yet
7 participants
@bartpeeters
Copy link
Contributor

bartpeeters commented Jan 3, 2018

Follow up from #579 (comment), should replace #525.


Issue:

  1. Open Firefox (its not an issue in chrome) https://codesandbox.io/s/XDAE3x0W8
  2. Click on day picker input (it shows calendar)
  3. Click on navigation bar element (next, prev or nav title elements)
  4. Main day picker input loses own focus and calendar stays open
  5. Now when you click anywhere outside of calendar. Calendar is still open but is should close.

The fix using a setTimeout onBlur to call this.input.focus() caused new issues:

  • A flicker in Chrome everytime you click inside the daypicker
  • When using a custom captionElement containing a it would not open because the timeout focus cancelled it This alternate solution is to remove the timeout and instead add an onBlur on the overlay doing the same thing as the onBlur of the input.

@gpbl gpbl changed the title alternate fix for https://github.com/gpbl/react-day-picker/pull/525/f… Alternate fix for #525 Jan 3, 2018

@gpbl

This comment has been minimized.

Copy link
Owner

gpbl commented Jan 3, 2018

Thanks @bartpeeters for your PR. I want to try it first, sure we have to remove that timeout thing since it doesn't work.

I don't understand why the new span element should fire the onBlur event here. It shouldn't ever get the focus. Have you find the reason behind it?

@gpbl gpbl changed the title Alternate fix for #525 Alternate solution for timeout blur Jan 3, 2018

@codecov

This comment has been minimized.

Copy link

codecov bot commented Jan 3, 2018

Codecov Report

Merging #598 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #598   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          15     15           
  Lines         621    620    -1     
  Branches      133    133           
=====================================
- Hits          621    620    -1
Impacted Files Coverage Δ
src/DayPickerInput.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3156e3d...04c3173. Read the comment docs.

@bartpeeters

This comment has been minimized.

Copy link
Contributor Author

bartpeeters commented Jan 3, 2018

@gpbl tabIndex on the daypicker (here) makes it so the span is focusable.

@gpbl
Copy link
Owner

gpbl left a comment

@bartpeeters

tabIndex on the daypicker (here) makes it so the span is focusable.

This does not make sense to me. The tabIndex in the DayPicker may fire a blur event, but when it is focused. Why should the parent span fire a blur event? blur does not bubble 🤔

ref={el => (this.daypicker = el)}
onTodayButtonClick={onTodayButtonClick}
{...dayPickerProps}
<span onBlur={this.handleInputBlur}>

This comment has been minimized.

@gpbl

gpbl Jan 5, 2018

Owner

My suspect is that this line does nothing. We should add an unit test to it.

@@ -52,18 +52,6 @@ describe('DayPickerInput', () => {
wrapper.find('input').simulate('blur');
expect(onBlur).toHaveBeenCalledTimes(1);
});
it('should focus the input if blur after clicking the overlay', done => {

This comment has been minimized.

@gpbl

gpbl Jan 5, 2018

Owner

This test must stay here :) Why does your change make it fail?

@bartpeeters

This comment has been minimized.

Copy link
Contributor Author

bartpeeters commented Jan 7, 2018

@gpbl Thanks for the feedback.

I implemented another solution, using the onFocus event of the overlay to always make sure the DaypickerInput input field is focused.

But do we want this? What if we have a custom overlay that includes an input? Now this input is unusable because it can't be focused. We can check if the nodeName of the focus event target is an input, but I don't really know if that is the way to go.

I also replaced the clickInside with the timeout by using a ref on the Overlay and checking if the relatedTarget of blur event is inside the overlay. I'm not sure that this is better than using a timeout.

About the bubbling of the onFocus and onBlur on the span which contains the overlay, the React Synthetic onFocus and onBlur events do bubble, unlike the normal blur and focus events.

@gpbl

This comment has been minimized.

Copy link
Owner

gpbl commented Jan 9, 2018

@bartpeeters thanks for the updates. I've not checked your changes yet, however I'm proposing a different solution:

  • reset this line to the original behavior, which was working well except on Firefox:
if (this.clickedInside) { 
-    this.blurTimeout = setTimeout(() => this.input.focus(), 0); 
+   this.input.focus(); 
}
  • on Firefox, we just disable this to resolve #579:
- if (this.clickedInside) { 
+ if (!isFirefox && this.clickedInside) { 
  • for the case when the calendar has other focusable elements inside, such as selects or input fields (as in #599), we allow the implementer to disable this focus with a new DayPickerInput prop: keepFocus that can be set to false.
@bartpeeters

This comment has been minimized.

Copy link
Contributor Author

bartpeeters commented Jan 12, 2018

@gpbl I like the idea of a new prop keepFocus.

The solution you propose has the same effect as these changes except they dont use isFirefox, so maybe you can take a look at the code. I'm also up to add the keepFocus prop and some tests if you approve of these changes.

@mattfysh

This comment has been minimized.

Copy link

mattfysh commented Jan 18, 2018

@gpbl how can I help push this forward? would really appreciate being able to set keepFocus to false

@bartpeeters bartpeeters force-pushed the bartpeeters:master branch from 9fbf809 to abd1e78 Jan 20, 2018

bartpeeters and others added some commits Jan 3, 2018

Bart Peeters
add back focus test. Another solution to fixing firefox bug using the…
… onFocus of the overlay to always make the DaypickerInput focused, but do we wanth this?
Bart Peeters
Add keepFocus prop, when true focusing the overlay will focus the
input, when false focusing the overlay will let the event through
Add onBlur on overlay that hides the overlay
when clicked outside overlay ant not on input.
Add tests for keepFocus and overlayblur, these are hard because enzyme
focus and blur don't work like in a real browser: focusing the overlay
will not let actually focus it, body will still be focused, so we check
that the input is not focused when keepFocus is false.
@bartpeeters

This comment has been minimized.

Copy link
Contributor Author

bartpeeters commented Jan 20, 2018

@mattfysh I just added the keepFocus prop and added api documentation for it. I think now we just need to wait till @gpbl has some time to review these changes.

Bart Peeters added some commits Jan 20, 2018

@@ -246,6 +247,16 @@ function MyDayPickerInput(props) {
<p>
The value of the <code>input</code> field.
</p>
<h3>
<Anchor id="keepFocus" />
keeFocus <code>boolean = true</code>

This comment has been minimized.

@ghaynes1292

ghaynes1292 Feb 7, 2018

keepFocus

This comment has been minimized.

@bartpeeters

bartpeeters Feb 7, 2018

Author Contributor

Thanks!

@rdiwelm

This comment has been minimized.

Copy link

rdiwelm commented Feb 14, 2018

Hey guys! How soon can we expect a new version release with the new keepFocus prop?

@gpbl

This comment has been minimized.

Copy link
Owner

gpbl commented Feb 16, 2018

@rdiwelm soon ! I have been pretty busy the last month, hoping to catch up all this weekend 🙏🏽 sorry for the lack of communication.

@roginfarrer

This comment has been minimized.

Copy link

roginfarrer commented Feb 23, 2018

I also have a feature depending on this release!

@rdiwelm

This comment has been minimized.

Copy link

rdiwelm commented Feb 27, 2018

Hey guys! Sorry to be asking again. Is there any ETA on this? We're kinda blocked too. :/

@kradical

This comment has been minimized.

Copy link
Contributor

kradical commented Feb 28, 2018

@roginfarrer @rdiwelm I was blocked by this too so we forked this dependency until this change gets merged. I understand that is not always a viable option.

@gpbl gpbl merged commit 6d16700 into gpbl:master Mar 4, 2018

3 checks passed

ci/circleci: checkout-and-test Your tests passed on CircleCI!
Details
codecov/patch 100% of diff hit (target 100%)
Details
codecov/project 100% (+0%) compared to 3156e3d
Details

@gpbl gpbl added the v:minor label Mar 5, 2018

@gpbl gpbl added this to the v7.1.0 milestone Mar 5, 2018

@gpbl

This comment has been minimized.

Copy link
Owner

gpbl commented Mar 5, 2018

Published as v7.1.0.

@rdiwelm

This comment has been minimized.

Copy link

rdiwelm commented Mar 5, 2018

Awesome! Thanks a lot, guys! :)

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