-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Fix React warning when Events are referenced later #682
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you elaborate on what this option does? Is there any possibility that this could result in zero events being fired?
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 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: 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:
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 ) |
I think perhaps it might be better to move |
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 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 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. |
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. |
There is also a throttled |
@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! |
@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? |
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. |
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. |
@erin-doyle if you cherry-pick these commits into your PR, that would work just fine :-) |
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: For DayPicker.jsx: |
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 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 Unfortunately, my attempts to rebase against
|
@mikelambert try |
Fixes react-dates#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.
… presses (ie, shift event on the way to a shift-tab event). Also turn off trailing edges, so that we don't need to hold onto events past their usefulness.
Ahhh great, thanks, makes sense! Rebased, and now needs re-approval. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but will defer to @erin-doyle here
src/components/DateInput.jsx
Outdated
@@ -126,15 +127,20 @@ class DateInput extends React.Component { | |||
|
|||
onKeyDown(e) { | |||
e.stopPropagation(); | |||
if (!['Shift', 'Control', 'Alt', 'Meta'].includes(e.key)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for perf, this should probably be an object literal or a Set defined at the module level.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 !
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.