-
Notifications
You must be signed in to change notification settings - Fork 120
Delay activation of item on mousedown event #439
Conversation
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).
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.
The code seems fine, but this looks like it's failing when run against the current beta branch?
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.
The failure must be related but it doesn't repro for me on master or 1.17-beta5 😖 |
b673805
to
ff7b282
Compare
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.
ff7b282
to
1849b11
Compare
Okay so @zertosh just told me that it actually does make sense that the timeout could fire before 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()) {} |
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 |
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.
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.