Improve view mode keybinding for markdown cells #10069

dhruvisompura · 2025-10-21T23:58:59Z

Related to #9713 (the ask to have a mechanism to switch between modes already exists)

This PR is a suggestion for improving the edit to view mode experience for markdown cells.

While playing around with notebooks, I noticed that there was some inconsistency with how markdown cells behaved when using the keyboard vs the cell action bar that caused me a bit of confusion.

When you are in edit mode for a markdown cell, the cell action bar has a chevron icon button that hides the editor for markdown cells. I assumed this action was changing the cell from edit to view mode. The notebook also has a keybinding to exit edit mode that maps to Escape. This command does not do the same thing as the button in the cell action bar. When you press Escape in a markdown cell, focus moves to the markdown cell container and leaves the editor. The markdown editor view stays around with the preview underneath it.

It was unclear that this mismatch in behavior was intentional! I think another reason that the edit to view mode transition in markdown cells is confusing has to do with not having a great visual indicator for being in view mode. We show a focus indicator around the cell when a user is in edit mode, but there isn't anything like that for view mode.

I can close out this PR If we don't want to consolidate the behavior and need it to be different, but I figured I'd see what folks thought. I find the consolidation makes things less confusing.

It turns out we have 2 different commands that address exiting edit mode:

positronNotebook.cell.collapseMarkdownEditor -> command for action that shows up in markdown cell action bars
positronNotebook.cell.quitEdit -> command to exit edit mode for all cell types that is not contributed to any action bars

I think we still need two separate actions, one that allows us to contribute a button to a cell action bar, and a generic one that we can attach the keybinding to. I just think the behavior for both should be the same!

BEFORE

Screen.Recording.2025-10-21.at.4.29.49.PM.mov

AFTER

Screen.Recording.2025-10-21.at.4.26.51.PM.mov

Release Notes

New Features

N/A

Bug Fixes

N/A

QA Notes

@:positron-notebooks

github-actions · 2025-10-21T23:59:21Z

E2E Tests 🚀
This PR will run tests tagged with: @:critical @:positron-notebooks

^readme ^{valid tags}

dhruvisompura · 2025-10-21T23:59:34Z

src/vs/workbench/contrib/positronNotebook/browser/ContextKeysManager.ts

Just added a bunch of comments here that I thought were helpful

src/vs/workbench/contrib/positronNotebook/browser/notebookCells/NotebookMarkdownCell.tsx

dhruvisompura · 2025-10-22T00:06:53Z

src/vs/workbench/contrib/positronNotebook/browser/ContextKeysManager.ts

+/**
+ * POSITRON_NOTEBOOK_CELL_IS_EDITING looks to be a duplicate of POSITRON_NOTEBOOK_CELL_EDITOR_FOCUSED.
+ * The context key value is set but never actually read anywhere in the codebase. Consider removing it if it's not needed.
+ */
 export const POSITRON_NOTEBOOK_CELL_IS_EDITING = new RawContextKey<boolean>('positronNotebookCellIsEditing', false);


This key will probably get cleaned up when #9992 is tackled since that will require updating this file.

seeM

Thanks this is neat! Probably worth discussing this in today's sync.

I believe we were matching Jupyter Lab where the expectation is that you "run" a markdown cell (usually with Shift+Enter) to preview it.

VSCode and PyCharm both use your suggested approach of defaulting to preview and only editing via a user action: pressing enter on the cell or double clicking it.

I think this is more intuitive, and there's already presedence so it shouldn't be too surprising for users.

One issue, maybe to track separately: should it also automatically preview when you click a different cell while editing a markdown cell?

Screen.Recording.2025-10-22.at.09.04.14.mov

src/vs/workbench/contrib/positronNotebook/browser/ContextKeysManager.ts

seeM · 2025-10-22T06:51:35Z

src/vs/workbench/contrib/positronNotebook/browser/ContextKeysManager.ts

 export const POSITRON_NOTEBOOK_CELL_IS_ONLY = new RawContextKey<boolean>('positronNotebookCellIsOnly', false);
+/**
+ * Context key that is true when the markdown editor of a cell is open for editing.
+ * This does not necessarily mean the cell is focused, just that the editor is visible.


Where could focus be if not in the open editor? Doesn't your PR make it so that an open editor is always focused?

Am I correct that if the editor is open then the cell will definitely be "active" and "selected"?

Where could focus be if not in the open editor? Doesn't your PR make it so that an open editor is always focused?

Hah, yes you are right. I added this comment before I made the code change and forgot to change it 🙈. With this code change you can no longer have the editor open without it being focused! I will get this comment updated, thanks for catching that!

Am I correct that if the editor is open then the cell will definitely be "active" and "selected"?

That sounds right to me. We don't really differentiate between the two right now (really only matters for multi-select) but for the purposes of this PR when the editor is open the cell should be "active" and "selected".

src/vs/workbench/contrib/positronNotebook/browser/positronNotebook.contribution.ts

midleman · 2025-10-22T12:05:56Z

Oh! I like this. I've been struggling with this myself and this makes it way more obvious what is happening where. 💛

nstrayer

I like it. I do think this behavior is more obvious. (Why would someone ever want to have the editor open but not be editing markdown text?)

The pattern does beg the question of if we should make registerCellAction able to do multiple action bar items with non-overlapping when clauses. Could run the risk of burdening the function with too much work though...

dhruvisompura · 2025-10-22T16:03:05Z

@nstrayer

The pattern does beg the question of if we should make registerCellAction able to do multiple action bar items with non-overlapping when clauses. Could run the risk of burdening the function with too much work though...

This crossed my mind and I was wondering if there was a way to consolidate this into one action but it didn't seem worth to effort to do that right now.

I do think a private helper used by both actions would normally be the ideal route to signal these actions are linked together but in this case the helper seemed redundant because of the different parameters that registerCellAction and registerNotebookAction take.

Maybe a comment is what we need in the interim.

dhruvisompura · 2025-10-22T18:34:24Z

One issue, maybe to track separately: should it also automatically preview when you click a different cell while editing a markdown cell?

@seeM Ah, great catch! I think it would make sense for it to automatically switch to the preview when you click to a different cell from a usability perspective. I'll see if I can include the change here.

That being said, if others think that the existing behavior makes sense, I can also leave it. I can't really think of a reason as to why we wouldn't want to change this, but please let me know if there is one!

dhruvisompura · 2025-10-22T22:42:32Z

src/vs/workbench/contrib/positronNotebook/browser/selectionMachine.ts

 	[SelectionState.MultiSelection]: [
 		SelectionState.MultiSelection,   // Modify selection
 		SelectionState.SingleSelection,  // Reduce to single cell
+		SelectionState.EditingSelection, // Enter edit mode for single cell


While I was testing these changes I noticed that we didn't handle going from multi selection -> editing which I think we should be doing?

Repro Steps:

Select multiple cells via Shift + arrow keys

double-click on a markdown cell in view mode -> expectation is that all other cells get deselected and the cell that was double-clicked is selected for editing

Here's the before video:

invalid_selection_state_error.mov

Here's the after video:

Screen.Recording.2025-10-22.at.3.41.38.PM.mov

Nice catch!

dhruvisompura · 2025-10-22T22:50:40Z

src/vs/workbench/contrib/positronNotebook/browser/notebookCells/NotebookCellWrapper.tsx

+
+			// Close any open markdown cell editors when clicking on a different cell
+			// This must happen before any early returns to ensure markdown cells always close
+			const cells = notebookInstance.cells.get();
+			for (const otherCell of cells) {
+				if (otherCell !== cell && otherCell.isMarkdownCell() && otherCell.editorShown.get()) {
+					otherCell.toggleEditor();
+				}
+			}
+


Alrighty, this is the code change that makes it so a markdown cell in edit mode transitions to view mode when a user clicks on a different cell.

I don't love the implementation being here for this (pretty sure there's a better way to do this), will probably undo this and revisit later. Having the cell itself handle closing the editor onBlur makes more sense but its a bit more work than I was expecting.

cc @nstrayer

Alternatively in the layout effect above you could add the condition:

// If the editor is opened but we're not in editing mode, close the editor if (cell.isMarkdownCell() && cell.editorShown.get() && selectionStatus !== CellSelectionStatus.Editing) { cell.toggleEditor(); }

Which will let the markdown cell itself ensure that it closes its cell. As long as it's commented like this I dont think either one is better than the other though!

Moved this into the layoutEffect and I noticed the UI was a bit snappier when it came to hiding the editor. Thanks for pointing this out!

seeM

Looks great and ty for the additional improvements! The integration test is unrelated and I think going to be skipped

Co-authored-by: Wasim Lorgat <[email protected]> Signed-off-by: Dhruvi Sompura <[email protected]>

nstrayer

Looks great. This is definitely a quality of life improvement to shave away some annoying frictions.

dhruvisompura added 3 commits October 21, 2025 08:50

Localize empty markdown cell text

214f612

Add support for exiting markdown cell edit mode

8d64083

Add comments to notebook context keys

be4bf77

dhruvisompura commented Oct 21, 2025

View reviewed changes

dhruvisompura commented Oct 22, 2025

View reviewed changes

src/vs/workbench/contrib/positronNotebook/browser/notebookCells/NotebookMarkdownCell.tsx Show resolved Hide resolved

dhruvisompura commented Oct 22, 2025

View reviewed changes

dhruvisompura requested review from cindyytong, nstrayer and seeM October 22, 2025 00:07

seeM reviewed Oct 22, 2025

View reviewed changes

nstrayer previously approved these changes Oct 22, 2025

View reviewed changes

dhruvisompura added 2 commits October 22, 2025 08:49

Update context key comment

89cfec9

Remove unnecessary optional chaining operator

0b4f47f

dhruvisompura dismissed nstrayer’s stale review via 0b4f47f October 22, 2025 15:55

Add comments to commands

2eaa86f

dhruvisompura requested a review from seeM October 22, 2025 17:01

dhruvisompura added 2 commits October 22, 2025 13:47

Allow transitioning from multi select to edit via double click

7d3f34f

Hide markdown editors on blur

9d01f2f

dhruvisompura commented Oct 22, 2025

View reviewed changes

dhruvisompura requested a review from nstrayer October 22, 2025 22:50

seeM previously approved these changes Oct 23, 2025

View reviewed changes

Update raw cell comment

e70d320

Co-authored-by: Wasim Lorgat <[email protected]> Signed-off-by: Dhruvi Sompura <[email protected]>

dhruvisompura dismissed seeM’s stale review via e70d320 October 23, 2025 14:34

nstrayer previously approved these changes Oct 23, 2025

View reviewed changes

Move editor management logic into useEffect

ab11b0c

dhruvisompura dismissed nstrayer’s stale review via ab11b0c October 24, 2025 19:02

dhruvisompura requested a review from nstrayer October 24, 2025 19:03

dhruvisompura merged commit 209e82e into main Oct 24, 2025
12 checks passed

dhruvisompura deleted the bugfix/view-mode-keybinding branch October 24, 2025 21:15

github-actions bot locked and limited conversation to collaborators Oct 24, 2025

Improve view mode keybinding for markdown cells #10069

Improve view mode keybinding for markdown cells #10069

Uh oh!

Conversation

dhruvisompura commented Oct 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Release Notes

New Features

Bug Fixes

QA Notes

Uh oh!

github-actions bot commented Oct 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seeM left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

midleman commented Oct 22, 2025

Uh oh!

nstrayer left a comment

Choose a reason for hiding this comment

Uh oh!

dhruvisompura commented Oct 22, 2025

Uh oh!

dhruvisompura commented Oct 22, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seeM left a comment

Choose a reason for hiding this comment

Uh oh!

nstrayer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

dhruvisompura commented Oct 21, 2025 •

edited

Loading

github-actions bot commented Oct 21, 2025 •

edited

Loading