Defer day focusing until next animation frame #1707
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ljharb
approved these changes
Jun 25, 2019
| @@ -58,7 +58,11 @@ class CalendarDay extends React.PureComponent { | |||
| const { isFocused, tabIndex } = this.props; | |||
| if (tabIndex === 0) { | |||
| if (isFocused || tabIndex !== prevProps.tabIndex) { | |||
| this.buttonRef.focus(); | |||
| requestAnimationFrame(() => { | |||
There was a problem hiding this comment.
do we already reference rAF elsewhere in the repo, or is this dependency a potential breaking change?
There was a problem hiding this comment.
This looks like the first usage
There was a problem hiding this comment.
any chance we could use https://npmjs.com/raf instead of the global?
nkinser
approved these changes
Jun 25, 2019
| @@ -58,7 +58,11 @@ class CalendarDay extends React.PureComponent { | |||
| const { isFocused, tabIndex } = this.props; | |||
| if (tabIndex === 0) { | |||
| if (isFocused || tabIndex !== prevProps.tabIndex) { | |||
| this.buttonRef.focus(); | |||
| requestAnimationFrame(() => { | |||
There was a problem hiding this comment.
should we cancelAnimationFrame when the component unmounts?
There was a problem hiding this comment.
I don't think we need to, since I included a check that this.buttonRef is still truthy, so if it unmounts in between, it will essentially be a noop anyway.
|
@nkinser I think it could, but hard to say. |
At Airbnb we noticed a bug affecting only mobile devices or desktop with mobile device emulation enabled. In months that span 5 weeks that are followed by months that span 6 weeks (e.g. May 2019), the scroll position of the calendar days was scrolled down some, causing the numbers to awkwardly overlap with the days of the week and look broken. This only happened when first opening the date picker, and once you go to a different month it would fix itself. After some sleuthing, I determined that this was caused by these .focus() calls happening at a weird time, causing a parent container (I think maybe the DayPicker transition container with overflow hidden) to have a > 0 scrollTop. It seems that we can prevent this from happening by deferring the focus until the next animation frame. I think this could also help us avoid some layout thrashing, which could give us a little performance boost here. I was entirely unable to reproduce this in react-dates storybook, but testing this fix out in the Airbnb codebase seems to fix the issue.
lencioni
force-pushed
the
jdl-defer-focus
branch
from
62dc070
to
1babba9
Compare
|
@ljharb I added some tests |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
At Airbnb we noticed a bug affecting only mobile devices or desktop with
mobile device emulation enabled. In months that span 5 weeks that are
followed by months that span 6 weeks (e.g. May 2019), the scroll
position of the calendar days was scrolled down some, causing the
numbers to awkwardly overlap with the days of the week and look broken.
This only happened when first opening the date picker, and once you go
to a different month it would fix itself.
After some sleuthing, I determined that this was caused by these
.focus() calls happening at a weird time, causing a parent container (I
think maybe the DayPicker transition container with overflow hidden) to
have a > 0 scrollTop.
It seems that we can prevent this from happening by deferring the focus
until the next animation frame. I think this could also help us avoid
some layout thrashing, which could give us a little performance boost
here.
I was entirely unable to reproduce this in react-dates storybook, but
testing this fix out in the Airbnb codebase seems to fix the issue.
Before fix:
After fix:
