Skip to content
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

Prevent race conditions in focus management code #1631

Merged
merged 15 commits into from Aug 9, 2018

Conversation

2 participants
@smashwilson
Copy link
Member

smashwilson commented Aug 9, 2018

The root cause of #1490 and other related uncaught TypeError bugs is a set of somewhat rare race conditions triggered by focus management methods (rememberLastFocus, setFocus, hasFocus, ...) being triggered on our React components while refs are unavailable. We had a certain amount of null-armor in there already, but we missed a few subtle code paths.

I'm resolving this by consistently using RefHolders to store refs in the GitTabItem component tree. RefHolders force us to always explicitly handle the case when a ref is unavailable.

Note that there's still the risk of a focus event being ignored because of this. I thought about catching focus calls and playing them back later when refs do become available but I feel like that would cause a bunch of weird cases where we yank focus in undesirably because it's already moved elsewhere. The risk there is that you might have to click twice sometimes, I suppose?

I'm also very carefully not taking this opportunity to tackle reworking focus management holistically in a fancier, more React-friendly declarative style. Now is the time for band-aids!

smashwilson added some commits Aug 9, 2018

@coveralls

This comment has been minimized.

Copy link

coveralls commented Aug 9, 2018

Coverage Status

Coverage increased (+0.6%) to 80.164% when pulling ab1a112 on aw/last-focus into 7ecd93a on master.

smashwilson added some commits Aug 9, 2018

@smashwilson smashwilson merged commit f5a4dc0 into master Aug 9, 2018

4 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.6%) to 80.164%
Details

Stability Sprint : 23 July - 3 August 2018 : v0.19.0 automation moved this from In Progress 🔧 to Merged ☑️ Aug 9, 2018

@smashwilson smashwilson deleted the aw/last-focus branch Aug 9, 2018

smashwilson added a commit that referenced this pull request Aug 14, 2018

Merge pull request #1631 from atom/aw/last-focus
Prevent race conditions in focus management code
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.