From 1e5fb536d65550ad8fa0fefcec731645b2afc74a Mon Sep 17 00:00:00 2001 From: Mark Sujew Date: Tue, 6 Aug 2024 17:59:08 +0200 Subject: [PATCH] Fix notebook output scrolling and text rendering (#14016) --- packages/core/src/browser/style/index.css | 3 +- .../browser/contributions/cell-operations.ts | 7 +- .../notebook-actions-contribution.ts | 3 +- .../notebook-cell-actions-contribution.ts | 12 +-- .../src/browser/service/notebook-options.ts | 3 +- .../src/browser/service/notebook-service.ts | 8 +- packages/notebook/src/browser/style/index.css | 26 ++++--- .../browser/view/notebook-cell-list-view.tsx | 39 +++++----- .../renderers/cell-output-webview.tsx | 77 +++++++++++++++++++ .../renderers/output-webview-internal.ts | 61 ++++++++++----- 10 files changed, 180 insertions(+), 59 deletions(-) diff --git a/packages/core/src/browser/style/index.css b/packages/core/src/browser/style/index.css index e2ba596cb6491..137b5bfb7d4d5 100644 --- a/packages/core/src/browser/style/index.css +++ b/packages/core/src/browser/style/index.css @@ -239,7 +239,8 @@ blockquote { -o-user-select: none; } -:focus { +/* Since an iframe has its own focus tracking, we don't show focus on iframes */ +:focus:not(iframe) { outline-width: 1px; outline-style: solid; outline-offset: -1px; diff --git a/packages/notebook/src/browser/contributions/cell-operations.ts b/packages/notebook/src/browser/contributions/cell-operations.ts index 6ed1b4b5d7a7d..1953be0527319 100644 --- a/packages/notebook/src/browser/contributions/cell-operations.ts +++ b/packages/notebook/src/browser/contributions/cell-operations.ts @@ -26,6 +26,11 @@ export function changeCellType(notebookModel: NotebookModel, cell: NotebookCellM if (cell.cellKind === type) { return; } + if (type === CellKind.Markup) { + language = 'markdown'; + } else { + language ??= cell.language; + } notebookModel.applyEdits([{ editType: CellEditType.Replace, index: notebookModel.cells.indexOf(cell), @@ -33,7 +38,7 @@ export function changeCellType(notebookModel: NotebookModel, cell: NotebookCellM cells: [{ ...cell.getData(), cellKind: type, - language: language ?? cell.language + language }] }], true); } diff --git a/packages/notebook/src/browser/contributions/notebook-actions-contribution.ts b/packages/notebook/src/browser/contributions/notebook-actions-contribution.ts index 3fce82523ed1d..4f781114e12e7 100644 --- a/packages/notebook/src/browser/contributions/notebook-actions-contribution.ts +++ b/packages/notebook/src/browser/contributions/notebook-actions-contribution.ts @@ -133,8 +133,7 @@ export class NotebookActionsContribution implements CommandContribution, MenuCon let cellLanguage: string = 'markdown'; if (cellKind === CellKind.Code) { - const firstCodeCell = notebookModel.cells.find(cell => cell.cellKind === CellKind.Code); - cellLanguage = firstCodeCell?.language ?? 'plaintext'; + cellLanguage = this.notebookService.getCodeCellLanguage(notebookModel); } notebookModel.applyEdits([{ diff --git a/packages/notebook/src/browser/contributions/notebook-cell-actions-contribution.ts b/packages/notebook/src/browser/contributions/notebook-cell-actions-contribution.ts index 2775a4afab51c..69606906c6ba9 100644 --- a/packages/notebook/src/browser/contributions/notebook-cell-actions-contribution.ts +++ b/packages/notebook/src/browser/contributions/notebook-cell-actions-contribution.ts @@ -33,6 +33,7 @@ import { NotebookEditorWidgetService } from '../service/notebook-editor-widget-s import { NotebookCommands } from './notebook-actions-contribution'; import { changeCellType } from './cell-operations'; import { EditorLanguageQuickPickService } from '@theia/editor/lib/browser/editor-language-quick-pick-service'; +import { NotebookService } from '../service/notebook-service'; export namespace NotebookCellCommands { /** Parameters: notebookModel: NotebookModel | undefined, cell: NotebookCellModel */ @@ -118,7 +119,7 @@ export namespace NotebookCellCommands { export const TO_MARKDOWN_CELL_COMMAND = Command.toLocalizedCommand({ id: 'notebook.cell.changeToMarkdown', - label: 'Change Cell to Mardown' + label: 'Change Cell to Markdown' }); export const TOGGLE_CELL_OUTPUT = Command.toDefaultLocalizedCommand({ @@ -147,6 +148,9 @@ export class NotebookCellActionContribution implements MenuContribution, Command @inject(ContextKeyService) protected contextKeyService: ContextKeyService; + @inject(NotebookService) + protected notebookService: NotebookService; + @inject(NotebookExecutionService) protected notebookExecutionService: NotebookExecutionService; @@ -346,7 +350,7 @@ export class NotebookCellActionContribution implements MenuContribution, Command commands.registerCommand(NotebookCellCommands.INSERT_MARKDOWN_CELL_BELOW_COMMAND, insertCommand(CellKind.Markup, 'below')); commands.registerCommand(NotebookCellCommands.TO_CODE_CELL_COMMAND, this.editableCellCommandHandler((notebookModel, cell) => { - changeCellType(notebookModel, cell, CellKind.Code); + changeCellType(notebookModel, cell, CellKind.Code, this.notebookService.getCodeCellLanguage(notebookModel)); })); commands.registerCommand(NotebookCellCommands.TO_MARKDOWN_CELL_COMMAND, this.editableCellCommandHandler((notebookModel, cell) => { changeCellType(notebookModel, cell, CellKind.Markup); @@ -376,9 +380,7 @@ export class NotebookCellActionContribution implements MenuContribution, Command const isMarkdownCell = selectedCell.cellKind === CellKind.Markup; const isMarkdownLanguage = language.value.id === 'markdown'; if (isMarkdownLanguage) { - if (!isMarkdownCell) { - changeCellType(activeNotebook, selectedCell, CellKind.Markup, language.value.id); - } + changeCellType(activeNotebook, selectedCell, CellKind.Markup, language.value.id); } else { if (isMarkdownCell) { changeCellType(activeNotebook, selectedCell, CellKind.Code, language.value.id); diff --git a/packages/notebook/src/browser/service/notebook-options.ts b/packages/notebook/src/browser/service/notebook-options.ts index c642e97821e3e..2ecf59f02f31f 100644 --- a/packages/notebook/src/browser/service/notebook-options.ts +++ b/packages/notebook/src/browser/service/notebook-options.ts @@ -37,7 +37,7 @@ const notebookOutputOptionsRelevantPreferences = [ export interface NotebookOutputOptions { // readonly outputNodePadding: number; - // readonly outputNodeLeftPadding: number; + readonly outputNodeLeftPadding: number; // readonly previewNodePadding: number; // readonly markdownLeftMargin: number; // readonly leftMargin: number; @@ -95,6 +95,7 @@ export class NotebookOptionsService { fontSize, outputFontSize: outputFontSize, fontFamily: this.preferenceService.get('editor.fontFamily')!, + outputNodeLeftPadding: 8, outputFontFamily: this.getNotebookPreferenceWithDefault(NotebookPreferences.OUTPUT_FONT_FAMILY), outputLineHeight: this.computeOutputLineHeight(outputLineHeight, outputFontSize ?? fontSize), outputScrolling: this.getNotebookPreferenceWithDefault(NotebookPreferences.OUTPUT_SCROLLING)!, diff --git a/packages/notebook/src/browser/service/notebook-service.ts b/packages/notebook/src/browser/service/notebook-service.ts index 3707d9f656da0..6f9697e18e67b 100644 --- a/packages/notebook/src/browser/service/notebook-service.ts +++ b/packages/notebook/src/browser/service/notebook-service.ts @@ -17,7 +17,7 @@ import { Disposable, DisposableCollection, Emitter, Resource, URI } from '@theia/core'; import { inject, injectable } from '@theia/core/shared/inversify'; import { BinaryBuffer } from '@theia/core/lib/common/buffer'; -import { NotebookData, TransientOptions } from '../../common'; +import { CellKind, NotebookData, TransientOptions } from '../../common'; import { NotebookModel, NotebookModelFactory, NotebookModelProps } from '../view-model/notebook-model'; import { FileService } from '@theia/filesystem/lib/browser/file-service'; import { NotebookCellModel, NotebookCellModelFactory, NotebookCellModelProps } from '../view-model/notebook-cell-model'; @@ -206,4 +206,10 @@ export class NotebookService implements Disposable { return false; } } + + getCodeCellLanguage(model: NotebookModel): string { + const firstCodeCell = model.cells.find(cellModel => cellModel.cellKind === CellKind.Code); + const cellLanguage = firstCodeCell?.language ?? 'plaintext'; + return cellLanguage; + } } diff --git a/packages/notebook/src/browser/style/index.css b/packages/notebook/src/browser/style/index.css index 844629eb2b36d..333c792073b9d 100644 --- a/packages/notebook/src/browser/style/index.css +++ b/packages/notebook/src/browser/style/index.css @@ -31,6 +31,10 @@ margin: 10px 0px; } +.theia-notebook-cell:focus { + outline: none; +} + .theia-notebook-cell.draggable { cursor: grab; } @@ -85,7 +89,7 @@ /* Markdown cell edit mode */ .theia-notebook-cell-content:has(.theia-notebook-markdown-editor-container>.theia-notebook-cell-editor) { - margin-left: 37px; + margin-left: 36px; margin-right: var(--theia-notebook-cell-editor-margin-right); outline: 1px solid var(--theia-notebook-cellBorderColor); } @@ -107,7 +111,7 @@ width: calc(100% - 46px); flex: 1; outline: 1px solid var(--theia-notebook-cellBorderColor); - margin: 0px 10px; + margin: 0px 16px 0px 10px; } .theia-notebook-cell.focused .theia-notebook-cell-editor-container { @@ -288,7 +292,7 @@ .theia-notebook-cell-output-webview { padding: 5px 0px; - margin: 0px 10px; + margin: 0px 15px 0px 9px; width: 100%; } @@ -350,7 +354,7 @@ transform: translateY(calc(-100% - 10px)); } -.theia-notebook-find-widget.search-mode > * > *:nth-child(2) { +.theia-notebook-find-widget.search-mode>*>*:nth-child(2) { display: none; } @@ -379,9 +383,9 @@ align-items: center; } -.theia-notebook-find-widget-buttons-first > div, -.theia-notebook-find-widget-buttons-second > div { - margin-right: 4px; +.theia-notebook-find-widget-buttons-first>div, +.theia-notebook-find-widget-buttons-second>div { + margin-right: 4px; } .theia-notebook-find-widget-buttons-second { @@ -457,11 +461,11 @@ } mark.theia-find-match { - color: var(--theia-editor-findMatchHighlightForeground); - background-color: var(--theia-editor-findMatchHighlightBackground); + color: var(--theia-editor-findMatchHighlightForeground); + background-color: var(--theia-editor-findMatchHighlightBackground); } mark.theia-find-match.theia-find-match-selected { - color: var(--theia-editor-findMatchForeground); - background-color: var(--theia-editor-findMatchBackground); + color: var(--theia-editor-findMatchForeground); + background-color: var(--theia-editor-findMatchBackground); } diff --git a/packages/notebook/src/browser/view/notebook-cell-list-view.tsx b/packages/notebook/src/browser/view/notebook-cell-list-view.tsx index 41b719be09bcb..0c66a6f6531ce 100644 --- a/packages/notebook/src/browser/view/notebook-cell-list-view.tsx +++ b/packages/notebook/src/browser/view/notebook-cell-list-view.tsx @@ -48,6 +48,7 @@ export class NotebookCellListView extends React.Component = React.createRef(); constructor(props: CellListProps) { super(props); @@ -82,6 +83,24 @@ export class NotebookCellListView extends React.Component { + animationFrame().then(() => { + if (!this.cellListRef.current) { + return; + } + let hasCellFocus = false; + let hasFocus = false; + if (this.cellListRef.current.contains(document.activeElement)) { + if (this.props.notebookModel.selectedCell) { + hasCellFocus = true; + } + hasFocus = true; + } + this.props.notebookContext.changeCellFocus(hasCellFocus); + this.props.notebookContext.changeCellListFocus(hasFocus); + }); + })); } override componentWillUnmount(): void { @@ -89,24 +108,7 @@ export class NotebookCellListView extends React.Component { - this.toDispose.push(onDomEvent(document, 'focusin', () => { - animationFrame().then(() => { - let hasCellFocus = false; - let hasFocus = false; - if (ref?.contains(document.activeElement)) { - if (this.props.notebookModel.selectedCell) { - hasCellFocus = true; - } - hasFocus = true; - } - this.props.notebookContext.changeCellFocus(hasCellFocus); - this.props.notebookContext.changeCellListFocus(hasFocus); - }); - })); - } - }> + return
    {this.props.notebookModel.cells .map((cell, index) => @@ -129,6 +131,7 @@ export class NotebookCellListView extends React.Component this.onDragOver(e, cell)} onDrop={e => this.onDrop(e, index)} draggable={true} + tabIndex={-1} ref={ref => cell === this.state.selectedCell && this.state.scrollIntoView && ref?.scrollIntoView({ block: 'nearest' })}>
    diff --git a/packages/plugin-ext/src/main/browser/notebooks/renderers/cell-output-webview.tsx b/packages/plugin-ext/src/main/browser/notebooks/renderers/cell-output-webview.tsx index c2f7555e7bbf3..745c77dfb231e 100644 --- a/packages/plugin-ext/src/main/browser/notebooks/renderers/cell-output-webview.tsx +++ b/packages/plugin-ext/src/main/browser/notebooks/renderers/cell-output-webview.tsx @@ -50,7 +50,83 @@ export function createCellOutputWebviewContainer(ctx: interfaces.Container, cell return child; } +// Should be kept up-to-date with: +// https://github.com/microsoft/vscode/blob/main/src/vs/workbench/contrib/notebook/browser/view/renderers/webviewThemeMapping.ts +const mapping: ReadonlyMap = new Map([ + ['theme-font-family', 'vscode-font-family'], + ['theme-font-weight', 'vscode-font-weight'], + ['theme-font-size', 'vscode-font-size'], + ['theme-code-font-family', 'vscode-editor-font-family'], + ['theme-code-font-weight', 'vscode-editor-font-weight'], + ['theme-code-font-size', 'vscode-editor-font-size'], + ['theme-scrollbar-background', 'vscode-scrollbarSlider-background'], + ['theme-scrollbar-hover-background', 'vscode-scrollbarSlider-hoverBackground'], + ['theme-scrollbar-active-background', 'vscode-scrollbarSlider-activeBackground'], + ['theme-quote-background', 'vscode-textBlockQuote-background'], + ['theme-quote-border', 'vscode-textBlockQuote-border'], + ['theme-code-foreground', 'vscode-textPreformat-foreground'], + // Editor + ['theme-background', 'vscode-editor-background'], + ['theme-foreground', 'vscode-editor-foreground'], + ['theme-ui-foreground', 'vscode-foreground'], + ['theme-link', 'vscode-textLink-foreground'], + ['theme-link-active', 'vscode-textLink-activeForeground'], + // Buttons + ['theme-button-background', 'vscode-button-background'], + ['theme-button-hover-background', 'vscode-button-hoverBackground'], + ['theme-button-foreground', 'vscode-button-foreground'], + ['theme-button-secondary-background', 'vscode-button-secondaryBackground'], + ['theme-button-secondary-hover-background', 'vscode-button-secondaryHoverBackground'], + ['theme-button-secondary-foreground', 'vscode-button-secondaryForeground'], + ['theme-button-hover-foreground', 'vscode-button-foreground'], + ['theme-button-focus-foreground', 'vscode-button-foreground'], + ['theme-button-secondary-hover-foreground', 'vscode-button-secondaryForeground'], + ['theme-button-secondary-focus-foreground', 'vscode-button-secondaryForeground'], + // Inputs + ['theme-input-background', 'vscode-input-background'], + ['theme-input-foreground', 'vscode-input-foreground'], + ['theme-input-placeholder-foreground', 'vscode-input-placeholderForeground'], + ['theme-input-focus-border-color', 'vscode-focusBorder'], + // Menus + ['theme-menu-background', 'vscode-menu-background'], + ['theme-menu-foreground', 'vscode-menu-foreground'], + ['theme-menu-hover-background', 'vscode-menu-selectionBackground'], + ['theme-menu-focus-background', 'vscode-menu-selectionBackground'], + ['theme-menu-hover-foreground', 'vscode-menu-selectionForeground'], + ['theme-menu-focus-foreground', 'vscode-menu-selectionForeground'], + // Errors + ['theme-error-background', 'vscode-inputValidation-errorBackground'], + ['theme-error-foreground', 'vscode-foreground'], + ['theme-warning-background', 'vscode-inputValidation-warningBackground'], + ['theme-warning-foreground', 'vscode-foreground'], + ['theme-info-background', 'vscode-inputValidation-infoBackground'], + ['theme-info-foreground', 'vscode-foreground'], + // Notebook: + ['theme-notebook-output-background', 'vscode-notebook-outputContainerBackgroundColor'], + ['theme-notebook-output-border', 'vscode-notebook-outputContainerBorderColor'], + ['theme-notebook-cell-selected-background', 'vscode-notebook-selectedCellBackground'], + ['theme-notebook-symbol-highlight-background', 'vscode-notebook-symbolHighlightBackground'], + ['theme-notebook-diff-removed-background', 'vscode-diffEditor-removedTextBackground'], + ['theme-notebook-diff-inserted-background', 'vscode-diffEditor-insertedTextBackground'], +]); + +const constants: Record = { + 'theme-input-border-width': '1px', + 'theme-button-primary-hover-shadow': 'none', + 'theme-button-secondary-hover-shadow': 'none', + 'theme-input-border-color': 'transparent', +}; + export const DEFAULT_NOTEBOOK_OUTPUT_CSS = ` +:root { + ${Array.from(mapping.entries()).map(([key, value]) => `--${key}: var(--${value});`).join('\n')} + ${Object.entries(constants).map(([key, value]) => `--${key}: ${value};`).join('\n')} +} + +body { + padding: 0; +} + table { border-collapse: collapse; border-spacing: 0; @@ -323,6 +399,7 @@ export class CellOutputWebviewImpl implements CellOutputWebview, Disposable { protected generateStyles(): { [key: string]: string } { return { + 'notebook-output-node-left-padding': `${this.options.outputNodeLeftPadding}px`, 'notebook-cell-output-font-size': `${this.options.outputFontSize || this.options.fontSize}px`, 'notebook-cell-output-line-height': `${this.options.outputLineHeight}px`, 'notebook-cell-output-max-height': `${this.options.outputLineHeight * this.options.outputLineLimit}px`, diff --git a/packages/plugin-ext/src/main/browser/notebooks/renderers/output-webview-internal.ts b/packages/plugin-ext/src/main/browser/notebooks/renderers/output-webview-internal.ts index 7e01a55751954..ea348cc8d269b 100644 --- a/packages/plugin-ext/src/main/browser/notebooks/renderers/output-webview-internal.ts +++ b/packages/plugin-ext/src/main/browser/notebooks/renderers/output-webview-internal.ts @@ -146,14 +146,10 @@ export async function outputWebviewPreload(ctx: PreloadContext): Promise { renderer: Renderer; element: HTMLElement; + container: HTMLElement; constructor(output: webviewCommunication.Output, items: rendererApi.OutputItem[]) { - this.element = document.createElement('div'); - // padding for scrollbars - this.element.style.paddingBottom = '10px'; - this.element.style.paddingRight = '10px'; - this.element.id = output.id; - document.body.appendChild(this.element); + this.createHtmlElement(output.id); this.outputId = output.id; this.allItems = items; } @@ -172,6 +168,25 @@ export async function outputWebviewPreload(ctx: PreloadContext): Promise { this.renderer?.disposeOutputItem?.(this.renderedItem?.id); this.element.innerHTML = ''; } + + private createHtmlElement(id: string): void { + // Recreates the output container structure used in VS Code + this.container = document.createElement('div'); + this.container.id = 'container'; + this.container.classList.add('widgetarea'); + const cellContainer = document.createElement('div'); + cellContainer.classList.add('cell_container'); + cellContainer.id = id; + this.container.appendChild(cellContainer); + const outputContainer = document.createElement('div'); + outputContainer.classList.add('output-container'); + cellContainer.appendChild(outputContainer); + this.element = document.createElement('div'); + this.element.id = id; + this.element.classList.add('output'); + outputContainer.appendChild(this.element); + document.body.appendChild(this.container); + } } const outputs: Output[] = []; @@ -469,7 +484,7 @@ export async function outputWebviewPreload(ctx: PreloadContext): Promise { function clearOutput(output: Output): void { output.clear(); - output.element.remove(); + output.container.remove(); } function outputsChanged(changedEvent: webviewCommunication.OutputChangedMessage): void { @@ -504,16 +519,16 @@ export async function outputWebviewPreload(ctx: PreloadContext): Promise { } } - function scrollParent(event: WheelEvent): boolean { + function shouldHandleScroll(event: WheelEvent): boolean { for (let node = event.target as Node | null; node; node = node.parentNode) { if (!(node instanceof Element)) { - continue; + return false; } // scroll up if (event.deltaY < 0 && node.scrollTop > 0) { // there is still some content to scroll - return false; + return true; } // scroll down @@ -521,23 +536,31 @@ export async function outputWebviewPreload(ctx: PreloadContext): Promise { // per https://developer.mozilla.org/en-US/docs/Web/API/Element/scrollHeight // scrollTop is not rounded but scrollHeight and clientHeight are // so we need to check if the difference is less than some threshold - if (node.scrollHeight - node.scrollTop - node.clientHeight > 2) { - return false; + if (node.scrollHeight - node.scrollTop - node.clientHeight < 2) { + continue; + } + + // if the node is not scrollable, we can continue. We don't check the computed style always as it's expensive + if (window.getComputedStyle(node).overflowY === 'hidden' || window.getComputedStyle(node).overflowY === 'visible') { + continue; } + + return true; } } - return true; + return false; } const handleWheel = (event: WheelEvent & { wheelDeltaX?: number; wheelDeltaY?: number; wheelDelta?: number }) => { - if (scrollParent(event)) { - theia.postMessage({ - type: 'did-scroll-wheel', - deltaY: event.deltaY, - deltaX: event.deltaX, - }); + if (event.defaultPrevented || shouldHandleScroll(event)) { + return; } + theia.postMessage({ + type: 'did-scroll-wheel', + deltaY: event.deltaY, + deltaX: event.deltaX, + }); }; window.addEventListener('message', async rawEvent => {