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

Undo of cell creation loses Notebook focus #99151

Closed
RMacfarlane opened this issue Jun 2, 2020 · 7 comments
Closed

Undo of cell creation loses Notebook focus #99151

RMacfarlane opened this issue Jun 2, 2020 · 7 comments
Assignees
Labels
insiders-released Patch has been released in VS Code Insiders notebook polish Cleanup and polish issue
Milestone

Comments

@RMacfarlane
Copy link
Contributor

Issue Type: Bug
Testing #98991

  1. Open a new .github-issues file and type in the first cell
  2. Add a new cell below this and type some text into it
  3. Undo repeatedly

The text from the new cell is removed, then the new cell itself, then undo stops working. I expected undo to remove text from the first cell at that point. If I focus the first cell and try to undo again, it does work.

VS Code version: Code - Insiders 1.46.0-insider (1bfa086, 2020-06-02T08:10:00.819Z)
OS version: Darwin x64 18.7.0

@alexdima
Copy link
Member

alexdima commented Jun 3, 2020

The root cause is hat the cell creation undo leads to the focused editor being disposed, which leads to the focus going to the body (and leaving the notebook). Once clicking anywhere inside the notebook, undo works again.

@alexdima alexdima changed the title Can't undo after removing cell until refocusing a cell Undo of focused cell creation loses focus to the body Jun 3, 2020
@alexdima alexdima changed the title Undo of focused cell creation loses focus to the body Undo of cell creation loses Notebook focus Jun 3, 2020
@alexdima alexdima removed their assignment Jun 3, 2020
@rebornix rebornix added bug Issue identified by VS Code Team member as probable bug notebook labels Jun 3, 2020
@rebornix rebornix modified the milestones: May 2020, June 2020 Jun 3, 2020
@rebornix
Copy link
Member

Now that the cells and notebook list share the same undo/redo stack, the issue should be gone now.

@RMacfarlane
Copy link
Contributor Author

I'm still seeing this in today's insiders

@RMacfarlane RMacfarlane reopened this Jul 1, 2020
@rebornix rebornix modified the milestones: June 2020, July 2020 Jul 6, 2020
@rebornix
Copy link
Member

rebornix commented Aug 3, 2020

@RMacfarlane are you still seeing this in Insiders now?

@rebornix rebornix added the info-needed Issue requires more information from poster label Aug 3, 2020
@RMacfarlane
Copy link
Contributor Author

Undo now works, but the focus indicator seems to get lost:
notebook_undo

@rebornix rebornix added polish Cleanup and polish issue and removed bug Issue identified by VS Code Team member as probable bug labels Aug 5, 2020
@rebornix rebornix removed the info-needed Issue requires more information from poster label Aug 5, 2020
@rebornix rebornix modified the milestones: July 2020, August 2020 Aug 5, 2020
rebornix added a commit that referenced this issue Aug 14, 2020
@rebornix
Copy link
Member

Pushed a fix which fixes this issue partially.

@roblourens
Copy link
Member

This happens when focus is in the editor, and when we delete the focused dom element, focus doesn't get reset to the list. We can't anything specifically for 'undo' because it happens in the model, but really we want to prevent this from happening for any cell modification. So I pushed a change to ensure that no list splice causes the list to lose DOM focus. Please take a look at the change @rebornix to check that this makes sense.

@github-actions github-actions bot locked and limited conversation to collaborators Oct 13, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
insiders-released Patch has been released in VS Code Insiders notebook polish Cleanup and polish issue
Projects
None yet
Development

No branches or pull requests

5 participants
@roblourens @rebornix @RMacfarlane @alexdima and others