Improve scrolling to heading (jupyterlab#15386)

* Fix scrolling on active heading * Add option to scroll heading to the top for notebook For editor it is not currently possible easily * Switch to command mode if we are jumping to an heading in md cell * Add tests * Add doc string to new attribute * Rebase follow-up * Fix linter * Update Playwright Snapshots * Revert incorrect updates --------- Co-authored-by: Michał Krassowski <[email protected]> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
krassowski · Dec 15, 2023 · 98d1e06 · 98d1e06
1 parent d89a1ae
commit 98d1e06
Show file tree

Hide file tree

Showing 7 changed files with 100 additions and 23 deletions.
diff --git a/galata/test/jupyterlab/toc-scrolling.test.ts b/galata/test/jupyterlab/toc-scrolling.test.ts
@@ -36,18 +36,40 @@ test.describe('Table of Contents scrolling to heading', () => {
   test('Notebook scrolls to heading', async ({ page }) => {
     await page.notebook.selectCells(0);
 
+    await page.keyboard.press('Enter');
+    await page.getByText('Mode: Edit').waitFor();
+
     await page.sidebar.getContentPanel(
       await page.sidebar.getTabPosition('table-of-contents')
     );
 
     await page
-      .locator('.jp-TableOfContents-tree >> text="the last one"')
-      .click({
-        button: 'left'
-      });
+      .locator('.jp-TableOfContents-tree')
+      .getByText('the last one')
+      .click();
+    // Should switch to command mode
+    await expect.soft(page.getByText('Mode: Command')).toBeVisible();
 
     const nbPanel = await page.notebook.getNotebookInPanel();
-    expect(await nbPanel.screenshot()).toMatchSnapshot(
+    expect
+      .soft(await nbPanel!.screenshot())
+      .toMatchSnapshot('scrolled-to-bottom-heading.png');
+
+    // Scroll up
+    const bbox = await nbPanel!.boundingBox();
+    await page.mouse.move(
+      bbox!.x + 0.5 * bbox!.width,
+      bbox!.y + 0.5 * bbox!.height
+    );
+    await page.mouse.wheel(0, -1200);
+    await page.waitForTimeout(100);
+
+    await page
+      .locator('.jp-TableOfContents-tree')
+      .getByText('the last one')
+      .click();
+
+    expect(await nbPanel!.screenshot()).toMatchSnapshot(
       'scrolled-to-bottom-heading.png'
     );
   });

diff --git a/...toc-scrolling.test.ts-snapshots/scrolled-to-bottom-heading-jupyterlab-linux.png b/...toc-scrolling.test.ts-snapshots/scrolled-to-bottom-heading-jupyterlab-linux.png
diff --git a/packages/codemirror/src/editor.ts b/packages/codemirror/src/editor.ts
@@ -395,15 +395,17 @@ export class CodeMirrorEditor implements CodeEditor.IEditor {
    *
    * #### Notes
    * This will remove any secondary cursors.
+   *
+   * @deprecated options bias and origin are not used
    */
   setCursorPosition(
     position: CodeEditor.IPosition,
-    options?: { bias?: number; origin?: string; scroll?: boolean }
+    options: { bias?: number; origin?: string; scroll?: boolean } = {}
   ): void {
     const offset = this.getOffsetAt(position);
     this.editor.dispatch({
       selection: { anchor: offset },
-      scrollIntoView: options?.scroll === false ? false : true
+      scrollIntoView: options.scroll === false ? false : true
     });
     // If the editor does not have focus, this cursor change
     // will get screened out in _onCursorsChanged(). Make an

diff --git a/packages/notebook-extension/schema/tracker.json b/packages/notebook-extension/schema/tracker.json
@@ -749,6 +749,12 @@
       "type": "number",
       "default": 50
     },
+    "scrollHeadingToTop": {
+      "title": "Scroll heading to top",
+      "description": "Whether to scroll heading to the document top when selecting it in the table of contents.",
+      "type": "boolean",
+      "default": true
+    },
     "showEditorForReadOnlyMarkdown": {
       "title": "Show editor for read-only Markdown cells",
       "description": "Should an editor be shown for read-only markdown",

diff --git a/packages/notebook-extension/src/index.ts b/packages/notebook-extension/src/index.ts
@@ -877,16 +877,35 @@ const tocPlugin: JupyterFrontEndPlugin<void> = {
   id: '@jupyterlab/notebook-extension:toc',
   description: 'Adds table of content capability to the notebooks',
   requires: [INotebookTracker, ITableOfContentsRegistry, ISanitizer],
-  optional: [IMarkdownParser],
+  optional: [IMarkdownParser, ISettingRegistry],
   autoStart: true,
   activate: (
     app: JupyterFrontEnd,
     tracker: INotebookTracker,
     tocRegistry: ITableOfContentsRegistry,
     sanitizer: IRenderMime.ISanitizer,
-    mdParser: IMarkdownParser | null
+    mdParser: IMarkdownParser | null,
+    settingRegistry: ISettingRegistry | null
   ): void => {
-    tocRegistry.add(new NotebookToCFactory(tracker, mdParser, sanitizer));
+    const nbTocFactory = new NotebookToCFactory(tracker, mdParser, sanitizer);
+    tocRegistry.add(nbTocFactory);
+    if (settingRegistry) {
+      Promise.all([app.restored, settingRegistry.load(trackerPlugin.id)])
+        .then(([_, setting]) => {
+          const onSettingsUpdate = () => {
+            nbTocFactory.scrollToTop =
+              (setting.composite['scrollHeadingToTop'] as boolean) ?? true;
+          };
+          onSettingsUpdate();
+          setting.changed.connect(onSettingsUpdate);
+        })
+        .catch(error => {
+          console.error(
+            'Failed to load notebook table of content settings.',
+            error
+          );
+        });
+    }
   }
 };
 

diff --git a/packages/notebook/src/toc.ts b/packages/notebook/src/toc.ts
@@ -546,6 +546,17 @@ export class NotebookToCFactory extends TableOfContentsFactory<NotebookPanel> {
     super(tracker);
   }
 
+  /**
+   * Whether to scroll the active heading to the top
+   * of the document or not.
+   */
+  get scrollToTop(): boolean {
+    return this._scrollToTop;
+  }
+  set scrollToTop(v: boolean) {
+    this._scrollToTop = v;
+  }
+
   /**
    * Create a new table of contents model for the widget
    *
@@ -582,24 +593,37 @@ export class NotebookToCFactory extends TableOfContentsFactory<NotebookPanel> {
           const el = headingToElement.get(heading);
 
           if (el) {
-            const widgetBox = widget.content.node.getBoundingClientRect();
-            const elementBox = el.getBoundingClientRect();
-
-            if (
-              elementBox.top > widgetBox.bottom ||
-              elementBox.bottom < widgetBox.top
-            ) {
-              el.scrollIntoView({ block: 'center' });
+            if (this.scrollToTop) {
+              el.scrollIntoView({ block: 'start' });
+            } else {
+              const widgetBox = widget.content.node.getBoundingClientRect();
+              const elementBox = el.getBoundingClientRect();
+
+              if (
+                elementBox.top > widgetBox.bottom ||
+                elementBox.bottom < widgetBox.top
+              ) {
+                el.scrollIntoView({ block: 'center' });
+              }
             }
           } else {
             console.debug('scrolling to heading: using fallback strategy');
-            await widget.content.scrollToItem(widget.content.activeCellIndex);
+            await widget.content.scrollToItem(
+              widget.content.activeCellIndex,
+              this.scrollToTop ? 'start' : undefined
+            );
           }
         };
 
         const cell = heading.cellRef;
         const cells = widget.content.widgets;
         const idx = cells.indexOf(cell);
+        // Switch to command mode to avoid entering Markdown cell in edit mode
+        // if the document was in edit mode
+        if (cell.model.type == 'markdown' && widget.content.mode != 'command') {
+          widget.content.mode = 'command';
+        }
+
         widget.content.activeCellIndex = idx;
 
         if (cell.inViewport) {
@@ -610,7 +634,7 @@ export class NotebookToCFactory extends TableOfContentsFactory<NotebookPanel> {
           });
         } else {
           widget.content
-            .scrollToItem(idx)
+            .scrollToItem(idx, this.scrollToTop ? 'start' : undefined)
             .then(() => {
               return onCellInViewport(cell);
             })
@@ -735,6 +759,8 @@ export class NotebookToCFactory extends TableOfContentsFactory<NotebookPanel> {
 
     return model;
   }
+
+  private _scrollToTop: boolean = true;
 }
 
 /**

diff --git a/packages/toc/src/model.ts b/packages/toc/src/model.ts
@@ -200,9 +200,11 @@ export abstract class TableOfContentsModel<
     if (this._activeHeading !== heading) {
       this._activeHeading = heading;
       this.stateChanged.emit();
-      if (emitSignal) {
-        this._activeHeadingChanged.emit(heading);
-      }
+    }
+
+    if (emitSignal) {
+      // Always emit the signal to trigger a scroll even if the value did not change
+      this._activeHeadingChanged.emit(this._activeHeading);
     }
   }