Fix React warning when Events are referenced later

[mikelambert]

Fixes #514 , which happens due to Events being stored to be delivered on the trailing edge of a keyDown. I believe we don't need the trailing edge, and it is better than a persisted Event.

[coveralls]

Coverage remained the same at 86.471% when pulling 4dfd0dc on mikelambert:patch-2 into 4de2733 on airbnb:master.

[ljharb]

Can you elaborate on what this option does? Is there any possibility that this could result in zero events being fired?

[mikelambert]

Please don't approve/submit yet. My fix may not be correct, but the existing code is also broken.

When a throttled function, you have the leading edge (the first time it's called), the inprogress events (that only get called-through if more than 300ms have elapsed since the last one), and then the trailing edge (which happens 300ms after the last call, if there have been no intermediate calls, and represents the last call that was received and buffered while waiting for 300ms to elapse.) There are probably lodash throttle/debounce documentation/stackoverflows that explain this better than I can. :)

Regardless, the trailing edge is delivered and called some number of milliseconds after the event was fired. This means that React has already cleared the event and it back into the pool for later reuse. So at this point, it is too late to call e.persist(), as the data has been lost.

In my pull request assumed that a series of keyDown events would all be identical, and so we could ignore the trailing edge. I was wrong. Consider this sequence of events:
a) hit Shift
b) now while Shift is pressed, hit Tab

If (b) happens within 300ms of (a), it will not be delivered. With the original code, (b) will be cleared, and be a dummy event by the time it is delivered. With my code, (b) will never be delivered. In both cases, the use of throttle causes (b) to not be delivered at all.

If you access the storybook and quickly hit shift-tab out of the Start field, the Picker will incorrectly remain open. If you wait a bit between shift-and-tab, it will correctly close.

I believe the correct fix here is to do one of the following:

1. turn off throttle entirely (I am curious if you can explain why it was necessary in the first place?)
2. implement throttle manually, such that instead of the default of saving arguments for later delivery...it actually calls event.persist() and saves them for later delivery

Regardless, my pull request as it exists is not correct, and is "as broken" as it was before (though with less React console warnings :P )

[ljharb]

I think perhaps it might be better to move throttle usage inside the onKeyDown handler?

[coveralls]

Coverage increased (+0.03%) to 86.5% when pulling ed694bc on mikelambert:patch-2 into 4de2733 on airbnb:master.

[mikelambert]

Going with your approach, there are still some issues with modifier-only keydowns "eating up" the throttle and keeping modifier+real_key events from being delivered. So I moved the throttle into onKeyDown, and omit modifier-only events, and then throw the rest through the throttler, which should work.

At this point, I also turned off the trailing edge. This means if you hold down the "tab" key for more than the key-repeat-delay and less than 300 ms, one tab will be delivered. Unlike if we support trailing edges (the original default), where it would send two tab events (ie, delivering the second tab button). This meant a "second event" could occur up to 300ms after the user let go of the key, which seems like a Bad Idea to me.

On the bright side, this also means we can let the React SyntheticEvents go back into the pool for re-use, and we don't have to worry about GC-ing these persist-ed objects either, which should be a bit better performance-wise as well.

I have tested this code with a bunch of attempted-fast tab and shift-tab in the storybook, and it seems to work without any issues.

[erin-doyle]

This is awesome @mikelambert thanks for submitting this! These warnings have been driving me crazy but I haven't had the time to dig in and look for a fix.

[erin-doyle]

There is also a throttled onKeyDown in DayPicker.js which manages full keyboard navigation capabilities within the Calendar area. There is an onKeyDown handler in the DayPickerKeyboardShortcuts.js as well though it's not being throttled currently but it's possible that was just a miss. Can you take a look at those as well?

[erin-doyle]

@mikelambert I stumbled upon this again. I'm noticing some serious lag at times in the key event handling along with the SyntheticEvent warnings and I really wonder if they'd all be fixed with this. Can you please see the feedback I left back on Nov 17 and make those changes? It would be great to get this merged!

[erin-doyle]

@ljharb what do you think about us merging this as is and I can follow up with making these changes in the remaining components that need them?

[ljharb]

Either way this still needs a rebase; and since this PR was from prior to the react-with-styles change, it might not be a trivial one.

[erin-doyle]

Ok well I'm actively working right now on trying to improve some issues with the keyboard navigation and I think this could be the fix to the problems I'm seeing. So I've already copied these changes into the branch I'm working on and making the same changes to the other components. If I get done before this has been rebased then we can supersede this with my PR.

[ljharb]

@erin-doyle if you cherry-pick these commits into your PR, that would work just fine :-)

[erin-doyle]

Ok so I take it back, I'm not sure if we should make these changes exactly to DayPicker.jsx and DayPickerKeyboardShortcuts.jsx.

First for DayPickerKeyboardShortcuts.jsx:
When adding a throttle to keyDown what it ends up doing is not being able to preventDefault on the long list of keys that need to be prevented but we really have to run through the function to determine what key it is and what action to take. It just doesn't make sense to me at that point to try to throttle anything here (and it's not being throttled at all now without issue).

For DayPicker.jsx:
In my testing it seems almost as if the throttle is not even really working as it is now. I can hold down keys and they aren't being throttled, I can increase the delay time and nothing changes. If I make the same changes as are here in this PR on DateInput.jsx, having onKeyDown call the throttle function, it definitely is throttling, such that the experience is kind of laggy. I am really starting to wonder if we should not use throttling at least here in this component. I would expect that most users navigating through the dates using arrow keys will be clicking on them fairly quickly (if they are not using a screen reader and waiting for the reading) and expecting them to be responsive. Reducing the delay to 200 is a decent compromise.

[coveralls]

Coverage increased (+0.03%) to 85.515% when pulling 95782e0 on mikelambert:patch-2 into 92cdc04 on airbnb:master.

[mikelambert]

Hi everyone, sorry for being MIA on the change request. :/ I am glad to hear Erin was able to try these changes out, and will defer to her experience since I'm not as familiar with DayPicker stuff. (And I'm happy to get this one change submitted as-is and let others deal with proper DayPicker fixes, or let someone else own this change and get the combined set of proper changes in.)

As far as this particular pull request, I just did a git merge to head, and there were no issues. I am not sure why react-with-styles would cause any merge issues, since this is unrelated to CSS?

Unfortunately, my attempts to rebase against master are failing. It seems there are multiple tag versions all being committed into master branch (16.0.1 and 15.5.2 in the same master commit log?), and my git-fu is not good enough to figure out how to rebase against multiple tags with differing versions somehow coexisting in the same commit log... ie, my git rebase master -i shows:

...
pick 94e1852 Version 16.0.0
pick 4d3b312 Add missing onKeyDown function to CalendarDay
pick e57d119 Version 16.0.1
pick 636f619 Revert "Merge pull request #866 from airbnb/maja-make-drp-ids-required"
pick 065b6a3 Version 15.5.2
pick f48b820 Version 15.5.3

[ljharb]

@mikelambert try git rebase upstream/master -i instead (where "upstream" is this repo)

[coveralls]

Coverage increased (+0.03%) to 85.515% when pulling 7ada678 on mikelambert:patch-2 into 92cdc04 on airbnb:master.

[mikelambert]

Ahhh great, thanks, makes sense! Rebased, and now needs re-approval.

[ljharb]

LGTM, but will defer to @erin-doyle here

[ljharb]

for perf, this should probably be an object literal or a Set defined at the module level.

[mikelambert]

Fixed.

[coveralls]

Coverage increased (+0.04%) to 85.524% when pulling 45037c8 on mikelambert:patch-2 into 92cdc04 on airbnb:master.

[erin-doyle]

Yep, I am now happy with just changing DateInput.jsx here for the purpose of this PR and I'll work on the rest. Thanks @mikelambert !