From 5ad0c7c6219df9b1d96df55a59cc80dfb7bf9948 Mon Sep 17 00:00:00 2001 From: Don Jayamanne Date: Fri, 19 Dec 2025 22:35:28 +1100 Subject: [PATCH] Fix Notebook cell layout issue when navigating cells (#284341) * Fix Notebook cell layout issue when navigating cells * Update src/vs/workbench/contrib/notebook/browser/view/cellParts/codeCell.ts Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- .../browser/view/cellParts/codeCell.ts | 17 +- .../test/browser/view/cellPart.test.ts | 187 ++++++++++++++++++ 2 files changed, 202 insertions(+), 2 deletions(-) diff --git a/src/vs/workbench/contrib/notebook/browser/view/cellParts/codeCell.ts b/src/vs/workbench/contrib/notebook/browser/view/cellParts/codeCell.ts index ba7f0e30b36..6bbcdf7eea2 100644 --- a/src/vs/workbench/contrib/notebook/browser/view/cellParts/codeCell.ts +++ b/src/vs/workbench/contrib/notebook/browser/view/cellParts/codeCell.ts @@ -766,6 +766,16 @@ export class CodeCellLayout { * crop content when the cell is partially visible (top or bottom clipped) or when content is * taller than the viewport. * + * Additional invariants: + * - Content height stability: once the layout has been initialized, scroll-driven re-layouts can + * observe transient Monaco content heights that reflect the current clipped layout (rather than + * the full input height). To keep the notebook list layout stable (avoiding overlapping cells + * while navigating/scrolling), we reuse the previously established content height for all reasons + * except `onDidContentSizeChange`. + * - Pointer-drag gating: while the user is holding the mouse button down in the editor (drag + * selection or potential drag selection), we avoid programmatic `editor.setScrollTop(...)` updates + * to prevent selection/scroll feedback loops and "stuck selection" behavior. + * * --------------------------------------------------------------------------- * SECTION 1. OVERALL NOTEBOOK VIEW (EACH CELL HAS AN 18px GAP ABOVE IT) * Legend: @@ -857,7 +867,10 @@ export class CodeCellLayout { const elementBottom = this.notebookEditor.getAbsoluteBottomOfElement(this.viewCell); const elementHeight = this.notebookEditor.getHeightOfElement(this.viewCell); const gotContentHeight = editor.getContentHeight(); - const editorContentHeight = Math.max((gotContentHeight === -1 ? editor.getLayoutInfo().height : gotContentHeight), gotContentHeight === -1 ? this._initialEditorDimension.height : gotContentHeight); // || this.calculatedEditorHeight || 0; + // If we've already calculated the editor content height once before and the contents haven't changed, use that. + const previouslyCalculatedHeight = this._initialized && reason !== 'onDidContentSizeChange' ? this._initialEditorDimension.height : undefined; + const fallbackEditorContentHeight = gotContentHeight === -1 ? Math.max(editor.getLayoutInfo().height, this._initialEditorDimension.height) : gotContentHeight; + const editorContentHeight = previouslyCalculatedHeight ?? fallbackEditorContentHeight; // || this.calculatedEditorHeight || 0; const editorBottom = elementTop + this.viewCell.layoutInfo.outputContainerOffset; const scrollBottom = this.notebookEditor.scrollBottom; // When loading, scrollBottom -scrollTop === 0; @@ -903,7 +916,7 @@ export class CodeCellLayout { } } - this._logService.debug(`${reason} (${this._editorVisibility})`); + this._logService.debug(`${reason} (${this._editorVisibility}, ${this._initialized})`); this._logService.debug(`=> Editor Top = ${top}px (editHeight = ${editorHeight}, editContentHeight: ${editorContentHeight})`); this._logService.debug(`=> eleTop = ${elementTop}, eleBottom = ${elementBottom}, eleHeight = ${elementHeight}`); this._logService.debug(`=> scrollTop = ${scrollTop}, top = ${top}`); diff --git a/src/vs/workbench/contrib/notebook/test/browser/view/cellPart.test.ts b/src/vs/workbench/contrib/notebook/test/browser/view/cellPart.test.ts index b7c5f2d5f85..9e5f87fc1a7 100644 --- a/src/vs/workbench/contrib/notebook/test/browser/view/cellPart.test.ts +++ b/src/vs/workbench/contrib/notebook/test/browser/view/cellPart.test.ts @@ -401,4 +401,191 @@ suite('CellPart', () => { ); } }); + + test('CodeCellLayout reuses content height after init', () => { + const LINE_HEIGHT = 21; + const STATUSBAR_HEIGHT = 22; + const CELL_TOP_MARGIN = 6; + const CELL_OUTLINE_WIDTH = 1; + const VIEWPORT_HEIGHT = 1000; + const ELEMENT_TOP = 100; + const ELEMENT_HEIGHT = 1200; + const OUTPUT_CONTAINER_OFFSET = 300; + const EDITOR_HEIGHT = 800; + + let contentHeight = 800; + const stubEditor = { + layoutCalls: [] as { width: number; height: number }[], + _lastScrollTopSet: -1, + getLayoutInfo: () => ({ width: 600, height: EDITOR_HEIGHT }), + getContentHeight: () => contentHeight, + layout: (dim: { width: number; height: number }) => { + stubEditor.layoutCalls.push(dim); + }, + setScrollTop: (v: number) => { + stubEditor._lastScrollTopSet = v; + }, + hasModel: () => true, + }; + const editorPart = { style: { top: '' } }; + const template: Partial = { + editor: stubEditor as unknown as ICodeEditor, + editorPart: editorPart as unknown as HTMLElement, + }; + const viewCell: Partial = { + isInputCollapsed: false, + layoutInfo: { + statusBarHeight: STATUSBAR_HEIGHT, + topMargin: CELL_TOP_MARGIN, + outlineWidth: CELL_OUTLINE_WIDTH, + editorHeight: EDITOR_HEIGHT, + outputContainerOffset: OUTPUT_CONTAINER_OFFSET, + editorWidth: 600, + } as unknown as CodeCellLayoutInfo, + }; + const notebookEditor = { + scrollTop: 0, + get scrollBottom() { + return VIEWPORT_HEIGHT; + }, + setScrollTop: (v: number) => { + /* no-op */ + }, + getLayoutInfo: () => ({ + fontInfo: { lineHeight: LINE_HEIGHT }, + height: VIEWPORT_HEIGHT, + stickyHeight: 0, + }), + getAbsoluteTopOfElement: () => ELEMENT_TOP, + getAbsoluteBottomOfElement: () => ELEMENT_TOP + OUTPUT_CONTAINER_OFFSET, + getHeightOfElement: () => ELEMENT_HEIGHT, + notebookOptions: { + getLayoutConfiguration: () => ({ editorTopPadding: 6 }), + }, + }; + + const layout = new CodeCellLayout( + true, + notebookEditor as unknown as IActiveNotebookEditorDelegate, + viewCell as CodeCellViewModel, + template as CodeCellRenderTemplate, + { debug: () => { } }, + { width: 600, height: EDITOR_HEIGHT } + ); + + layout.layoutEditor('init'); + assert.strictEqual(layout.editorVisibility, 'Full'); + assert.strictEqual(stubEditor.layoutCalls.at(-1)?.height, 800); + + // Simulate Monaco reporting a transient smaller content height on scroll. + contentHeight = 200; + layout.layoutEditor('nbDidScroll'); + assert.strictEqual(layout.editorVisibility, 'Full'); + assert.strictEqual( + stubEditor.layoutCalls.at(-1)?.height, + 800, + 'nbDidScroll should reuse the established content height' + ); + + layout.layoutEditor('onDidContentSizeChange'); + assert.strictEqual(layout.editorVisibility, 'Full'); + assert.strictEqual( + stubEditor.layoutCalls.at(-1)?.height, + 200, + 'onDidContentSizeChange should refresh the content height' + ); + }); + + test('CodeCellLayout does not programmatically scroll editor while pointer down', () => { + const LINE_HEIGHT = 21; + const CELL_TOP_MARGIN = 6; + const CELL_OUTLINE_WIDTH = 1; + const STATUSBAR_HEIGHT = 22; + const VIEWPORT_HEIGHT = 220; + const ELEMENT_TOP = 100; + const EDITOR_CONTENT_HEIGHT = 500; + const EDITOR_HEIGHT = EDITOR_CONTENT_HEIGHT; + const OUTPUT_CONTAINER_OFFSET = 600; + const ELEMENT_HEIGHT = 900; + const scrollTop = ELEMENT_TOP + CELL_TOP_MARGIN + 20; + const scrollBottom = scrollTop + VIEWPORT_HEIGHT; + + const stubEditor = { + _lastScrollTopSet: -1, + getLayoutInfo: () => ({ width: 600, height: EDITOR_HEIGHT }), + getContentHeight: () => EDITOR_CONTENT_HEIGHT, + layout: () => { + /* no-op */ + }, + setScrollTop: (v: number) => { + stubEditor._lastScrollTopSet = v; + }, + hasModel: () => true, + }; + const editorPart = { style: { top: '' } }; + const template: Partial = { + editor: stubEditor as unknown as ICodeEditor, + editorPart: editorPart as unknown as HTMLElement, + }; + const viewCell: Partial = { + isInputCollapsed: false, + layoutInfo: { + statusBarHeight: STATUSBAR_HEIGHT, + topMargin: CELL_TOP_MARGIN, + outlineWidth: CELL_OUTLINE_WIDTH, + editorHeight: EDITOR_HEIGHT, + outputContainerOffset: OUTPUT_CONTAINER_OFFSET, + } as unknown as CodeCellLayoutInfo, + }; + const notebookEditor = { + scrollTop, + get scrollBottom() { + return scrollBottom; + }, + setScrollTop: (v: number) => { + /* no-op */ + }, + getLayoutInfo: () => ({ + fontInfo: { lineHeight: LINE_HEIGHT }, + height: VIEWPORT_HEIGHT, + stickyHeight: 0, + }), + getAbsoluteTopOfElement: () => ELEMENT_TOP, + getAbsoluteBottomOfElement: () => ELEMENT_TOP + OUTPUT_CONTAINER_OFFSET, + getHeightOfElement: () => ELEMENT_HEIGHT, + notebookOptions: { + getLayoutConfiguration: () => ({ editorTopPadding: 6 }), + }, + }; + + const layout = new CodeCellLayout( + true, + notebookEditor as unknown as IActiveNotebookEditorDelegate, + viewCell as CodeCellViewModel, + template as CodeCellRenderTemplate, + { debug: () => { } }, + { width: 600, height: EDITOR_HEIGHT } + ); + + layout.layoutEditor('init'); + stubEditor._lastScrollTopSet = -1; + + layout.setPointerDown(true); + layout.layoutEditor('nbDidScroll'); + assert.strictEqual(layout.editorVisibility, 'Full (Small Viewport)'); + assert.strictEqual( + stubEditor._lastScrollTopSet, + -1, + 'Expected no programmatic editor.setScrollTop while pointer is down' + ); + + layout.setPointerDown(false); + layout.layoutEditor('nbDidScroll'); + assert.strictEqual(layout.editorVisibility, 'Full (Small Viewport)'); + assert.notStrictEqual( + stubEditor._lastScrollTopSet, + -1, + 'Expected editor.setScrollTop to resume once pointer is released' + ); + }); });