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

Creating a new empty editor group can leave focus in inactive group #189256

Closed
Tracked by #189320
matthew-e-brown opened this issue Jul 30, 2023 · 10 comments
Closed
Tracked by #189320
Assignees
Labels
author-verification-requested Issues potentially verifiable by issue author bug Issue identified by VS Code Team member as probable bug insiders-released Patch has been released in VS Code Insiders verified Verification succeeded workbench-editor-groups Issues related to editor groups

Comments

@matthew-e-brown
Copy link

matthew-e-brown commented Jul 30, 2023

Information

Does this issue occur when all extensions are disabled?:

Yes.

VS Code version / OS version:

Version: 1.80.2 (user setup)
Commit: 2ccd690cbff1569e4a83d7c43d45101f817401dc
Date: 2023-07-27T20:40:28.909Z
Electron: 22.3.14
ElectronBuildId: 22695494
Chromium: 108.0.5359.215
Node.js: 16.17.1
V8: 10.8.168.25-electron.0
OS: Windows_NT x64 10.0.19045

Expected behaviour

Explicitly creating a new group above, below, to the right, or to the left of the current group should always behave the same. With regards to focus, I expect that the new, empty group should receive focus right away. This means that commands like Go to File... (CTRL+P) or File: New Untitled Text File (CTRL+N) should open and create files in the new group.

As intuition would imply, the new group receiving focus should disable text input in the previous editor. From there, it stands to reason that using workbench.action.focus(Above|Below|Left|Right)Group should be able to move focus back to the original group, or to any other groups relative to the new one.

Actual behaviour

When creating a new group, there are certain scenarios (detailed below) in which the new editor does not properly receive focus. Instead, VS Code enters a strange state where focus appears to be both on the new editor group and in the previous editor at the same time. This breaks a few things in odd ways:

  • CTRL+N correctly creates a blank plain-text editor in the new group;
  • but CTRL+P opens a new file in the previous group, where the text focus is.
  • All "text-related" inputs are still made to the previous group: typing, navigating with page up and down, CTRL+ALT+↑/↓ to create multi-cursors, and so on.
  • Commands that act on the currently focused tab or group do not work: you can type into the "half" focused editor, but CTRL+S does nothing.
  • Because the focus*Group commands work relative to the currently focused group, and the group focus is updated correctly, getting text focus into the new group cannot be done with a simple focus*Group command. For example, newGroupRight followed by focusRightGroup does nothing, unless that new group has something further to the right of it, in which case text focus will move all the way to the third group.

Steps to reproduce

I have done more than an hour of testing, and still cannot track down the exact circumstances under which this bug occurs. It seems to depend on a number of layout-related factors.

  • It seems to happen when the most recently closed group is on the same axis as the desired new group's position.
  • It seems to be affected by the number of currently opened groups, as well as whether or not there are any editors within those groups.
  • It seems to also occur under slightly different circumstances for X/Y axes (newGroupLeft and newGroupRight vs. newGroupAbove and newGroupBelow).
  • The workbench.editor.closeEmptyGroups setting does not seem to affect this behaviour.

Below is a series of steps to reproducibly demonstrate the above points. Although I just mentioned that closeEmptyGroups does not affect the behaviour, these steps should be followed with it set to true in order to get identical results.

  1. Open a new VS Code window in a folder/workspace with at least one file in it (to be able to test CTRL+P).
  2. Open any file either with CTRL+P or by double clicking on it in the explorer. It should be in what is currently the only editor in the only group.
  3. Using either the command palette or a keyboard shortcut, open a new editor group to the right or to the left.
    • The new group should be darkened, indicating focus.
    • The tabs at the top of the old group should be dimmed, indicating a lack of focus.
    • The caret should no longer be visible in the previous editor.
    • As such, attempts to enter text or navigate with the keyboard should do nothing.
  4. Open a file in this new editor group (CTRL+P or CTRL+N).
    • It should work completely fine.
  5. Close the second editor group (CTRL+W).
  6. Open a new editor group to the right or to the left.
    • The new group should be darkened.
    • The tabs at the top of the old group should be dimmed.
    • This time, however, the caret should remain in the previous editor.
    • Entering or deleting text, navigating, and so on, work in the previous editor as if no other group was ever made.
    • Attempts to save the file, however, should do nothing.
  7. Open a file in this new editor group (CTRL+P).
    • It should appear in the first editor group, restoring focus to that group and leaving the second one empty.
  8. Close the second editor group (CTRL+W).
  9. Open a new second group above or below.
    • Because the most recently closed group was to the side, this new group should function completely normally (at least, that's my best guess as to why).
  10. Use CTRL+P to open a file in this new editor.
    • It should have focus and work totally fine.
  11. Open a third group to the left or to the right of the second one.
    • It should also receive focus and work totally fine.
  12. Open a file in the third editor group.
  13. Open a fourth, final group to the left or to the right of the third group.
    • Text focus should stay in the third group.
    • You can now choose your own adventure: CTRL+P will open a file in the third group, leaving the fourth empty, but CTRL+N will open a blank plain-text file in the fourth group.

Screen recording

2023-07-30_00-21-02.mp4

Keyboard shortcuts of note, if you're going to go off of the screen-recording:

  • CTRL+E, CTRL+SHIFT+[Arrow] - new editor group
  • CTRL+E, CTRL+[Arrow] - move focus to group

Sorry (🍁) that this issue is so insanely long. I really wanted to make sure that it is accurate and reproducible to maximize the odds of it getting fixed: it's really getting in the way of my attempt to use my mouse less 😄.


Edit: I ran the F1 > Help: Troubleshoot Issue command and followed the steps. I've added my results as a comment.

@VSCodeTriageBot
Copy link
Collaborator

Please diagnose the root cause of the issue by running the command F1 > Help: Troubleshoot Issue and following the instructions. Once you have done that, please update the issue with the results.

Happy Coding!

@VSCodeTriageBot VSCodeTriageBot added info-needed Issue requires more information from poster and removed ~confirmation-needed labels Jul 30, 2023
@matthew-e-brown
Copy link
Author

matthew-e-brown commented Jul 30, 2023

I ran the issue troubleshooter, including installing and replicating the issue inside of VS Code - Insiders. After all that, it gave me a popup to create a new issue (which, just like the first time I tried to report this, does absolutely nothing when you click "Create on GitHub"):

Issue Reporter window

...Do I need to add something from this window to this issue? What does the Triage Bot mean by "update the issue with the results?" Hopefully it'll recognize this reply as my update.

Like I said, clicking "Create on GitHub" does nothing, so here's all the stuff from those three checkboxes, manually copied over:

System information
Item Value
CPUs AMD Ryzen 7 5800X 8-Core Processor (16 x 3800)
GPU Status 2d_canvas: enabled canvas_oop_rasterization: disabled_off direct_rendering_display_compositor: disabled_off_ok gpu_compositing: enabled multiple_raster_threads: enabled_on opengl: enabled_on rasterization: enabled raw_draw: disabled_off_ok video_decode: enabled video_encode: enabled vulkan: disabled_off webgl: enabled webgl2: enabled webgpu: enabled
Load (avg)  
Memory (System) 31.91GB (19.69GB free)
Process Argv . --crash-reporter-id 6b04cac5-0b19-45dc-a31b-7e4561047beb
Screen Reader no
VM 0%
Enabled extensions (0) Extensions: none
A/B experiment info
vsliv368:30146709
vsreu685:30147344
python383:30185418
vspor879:30202332
vspor708:30202333
vspor363:30204092
vslsvsres303:30308271
vserr242cf:30382550
pythontb:30283811
vsjup518:30340749
pythonptprofiler:30281270
vshan820:30294714
vstes263:30335439
vscod805cf:30301675
binariesv615:30325510
bridge0708:30335490
bridge0723:30353136
vsaa593:30376534
pythonvs932:30410667
py29gd2263cf:30792227
vsclangdf:30486550
c4g48928:30535728
dsvsc012:30540252
pynewext54:30695312
azure-dev_surveyone:30548225
vsccc:30610678
3biah626:30602489
89544117:30613380
a9j8j154:30646983
showlangstatbar:30737416
vsctsb:30748421
03d35959:30757346
pythonfmttext:30731395
pythoncmv:30756943
fixshowwlkth:30771522
showindicator:30785052
pythongtdpath:30769146
i26e3531:30792625
gsofb:30797622
pythonnosmt12:30797651
e537b577:30795824
dsvsc013:30795093
dsvsc014:30797589

@bpasero bpasero added bug Issue identified by VS Code Team member as probable bug confirmed Issue has been confirmed by VS Code Team member grid-widget Grid view widget and removed info-needed Issue requires more information from poster labels Jul 31, 2023
@bpasero bpasero assigned joaomoreno and unassigned bpasero Jul 31, 2023
@bpasero
Copy link
Member

bpasero commented Jul 31, 2023

@matthew-e-brown kudos, excellent finding and very good issue description. I can reproduce the issue but it does not seem like a recent regression.

When debugging this, I can see how document.activeElement is lost as part of a grid operation. Easy steps:

  • log the document.activeElement before and after the call to this.orientation = orientation (here)
  • open 1 editor and select some text to see blue selection and focus
  • trigger the command workbench.action.newGroupRight
  • => 🐛 the activeElement becomes body
  • if you try again this will not happen because the grid seems to have remembered some state

I think the cause is a call to flipNode:

this.root = flipNode(this._root, orthogonalSize, size);

This probably removes and adds the grid from the DOM, thus focus is lost. I think this operation would have to remember and restore the document.activeElement to resolve this if the activeElement is part of a node that gets mutated like this.

Irrespective, I am actually wondering why the commands to create new editor groups would not focus the new group, that is maybe something I could change, but it might also break existing assumptions of our users...

@bpasero bpasero changed the title workbench.action.newGroup* commands do not handle focus correctly in some circumstances Some grid operations cause the activeElement to get lost Jul 31, 2023
@bpasero
Copy link
Member

bpasero commented Jul 31, 2023

Looking more into this, I see how I already accommodate for this issue in some parts of the code, for example here:

// Restore focus if we had it previously (we run this after gridWidget.removeView() is called
// because removing a view can mean to reparent it and thus focus would be removed otherwise)
if (restoreFocus) {
this._activeGroup.focus();
}

I am pushing a change to do the same for addView to workaround: #189292.

Leaving this still open to consider for grid.

@beto811 beto811 mentioned this issue Jul 31, 2023
@joaomoreno joaomoreno removed bug Issue identified by VS Code Team member as probable bug confirmed Issue has been confirmed by VS Code Team member grid-widget Grid view widget labels Aug 30, 2023
@joaomoreno joaomoreno assigned bpasero and unassigned joaomoreno Aug 30, 2023
@bpasero bpasero added bug Issue identified by VS Code Team member as probable bug workbench-editor-groups Issues related to editor groups labels Aug 31, 2023
@bpasero bpasero added this to the August 2023 milestone Aug 31, 2023
@bpasero
Copy link
Member

bpasero commented Aug 31, 2023

Decided to push a fix that the commands to create editor groups (New Editor Group to the...) really move focus into the new group, which I think is the intention anyways.

@VSCodeTriageBot VSCodeTriageBot added the unreleased Patch has not yet been released in VS Code Insiders label Aug 31, 2023
@VSCodeTriageBot VSCodeTriageBot added insiders-released Patch has been released in VS Code Insiders and removed unreleased Patch has not yet been released in VS Code Insiders labels Aug 31, 2023
@roblourens roblourens added the verified Verification succeeded label Aug 31, 2023
@bpasero bpasero reopened this Sep 1, 2023
@VSCodeTriageBot VSCodeTriageBot removed the insiders-released Patch has been released in VS Code Insiders label Sep 1, 2023
@bpasero
Copy link
Member

bpasero commented Sep 1, 2023

I am not 100% sure my fix is good. Forcing keyboard focus into the new group is a somewhat breaking change for muscle memory. For example, you might have keyboard focus in a tree/list, want to create a new empty group and then decide to open an item from the tree/list in it. This is no longer possible if we forcefully move focus to the new empty group.

@bpasero bpasero added the candidate Issue identified as probable candidate for fixing in the next release label Sep 1, 2023
@bpasero bpasero removed candidate Issue identified as probable candidate for fixing in the next release bug Issue identified by VS Code Team member as probable bug verified Verification succeeded labels Sep 1, 2023
@bpasero bpasero modified the milestones: August 2023, September 2023 Sep 1, 2023
@bpasero
Copy link
Member

bpasero commented Sep 1, 2023

@matthew-e-brown Looking more into this, this is actually working as intended (but I am thinking we can tweak this - more at the end) with 1 slight bug: in some cases, opening an editor group will result in an operation that removes the current editor group from the DOM and adds it back. If that editor group had keyboard focus, focus will move to the body element as a result. This does not always happen, but explains why you only see this sometimes.

The bug here is that we loose focus to the body element and fail to restore it. The actual consistent behaviour here is for the editor to always show a blinking cursor and have keyboard focus, even when the group to the side is active. The rationale behind this is that as a user you might want to stay focussed where you are and pick an editor to open in that group. Imagine you have keyboard focus in the explorer, opens an empty group to the side and then want to select a file from the explorer to open using keyboard. If we were to forcefully move focus to the empty group, you could no longer do this.

The somewhat weird part here is that you see an inactive group that has keyboard focus. You cannot really get into this state unless you follow the steps in this issue.

I wonder if the fix needs to be clever:

  • if focus is in the editor, opening a new group moves focus to the new group to prevent an editor having keyboard focus without being in the active group (and for example saving not working)
  • if focus is anywhere else, opening a new group will make it active but not focus it so that the scenario with the explorer still works

Does that sound reasonable?

@bpasero bpasero changed the title Some grid operations cause the activeElement to get lost Creating a new empty editor group can leave focus in inactive group Sep 1, 2023
@bpasero bpasero added the bug Issue identified by VS Code Team member as probable bug label Sep 1, 2023
@bpasero bpasero closed this as completed in 7545ee2 Sep 1, 2023
@VSCodeTriageBot VSCodeTriageBot added the unreleased Patch has not yet been released in VS Code Insiders label Sep 1, 2023
@matthew-e-brown
Copy link
Author

matthew-e-brown commented Sep 2, 2023

@bpasero Your fix seems perfectly reasonable :)

I was a bit confused what the difference was between "an editor having focus" and "an active editor" were, but your explorer example cleared it up. It feels a bit silly to admit, but I honestly never considered a scenario where you'd have focus in a non-editor panel and attempt to use these commands. Now that I'm thinking about it, it only makes sense that "below" would be "below the one you were using before you jumped to the explorer"---at which point, I think it would only make sense for focus to remain where it was.

Forcing keyboard focus into the new group is a somewhat breaking change for muscle memory.

I wonder if perhaps it would make sense to use two (sets of) commands;

  • workbench.editor.newGroupRight - creates a new editor group and makes it active, and also moves focus into it if focus was previously in the editor; otherwise focus remains where it was, but the new group does still become active for opening files into it from elsewhere.
  • workbench.editor.createGroupRight - creates a new editor group to the right, but leaves the current one as the active group and does not move focus into it.

Something like that would keep the default (moving focus into the new group) for most people, but allow those who have muscle memory for the other behaviour use a different command if it suits their workflow better. createGroupRight isn't the best name, but without renaming newGroupRight to something like newActiveGroupRight and breaking everybody's keybinds.json, I'm not sure of a better name 😛.

It's a pretty niche market to warrant adding a whole new command for, but... just a thought. 🙂

@bpasero
Copy link
Member

bpasero commented Sep 2, 2023

Yeah thanks for the suggestion but I think the way it works now is OK for the future. I cannot really envision that someone would want to create a new group without also activating it.

I actually think - because of this bug where focus would move to the body element - people have often not seen the underlying issue here because it felt like it behaves now in most cases.

@VSCodeTriageBot VSCodeTriageBot added insiders-released Patch has been released in VS Code Insiders and removed unreleased Patch has not yet been released in VS Code Insiders labels Sep 6, 2023
@bpasero bpasero added the author-verification-requested Issues potentially verifiable by issue author label Sep 8, 2023
@VSCodeTriageBot
Copy link
Collaborator

This bug has been fixed in the latest release of VS Code Insiders!

@matthew-e-brown, you can help us out by commenting /verified if things are now working as expected.

If things still don't seem right, please ensure you're on version e073d67 of Insiders (today's or later - you can use Help: About in the command palette to check), and leave a comment letting us know what isn't working as expected.

Happy Coding!

@roblourens roblourens added the verified Verification succeeded label Sep 27, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Oct 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
author-verification-requested Issues potentially verifiable by issue author bug Issue identified by VS Code Team member as probable bug insiders-released Patch has been released in VS Code Insiders verified Verification succeeded workbench-editor-groups Issues related to editor groups
Projects
None yet
Development

No branches or pull requests

5 participants