Fix scrolling on execution and switching notebook mode #15702

krassowski · 2024-01-26T15:01:19Z

References

Fixes #15693, solving two regressions introduced in 4.1 cycle:

fixes erratic scrolling after Shift+Enter in non-windowing modes (defer, none),
fixes too much scrolling after Ctrl+Enter in both windowing and non-windowing modes (Chromium-based browsers, Firefox not affected).

Code changes

corrects the node used for scrolling and padding computation - fixing a regression due to change in DOM structure introduced by Add virtual scrollbar component to windowed lists. #15533 (see the use of _outerElement)
fixes behaviour of setting command mode which was causing a spurious scroll before running cells due to focus triggering a browser scroll - regression introduced by Fix tab trap notebook cells #14115 (see preventScrollOnFocus)
the logic for scroll offset computation is now unified between windowing modes; when windowing is disabled the actual dimensions of the notebook and cell are used (see precomputed argument).
repeats scrolling if notebook area resizes within 100ms of scroll request in non-windowing mode; this ensures that if a very brief change in dimensions due to output of the previously active cell hiding and re-appearing (in Shift + Enter scenario) shifts the newly active cell out of the viewport, that cell will be scrolled to again. This was previously used (with 50ms) in windowed mode which is why Shift + Enter worked there.

The last point might be better solved by changing when scroll gets called when a cell is run. Currently the sequence is:

schedule cell execution (clears output)
switch to next cell
scroll if needed
return a promise which resolves when cell(s) executed

This does not work great when executing a cell which has long output in the following scenario:

schedule cell execution
switch to next cell
scroll if needed: decides no scrolling is needed
execution completes, output is populated
the newly added output shifts the next cell out of view (bad)

Therefore, it might be tempting to change this sequence to:

schedule cell execution
switch to next cell
await execution to complete (and output to be populated)
scroll to the output

But this would lead to surprising results if execution takes longer than ~100ms because a user could manually scroll away and be surprised that once the cell(s) completed execution, they are snapped back to the cell after the last executed cell.

Thus we would also need to cancel the pending scroll to next cell if user has already scrolled the view.

User-facing changes

First, spurious scrolling is eliminated if the following cell (for shift + enter) or current cell (for ctrl + enter) is significantly visible (see examples below)

Shift + Enter: short cell → long cell

4.1.0b2 (`defer` mode)	This PR (`defer` mode)	3.6.x (reference)

Note: this already worked in full mode since 4.0.9+ thanks to #15288 (see details).

4.1.0b2 (`full` mode)	This PR (`full` mode)	4.0.10 (`full` mode)

Ctrl + Enter: long cell (partially out of view), Chrome

4.1.0b2 (`defer` mode)	This PR (`defer` mode)	3.6.x (reference)

Note: this was also broken in full, and this PR also fixes it in the full mode (see details).

4.1.0b2 (`full` mode)	This PR (`full` mode)

Second, when execution with Shift + Enter invokes scrolling to next cell which was out of viewport, that cell will be positioned so that its top is in the middle of the viewport.

Backwards-incompatible changes

None

jupyterlab-probot · 2024-01-26T15:01:21Z

Thanks for making a pull request to jupyterlab!
To try out this branch on binder, follow this link:

MDN: https://developer.mozilla.org/en-US/docs/Web/API/Element/scrollTop

krassowski · 2024-01-27T14:36:18Z

packages/notebook/src/actions.tsx

+    // If a cell is outside of viewport and scrolling is needed,
+    // prefer to align the cell to the top viewport edge,
+    // rather than to the bottom (so that editor is visible)
+    void Private.handleRunState(notebook, state, true, 'start');


This is leading to the should scroll as little as possible on markdown rendering test failing, which highlights trade-offs of different approaches, compare:

with alternative:

and with:

It seems to me that we would want to restore the JupyterLab 3 behaviour here, maybe a variation of it (say "top edge in the 1/4 of the viewport from the top").

So the problem with the earlier implementation (before #15288) was not that auto alignment differed from Lab 3.6, just that "center" was implemented as "align the middles of the cell and the viewport". This obviously led to very annoying behaviour for cells longer than viewport as seemingly random part of the output was shown:

I will push a commit to fix that behaviour, which should simplify the code too.

krassowski · 2024-01-29T11:09:06Z

I now see that the fault of cell scrolling on switching to command mode affects many more actions than just running cells; for example pressing Esc in edit mode for a cell partially out of view would scroll, opening context menu over such a cell would scroll, etc. I will try to switch scroll on focus behaviour off by default.

krassowski · 2024-01-29T15:13:31Z

packages/ui-components/src/components/windowedlist.ts

+    if (this.viewModel.windowingActive) {
+      if (!this._itemsResizeObserver) {
+        this._itemsResizeObserver = new ResizeObserver(
+          this._onItemResize.bind(this)


False positive from testing code, resolved in 0870ffe

krassowski · 2024-01-29T15:13:39Z

packages/ui-components/src/components/windowedlist.ts

+      this._outerElement.addEventListener('scroll', this, passiveIfSupported);
+
+      this._scrollbarResizeObserver = new ResizeObserver(
+        this._adjustDimensionsForScrollbar.bind(this)


False positive from testing code, resolved in 0870ffe

krassowski · 2024-01-29T15:13:43Z

packages/ui-components/src/components/windowedlist.ts

+    } else {
+      if (!this._areaResizeObserver) {
+        this._areaResizeObserver = new ResizeObserver(
+          this._onAreaResize.bind(this)


False positive from testing code, resolved in 0870ffe

packages/ui-components/src/components/windowedlist.ts

krassowski · 2024-01-29T17:10:37Z

Note for reviewer: failures of Visual Regression Tests are unrelated:

Notebook Run › Run Notebook and capture cell outputs is flaky (toolbar blinking)
Notebook Toolbar › Paste cell and Table of Contents scrolling to heading › Notebook scrolls to heading will be fixed by Add scroll margin to headings for better alignment #15703 (also needs review)

gabalafou

I'm not done reviewing, but I thought I would go ahead and leave the review comments I have so far.

I'm not sure I feel confident about my ability to review the changes to the windowed list classes.

galata/playwright.config.js

gabalafou · 2024-01-30T04:05:06Z

packages/ui-components/src/components/windowedlist.ts

+   *           If the item is less than one viewport away, scroll so that it becomes fully visible (following the `auto` heuristics).
+   *           If the item is more than one viewport away, scroll so that it is centered within the viewport (`center` if smaller than viewport, `top-center` otherwise).
+   *   center - Align the middle of the item with the middle of the viewport (it only works well for items smaller than the viewport).
+   *   top-center - Align the top of the item with the middle of the viewport (works well for items larger than the viewport).


packages/notebook/src/actions.tsx

gabalafou · 2024-01-30T04:27:10Z

packages/notebook/src/actions.tsx

+    // If a cell is outside of viewport and scrolling is needed,
+    // prefer to align the cell to the top viewport edge,
+    // rather than to the bottom (so that editor is visible)
+    void Private.handleRunState(notebook, state, true, 'center');


Why 'center' and not 'top-center'? I thought the idea was to keep the cell that we just left in view, which we cannot guarantee if we scroll the center of the next cell into the center of the viewport.

Great question! This is because it only applies to auto strategy and handleRunState uses the smart strategy, which will:

(a) not scroll if significantly visible, otherwise

(b) scroll using auto heuristics if less than one viewport away
this point is important because if an item is less than one viewport and not significantly visible it implies that it fits in the viewport hence we can apply center rather than top-center.

(c) choose between top-center and center depending on item size.

So in (b) we can use center over top-center; but should we? For small cells it is nicer, and in a rare case of item size being equal to viewport we loose past context (future context would not be visible either way), so maybe we could choose between center and top-center depending on item size, but I did not want to make the logic any more complicated; in fact I was considering removing this fallback to auto heuristic here but was also cautious not to modify the logic too much in case if downstream already depends on smart falling back to auto in this circumstance. In either case, yes we could also use top-center here. What do you think?

This also highlights that the comment above the line you commented on is outdated because I had not updated it since the previous solution was in place. I will update it now.

The comment was updated in 5b02962

I can't help but wondering if the added code complexity outweighs the UX benefit. Seems to me that if we don't introduce the alignPreference parameter then the consequence is that in the scenario labeled (b), in which the cell fits within the viewport and is less than one viewport away, then the cell will be scrolled to in such a way that either its top edge will align with the top edge of the scrollable container or its bottom edge with the bottom edge. I'm probably missing something but I don't see how that's significantly worse than scrolling the cell's top edge (or center) to the center of the scroll-container.

gabalafou

I can see that a lot of thought, care, and hard work has gone into this PR, so I hesitate to ask this question, but I was wondering if it would make sense to split up this PR. I may be misunderstanding the code, but it seems that only some of the code changes in this PR strictly address fixing the regressions that caused the bug reported in issue #15693. The other code changes seem like enhancements. Would it make sense to separate the two?

Marking request changes for my question about the test code.

galata/test/jupyterlab/notebook-scroll.test.ts

galata/test/jupyterlab/windowed-notebook.test.ts

packages/notebook/src/actions.tsx

galata/test/jupyterlab/utils.ts

gabalafou · 2024-01-31T04:12:18Z

packages/notebook/src/actions.tsx

+    // If a cell is outside of viewport and scrolling is needed,
+    // prefer to align the cell to the top viewport edge,
+    // rather than to the bottom (so that editor is visible)
+    void Private.handleRunState(notebook, state, true, 'center');


I can't help but wondering if the added code complexity outweighs the UX benefit. Seems to me that if we don't introduce the alignPreference parameter then the consequence is that in the scenario labeled (b), in which the cell fits within the viewport and is less than one viewport away, then the cell will be scrolled to in such a way that either its top edge will align with the top edge of the scrollable container or its bottom edge with the bottom edge. I'm probably missing something but I don't see how that's significantly worse than scrolling the cell's top edge (or center) to the center of the scroll-container.

krassowski · 2024-01-31T08:00:47Z

I may be misunderstanding the code, but it seems that only some of the code changes in this PR strictly address fixing the regressions that caused the bug reported in issue #15693. The other code changes seem like enhancements. Would it make sense to separate the two?

No, this is incorrect. As soon as we change preventScrollOnFocus we need to perform manual scrolling because browser no longer handles it, hence we need to:

update the logic for scrolling implemented in windowed list module to align with user expectations (which has a nice side effect of making it consistent between windowed and non-windowed mode)
fix the regression of wrong node being scrolled (_outerElement) because otherwise the above does nor work in defer/none mode
if we stopped at this we would then have shift + enter in inconsistent state as a result of this PR hence the update to scrollback logic

I do not see any changes in this PR which are not directly addressing the erractic jumps reported in #15693 (and while demonstrated on one example - ctrl + enter, also appearing in scenario of shift + enter as described in the issue in detail in course of investigation; further, given the above resolution approach there is not even a way to fix ctrl + enter without fixing shift + enter).

gabalafou

Okay thanks for the clarification.

Apart from some reservations I have about increasing the complexity of the code with the alignPreference parameter, this code looks good to me!

However, I still think there's something amiss with those test files. Could you diff them to make sure?

gabalafou · 2024-01-31T17:01:06Z

galata/test/jupyterlab/notebook-scroll.test.ts

@@ -131,15 +124,53 @@ test.describe('Notebook scroll on execution', () => {
    // The third cell should be positioned at the bottom, revealing between 10 to 20% of its content.
    await expect(thirdCellLocator).toBeInViewport({ ratio: 0.1 });
    await expect(thirdCellLocator).not.toBeInViewport({ ratio: 0.2 });
+    await expect(thirdCellLocator).not.toBeInViewport({ ratio: 0.2 });


This line is duplicated

Great catch, thanks! It now looks good for me:

20c20 < windowingMode: 'full' --- > windowingMode: 'none' 25c25 < test.describe('Notebook scroll on navigation (with windowing)', () => { --- > test.describe('Notebook scroll on navigation (no windowing)', () => { 65c65 < test.describe('Notebook scroll on execution (with windowing)', () => { --- > test.describe('Notebook scroll on execution (no windowing)', () => { 172c172 < test.describe('Notebook scroll over long outputs (with windowing)', () => { --- > test.describe('Notebook scroll over long outputs (no windowing)', () => {

krassowski · 2024-01-31T18:50:09Z

@gabalafou does it look good to you now?

gabalafou

Okay, I'm done reviewing and I approve with one reservation, which is that I'm still not sure whether the UX benefit of alignPreference is worth the added complexity (not sure if you saw my comment asking about that). But this is not blocking for me. I also left some comments in this last round of review, but these are also not blocking.

Great job unifying the windowing and no-windowing UX!

packages/ui-components/src/components/windowedlist.ts

gabalafou · 2024-01-31T23:27:21Z

packages/ui-components/src/components/windowedlist.ts

+   * Handle viewport area resize.
+   */
+  private _onAreaResize(_entries: ResizeObserverEntry[]): void {
+    this._scrollBackToItemOnResize();


Just checking my own understanding here.

Is this line more for code symmetry and to cover edge cases than to cover a commonly occurring scenario? I mean, it's generally unlikely that an area resize will happen within 100 ms of the app scrolling to some item, right?

No, it is actually quite common when user executes a cell which produces output and advances to the next cell. See:

Ah, for some reason I thought the area resize observer was observing the outer (scrollable) element (for e.g. window resize). Thanks for the clarification.

packages/notebook/src/actions.tsx

gabalafou · 2024-01-31T23:45:33Z

packages/notebook/src/toc.ts

@@ -610,7 +610,8 @@ export class NotebookToCFactory extends TableOfContentsFactory<NotebookPanel> {
            console.debug('scrolling to heading: using fallback strategy');
            await widget.content.scrollToItem(
              widget.content.activeCellIndex,
-              this.scrollToTop ? 'start' : undefined
+              this.scrollToTop ? 'start' : undefined,
+              0


Hmm, I never asked about this change (margin=0) and I'm not sure I understand why it's here

Good question. First, this is the fallback strategy which is unlikely to be used. Before this PR it was broken in defer mode because of wrong node being used for scrolling further down the line. Currently both defer and full mode will work, invoking a logic which considers the margin. Now, the default visibility margin for scrolling on action is 25%. But when user uses ToC they often want to find the heading and if it was partially revealed and the clicked either way it means that they expect actions anyways (hence it should be zero).

gabalafou · 2024-01-31T23:51:22Z

packages/testing/src/jest-shim.ts

+  }
+}
+
+window.ResizeObserver = ResizeObserverMock;


Also checking my understanding here. If the ResizeObserver is basically just a no-op mock, that means that none of the tests are hitting code paths that would lead to _scrollBackToItemOnResize() being called, correct?

that means that none of the tests are hitting code paths

More precisely none of the unit tests get to call _scrollBackToItemOnResize(), but the integration tests do. In fact if one was to remove it the visual tests would fail.

Co-authored-by: gabalafou <gabriel@fouasnon.com>

for more information, see https://pre-commit.ci

krassowski · 2024-02-01T10:42:03Z

Thank you for the review @gabalafou!

with one reservation, which is that I'm still not sure whether the UX benefit of alignPreference is worth the added complexity (not sure if you saw my comment asking about that).

I just want to acknowledge that I saw your comment and take responsibility of dealing with the added complexity arising from having this parameter if it ever becomes a problem (and I believe this is worth the - small - UX benefit).

krassowski added 2 commits January 25, 2024 14:44

Mirror the tests for scroll for non-windowed notebook

b89a99d

Fix scrolling on Shift+Enter in defer and Ctrl+Enter in both modes.

56c0d23

krassowski added this to the 4.1.0 milestone Jan 26, 2024

krassowski added the bug label Jan 26, 2024

github-actions bot added pkg:notebook tag:Testing pkg:ui-components labels Jan 26, 2024

github-actions bot assigned krassowski Jan 26, 2024

krassowski changed the title ~~Fix scroll in non windowed mode~~ Fix scrolling on execution Jan 26, 2024

krassowski added 3 commits January 26, 2024 17:05

Fix scrolling to anchor test in non-windowed mode

4b4fb3f

Add a shim for scrollTo not yet implemented in jsdom

7ffb628

MDN: https://developer.mozilla.org/en-US/docs/Web/API/Element/scrollTop

Add test for running in place

771413f

krassowski commented Jan 27, 2024

View reviewed changes

krassowski added 2 commits January 27, 2024 21:21

Align the top of large cells with the middle of viewport

807ec33

Do not scroll if fully visible in smart logic

f7da641

krassowski added 3 commits January 29, 2024 12:11

Always prevent scroll on focus when changing cell mode

57a1d25

Improve tests for scrolling

24ca9f1

Add a mock for ResizeObserver

133a55c

krassowski force-pushed the fix-scroll-in-non-windowed-mode branch from 6b37a7c to 133a55c Compare January 29, 2024 12:37

github-advanced-security bot found potential problems Jan 29, 2024

View reviewed changes

packages/ui-components/src/components/windowedlist.ts Fixed Show fixed Hide fixed

packages/ui-components/src/components/windowedlist.ts Fixed Show fixed Hide fixed

packages/ui-components/src/components/windowedlist.ts Fixed Show fixed Hide fixed

krassowski added 4 commits January 29, 2024 13:27

Better mock to silence code scanning warnings

0870ffe

Update markdown rendering test

6636ca7

Fix typo in splitCell description

f0947b1

Catch internally re-generated promise

f8acac5

krassowski marked this pull request as ready for review January 29, 2024 15:44

krassowski requested a review from gabalafou January 29, 2024 15:46

krassowski changed the title ~~Fix scrolling on execution~~ Fix scrolling on execution and switching notebook mode Jan 29, 2024

gabalafou reviewed Jan 30, 2024

View reviewed changes

krassowski added 2 commits January 30, 2024 09:24

Merge branch 'main' into fix-scroll-in-non-windowed-mode

22d657b

Update a comment following review

5b02962

gabalafou requested changes Jan 31, 2024

View reviewed changes

krassowski added 4 commits January 31, 2024 07:30

Correct mode in test file

3552ca2

Review: rename utility test function

841bc19

Review: cellBBox → cellBbox

178b59b

Review: remove scroll arg from handleRunState

39ce21d

krassowski requested a review from gabalafou January 31, 2024 08:04

gabalafou requested changes Jan 31, 2024

View reviewed changes

Fix duplicated line

ee8b741

krassowski requested a review from gabalafou January 31, 2024 18:50

gabalafou approved these changes Jan 31, 2024

View reviewed changes

krassowski and others added 2 commits February 1, 2024 09:17

Apply suggestions on comments/documentation

7364c33

Co-authored-by: gabalafou <gabriel@fouasnon.com>

[pre-commit.ci] auto fixes from pre-commit.com hooks

0520c17

for more information, see https://pre-commit.ci

krassowski merged commit 5ba76a8 into jupyterlab:main Feb 1, 2024
78 of 79 checks passed

krassowski mentioned this pull request Feb 5, 2024

Audit scrolling after a cell operation #9283

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix scrolling on execution and switching notebook mode #15702

Fix scrolling on execution and switching notebook mode #15702

krassowski commented Jan 26, 2024 •

edited

jupyterlab-probot bot commented Jan 26, 2024

krassowski Jan 27, 2024

krassowski Jan 27, 2024

krassowski Jan 27, 2024 •

edited

krassowski commented Jan 29, 2024

krassowski Jan 29, 2024

krassowski Jan 29, 2024

krassowski Jan 29, 2024

krassowski commented Jan 29, 2024

gabalafou left a comment

gabalafou Jan 30, 2024

gabalafou Jan 30, 2024

krassowski Jan 30, 2024

krassowski Jan 30, 2024

gabalafou Jan 31, 2024

gabalafou left a comment

gabalafou Jan 31, 2024

krassowski commented Jan 31, 2024 •

edited

gabalafou left a comment

gabalafou Jan 31, 2024

krassowski Jan 31, 2024

krassowski commented Jan 31, 2024

gabalafou left a comment

gabalafou Jan 31, 2024

krassowski Feb 1, 2024 •

edited

gabalafou Feb 5, 2024

gabalafou Jan 31, 2024

krassowski Feb 1, 2024 •

edited

gabalafou Jan 31, 2024

krassowski Feb 1, 2024

krassowski commented Feb 1, 2024

Fix scrolling on execution and switching notebook mode #15702

Fix scrolling on execution and switching notebook mode #15702

Conversation

krassowski commented Jan 26, 2024 • edited

References

Code changes

User-facing changes

Shift + Enter: short cell → long cell

Ctrl + Enter: long cell (partially out of view), Chrome

Backwards-incompatible changes

jupyterlab-probot bot commented Jan 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krassowski Jan 27, 2024 • edited

Choose a reason for hiding this comment

krassowski commented Jan 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krassowski commented Jan 29, 2024

gabalafou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabalafou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krassowski commented Jan 31, 2024 • edited

gabalafou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krassowski commented Jan 31, 2024

gabalafou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krassowski Feb 1, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krassowski Feb 1, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krassowski commented Feb 1, 2024

krassowski commented Jan 26, 2024 •

edited

krassowski Jan 27, 2024 •

edited

krassowski commented Jan 31, 2024 •

edited

krassowski Feb 1, 2024 •

edited

krassowski Feb 1, 2024 •

edited