Toggling views/panels has inconsistent focus behaviour

[bpasero]

While investigating https://github.com/microsoft/vscode/issues/198293 I noticed that our action to toggle visibility of primary or secondary side bar or panel has inconsistent behaviour when it comes to passing focus to the view that becomes visible or not.

Notice when the explorer is active how focus remains in the editor:

And now with the SCM view:

First of all, these actions seem to call into methods to toggle visibility of the container via layout service which eventually calls into openPaneComposite, e.g. for the primary sidebar:

https://github.com/microsoft/vscode/blob/4520d915c98954dc96dd0bc00b8bb68181cbf2b6/src/vs/workbench/browser/layout.ts#L1750

The last parameter is a true to indicate that focus should move to that pane.

However, at the point where we want to focus the pane, it is not yet visible because the grid only updates a few lines below:

https://github.com/microsoft/vscode/blob/4520d915c98954dc96dd0bc00b8bb68181cbf2b6/src/vs/workbench/browser/layout.ts#L1756

Any view that implements focus by e.g. focussing the list will be a no-op because the DOM nodes are not yet visible. The SCM view is probably a bit async and that is why it works by chance.

I am actually not sure how to address this: people might have gotten used to the fact that these commands typically preserve focus for most of our views. I still think that the implementation is currently buggy around focussing the pane when the container becomes visible, so maybe a fix would need to be:

• handle focus properly after the container in the grid is visible
• make focus more explicit so that the toggling actions can explicitly pass in focus: false to preserve todays behaviour

//cc @sbatten

[benibenj]

I started to investigate this last week. The the SCM view has a custom hack to make this work. But for all other views focus is not set correctly because the dom elements are not visible at the time of focus being set. Moving the this.workbenchGrid.setViewVisible(this.sideBarPartView, !hidden); statement up before the open composite call fixes this. It's an easy fix for both side bars, however, there seems to be a bit more going on in the panel which is why I have not pushed a fix yet

https://github.com/microsoft/vscode/blob/0334df2899efa05a65a86f79e3f76588e17c667f/src/vs/workbench/browser/layout.ts#L1899-L1905

It looks like setPanelHidden runs twice. The first time when the user toggles the panel and the second time when the grid updated. This seems a bit odd

[bpasero]

Yeah but I am also not certain what the right fix is here actually: should toggling focus or not. So far, most views are not focused and changing that might break muscle memory 🤔

[benibenj]

Yeah I'm also not sure. In most cases we do move focus. For example when you use the keybinding to show a view (Ctrl+Shift+E) so it might make sense to be consistent and always move focus.

@joaomoreno what do you think? You reported this behaviour as being a bug microsoft/vscode-copilot#9799

[joaomoreno]

So far, most views are not focused and changing that might break muscle memory 🤔

I feel the same way: let's take the path of least breakage when trying to be consistent. I.e. let's not focus views when side bars come and go, unless it's an explicit call like workbench.scm.focus.