Skip to content
This repository was archived by the owner on Dec 15, 2022. It is now read-only.

Delay activation of item on mousedown event #439

Merged
merged 2 commits into from
May 15, 2017

Conversation

matthewwithanm
Copy link
Contributor

Because the item's activation could result in a change of focus, we
need to wait to take it until the event has finished propagating.
Otherwise, the focus will be set by the browser as part of the default
action for the mousedown event.

Sorry there's no test but it doesn't seem possible to simulate the problematic browser behavior and still test what we want to.

matthewwithanm added a commit to atom/atom that referenced this pull request May 12, 2017
This was removed in #14175 in order to solve #14173 (editors not being
focused when clicking tabs). However, it means that the pane steals
focus from its children. The solution is to add the guard back and to
solve #14173 in another way: by delaying the activation of the item
(see atom/tabs#439).
Copy link
Contributor

@nathansobo nathansobo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code seems fine, but this looks like it's failing when run against the current beta branch?

matthewwithanm added a commit that referenced this pull request May 12, 2017
This was originally added way back in 310c058 however, a tabindex of -1
means that the tab bar can receive focus.

As a result, focus would end up being transferred to the tab bar as
part of the default action of the mousedown handler. When the `Pane` is
activated, it would see that its child (the tab bar) had focus and
leave it there, meaning the tab bar (instead of the editor) would
retain focus. This behavior was first reported in #14173, however the
original fix for it (#14175) resulted in Panes always stealing the
focus from their children. This regression was rectified with
atom/atom#14403 and #439, but really the tab bar shouldn't be
focusable in the first place. Making this adjustment means we no longer
have to delay the activation of the item and pane (#150 and
atom/tabs#tabs#439) since the focus will no longer be given to the tab
bar as part of the default action of the mousedown handler.
@matthewwithanm
Copy link
Contributor Author

The failure must be related but it doesn't repro for me on master or 1.17-beta5 😖

@matthewwithanm matthewwithanm force-pushed the fb-mdt-delay-item-activation branch 3 times, most recently from b673805 to ff7b282 Compare May 13, 2017 01:12
Because the item's activation could result in a change of focus, we
need to wait to take it until the event has finished propagating.
Otherwise, the focus will be set by the browser as part of the default
action for the mousedown event.
@matthewwithanm matthewwithanm force-pushed the fb-mdt-delay-item-activation branch from ff7b282 to 1849b11 Compare May 13, 2017 01:19
@matthewwithanm
Copy link
Contributor Author

matthewwithanm commented May 13, 2017

Okay so @zertosh just told me that it actually does make sense that the timeout could fire before setImmediate() (TIL). Using the same scheduler makes us green. (The promise brings in the microtask queue too, but that's not important…I just did it because we don't have a waitsForCallback 😝)

Here's his proof script, if you're interested!

setTimeout(() => {
  console.log('setTimeout-1');
}, 1);

setImmediate(() => {
  console.log('setImmediate');
});

const soon = Date.now() + 100;
while (soon > Date.now()) {}

@as-cii
Copy link
Contributor

as-cii commented May 15, 2017

This looks good, @matthewwithanm! Thanks for this pull request. ✨

I only tweaked tests a little bit in a295987 to rely more on the state of the system rather than waiting for the next setImmediate callback. I will 🚢 this as soon as we get a green build.

@as-cii as-cii merged commit 6372f97 into master May 15, 2017
@as-cii as-cii deleted the fb-mdt-delay-item-activation branch May 15, 2017 09:20
matthewwithanm added a commit that referenced this pull request May 15, 2017
This was originally added way back in 310c058 however, a tabindex of -1
means that the tab bar can receive focus.

As a result, focus would end up being transferred to the tab bar as
part of the default action of the mousedown handler. When the `Pane` is
activated, it would see that its child (the tab bar) had focus and
leave it there, meaning the tab bar (instead of the editor) would
retain focus. This behavior was first reported in #14173, however the
original fix for it (#14175) resulted in Panes always stealing the
focus from their children. This regression was rectified with
atom/atom#14403 and #439, but really the tab bar shouldn't be
focusable in the first place. Making this adjustment means we no longer
have to delay the activation of the item and pane (#150 and
atom/tabs#tabs#439) since the focus will no longer be given to the tab
bar as part of the default action of the mousedown handler.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants