Backport PR #15386: Improve scrolling to heading (#15565)

* Backport PR #15386: Improve scrolling to heading

* 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 <5832902+krassowski@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
(cherry picked from commit 98d1e06)

* Change the `scrollHeadingToTop` setting to avoid behaviour change in patch release

and add a note about upcoming change in the next minor release

* Update snapshot to reflect tiny rendering change

---------

Co-authored-by: Frédéric Collonval <fcollonval@users.noreply.github.com>

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'
     );
   });

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

packages/notebook-extension/schema/tracker.json

@@ -739,6 +739,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. Starting with JupyterLab 4.1 this setting will be enabled by default.",
+      "type": "boolean",
+      "default": false
+    },
     "showEditorForReadOnlyMarkdown": {
       "title": "Show editor for read-only Markdown cells",
       "description": "Should an editor be shown for read-only markdown",

packages/notebook-extension/src/index.ts

@@ -871,16 +871,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
+          );
+        });
+    }
   }
 };
 

packages/notebook/src/toc.ts

@@ -485,6 +485,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
    *
@@ -521,24 +532,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) {
@@ -549,7 +573,7 @@ export class NotebookToCFactory extends TableOfContentsFactory<NotebookPanel> {
           });
         } else {
           widget.content
-            .scrollToItem(idx)
+            .scrollToItem(idx, this.scrollToTop ? 'start' : undefined)
             .then(() => {
               return onCellInViewport(cell);
             })
@@ -674,6 +698,8 @@ export class NotebookToCFactory extends TableOfContentsFactory<NotebookPanel> {
 
     return model;
   }
+
+  private _scrollToTop: boolean = true;
 }
 
 /**

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);
     }
   }