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

Commit

Permalink
🐛 Tab bar shouldn't be focusable
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
matthewwithanm committed May 12, 2017
1 parent dd32a7d commit 28ef8e1
Show file tree
Hide file tree
Showing 2 changed files with 8 additions and 23 deletions.
11 changes: 2 additions & 9 deletions lib/tab-bar-view.coffee
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ class TabBarView
@element.classList.add("tab-bar")
@element.classList.add("inset-panel")
@element.setAttribute('is', 'atom-tabs')
@element.setAttribute("tabindex", -1)

@tabs = []
@tabsByElement = new WeakMap
Expand Down Expand Up @@ -404,14 +403,8 @@ class TabBarView
@rightClickedTab.element.classList.add('right-clicked')
event.preventDefault()
else if event.which is 1 and not event.target.classList.contains('close-icon')
# Delay action. This is important because the browser will set the focus
# as part of the default action of the mousedown event; therefore, any
# change we make to the focus as part of the handler would be overwritten.
# We could use `preventDefault()` to address this, but that would also
# make the tab undraggable.
setImmediate =>
@pane.activateItem(tab.item)
@pane.activate() unless @pane.isDestroyed()
@pane.activateItem(tab.item)
@pane.activate() unless @pane.isDestroyed()
else if event.which is 2
@pane.destroyItem(tab.item)
event.preventDefault()
Expand Down
20 changes: 6 additions & 14 deletions spec/tabs-spec.coffee
Original file line number Diff line number Diff line change
Expand Up @@ -261,22 +261,14 @@ describe "TabBarView", ->
spyOn(pane, 'activate')

event = triggerMouseEvent('mousedown', tabBar.tabAtIndex(0).element, which: 1)
# Pane activation is delayed because focus is stolen by the tab bar
# immediately afterward unless propagation of the mousedown event is
# stopped. But stopping propagation of the mousedown event prevents the
# dragstart event from occurring.
waits(1)
runs ->
expect(pane.getActiveItem()).toBe pane.getItems()[0]
expect(event.preventDefault).not.toHaveBeenCalled() # allows dragging
expect(pane.getActiveItem()).toBe pane.getItems()[0]
expect(event.preventDefault).not.toHaveBeenCalled() # allows dragging

event = triggerMouseEvent('mousedown', tabBar.tabAtIndex(2).element, which: 1)
waits(1)
runs ->
expect(pane.getActiveItem()).toBe pane.getItems()[2]
expect(event.preventDefault).not.toHaveBeenCalled() # allows dragging
event = triggerMouseEvent('mousedown', tabBar.tabAtIndex(2).element, which: 1)
expect(pane.getActiveItem()).toBe pane.getItems()[2]
expect(event.preventDefault).not.toHaveBeenCalled() # allows dragging

expect(pane.activate.callCount).toBe 2
expect(pane.activate.callCount).toBe 2

it "closes the tab when middle clicked", ->
jasmine.attachToDOM(tabBar.element) # Remove after Atom 1.2.0 is released
Expand Down

0 comments on commit 28ef8e1

Please sign in to comment.