Alternate solution for timeout blur #598

bartpeeters · 2018-01-03T18:52:13Z

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

Issue:

Open Firefox (its not an issue in chrome) https://codesandbox.io/s/XDAE3x0W8
Click on day picker input (it shows calendar)
Click on navigation bar element (next, prev or nav title elements)
Main day picker input loses own focus and calendar stays open
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 · 2018-01-03T21:48:25Z

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?

codecov · 2018-01-03T22:10:37Z

Codecov Report

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

@@          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 · 2018-01-03T22:40:55Z

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

gpbl

@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 🤔

gpbl · 2018-01-05T21:23:17Z

src/DayPickerInput.js

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


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

gpbl · 2018-01-05T21:25:29Z

test/daypickerinput/events.js

@@ -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 test must stay here :) Why does your change make it fail?

bartpeeters · 2018-01-07T13:33:29Z

@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 · 2018-01-09T08:35:01Z

@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 Switch between years and months on DatePickerInput #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 v7 - Input stealing focus when interacting with caption element #599), we allow the implementer to disable this focus with a new DayPickerInput prop: keepFocus that can be set to false.

bartpeeters · 2018-01-12T20:40:53Z

@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 · 2018-01-18T00:10:45Z

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

… onFocus of the overlay to always make the DaypickerInput focused, but do we wanth this?

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 · 2018-01-20T15:02:11Z

@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.

ghaynes1292 · 2018-02-07T21:04:37Z

docs/src/pages/api/DayPickerInput.js

@@ -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>


rdiwelm · 2018-02-14T04:32:44Z

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

gpbl · 2018-02-16T09:18:38Z

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

roginfarrer · 2018-02-23T19:47:13Z

I also have a feature depending on this release!

rdiwelm · 2018-02-27T02:41:52Z

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

kradical · 2018-02-28T18:14:17Z

@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 · 2018-03-05T10:05:30Z

Published as v7.1.0.

rdiwelm · 2018-03-05T16:29:47Z

Awesome! Thanks a lot, guys! :)

Alternate solution for timeout blur

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 changed the title ~~Alternate fix for #525~~ Alternate solution for timeout blur Jan 3, 2018

gpbl reviewed Jan 5, 2018

View reviewed changes

gpbl mentioned this pull request Jan 9, 2018

v7 - Input stealing focus when interacting with caption element #599

Closed

bartpeeters force-pushed the master branch from 9fbf809 to abd1e78 Compare January 20, 2018 12:10

bartpeeters and others added 4 commits January 20, 2018 13:59

Fix test clearTimeout blur

89b3a1a

remove check for focus after blur in test

dfb0b34

add back focus test. Another solution to fixing firefox bug using the…

73066ab

… onFocus of the overlay to always make the DaypickerInput focused, but do we wanth this?

Bart Peeters added 2 commits January 20, 2018 16:44

fix lint errors and remove package-lock

06d1d95

add documentation for the new keepFocus prop

769e3f2

ghaynes1292 reviewed Feb 7, 2018

View reviewed changes

Fix documentation typo

04c3173

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

gpbl mentioned this pull request Mar 5, 2018

Switch between years and months on DatePickerInput #579

Closed

gpbl added the v:minor label Mar 5, 2018

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

leiit mentioned this pull request Mar 15, 2018

DayPickerInput: Click handling broken in IE #668

Closed

kimamula pushed a commit to kimamula/react-day-picker that referenced this pull request Aug 17, 2022

Merge pull request gpbl#598 from bartpeeters/master

6ae71cc

Alternate solution for timeout blur

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Alternate solution for timeout blur #598

Alternate solution for timeout blur #598

bartpeeters commented Jan 3, 2018 •

edited by gpbl

Loading

gpbl commented Jan 3, 2018 •

edited

Loading

codecov bot commented Jan 3, 2018 •

edited

Loading

bartpeeters commented Jan 3, 2018

gpbl left a comment

gpbl Jan 5, 2018

gpbl Jan 5, 2018

bartpeeters commented Jan 7, 2018 •

edited

Loading

gpbl commented Jan 9, 2018

bartpeeters commented Jan 12, 2018

mattfysh commented Jan 18, 2018

bartpeeters commented Jan 20, 2018

ghaynes1292 Feb 7, 2018

bartpeeters Feb 7, 2018

rdiwelm commented Feb 14, 2018

gpbl commented Feb 16, 2018

roginfarrer commented Feb 23, 2018

rdiwelm commented Feb 27, 2018

kradical commented Feb 28, 2018

gpbl commented Mar 5, 2018

rdiwelm commented Mar 5, 2018

Alternate solution for timeout blur #598

Alternate solution for timeout blur #598

Conversation

bartpeeters commented Jan 3, 2018 • edited by gpbl Loading

gpbl commented Jan 3, 2018 • edited Loading

codecov bot commented Jan 3, 2018 • edited Loading

Codecov Report

bartpeeters commented Jan 3, 2018

gpbl left a comment

Choose a reason for hiding this comment

gpbl Jan 5, 2018

Choose a reason for hiding this comment

gpbl Jan 5, 2018

Choose a reason for hiding this comment

bartpeeters commented Jan 7, 2018 • edited Loading

gpbl commented Jan 9, 2018

bartpeeters commented Jan 12, 2018

mattfysh commented Jan 18, 2018

bartpeeters commented Jan 20, 2018

ghaynes1292 Feb 7, 2018

Choose a reason for hiding this comment

bartpeeters Feb 7, 2018

Choose a reason for hiding this comment

rdiwelm commented Feb 14, 2018

gpbl commented Feb 16, 2018

roginfarrer commented Feb 23, 2018

rdiwelm commented Feb 27, 2018

kradical commented Feb 28, 2018

gpbl commented Mar 5, 2018

rdiwelm commented Mar 5, 2018

bartpeeters commented Jan 3, 2018 •

edited by gpbl

Loading

gpbl commented Jan 3, 2018 •

edited

Loading

codecov bot commented Jan 3, 2018 •

edited

Loading

bartpeeters commented Jan 7, 2018 •

edited

Loading