Fix scrolling on editor interactions when active cell is out of view in windowed mode

[krassowski]

References

Fixes #15594

Code changes

• Adds test cases for scrolling (should fail in the first commit)
• Fixes scrolling on editor interactions, taking advantage of new EditorView.scrollHandler:
  • old logic focused editors that were out of view in windowed mode upon any keypress; it was also used to ensure scrolling, but it had a bad side effect of scrolling on non-input and non-selection key presses, such as Alt; now the condition permitting this code path when active cell is out of view was removed in favour of dedicated scrollRequested logic (see below)
  • when Cell is out of viewport (in windowed mode) and the editor requests scrolling it does not attempt to scroll as this is futile (the editor does not know by how much to scroll the virtualized container); instead it emits a new scrollRequested signal,
  • the windowed notebook receives the scrollRequested signal, scrolls the cell into the view and then scrolls the editor as requested.

User-facing changes

Interacting with out-of view cells in windowed mode is more predictable and reflects the interaction model seen in non-windowed mode.

Backwards-incompatible changes

None

[jupyterlab-probot]

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

[krassowski]

Based on the investigation in #16007 (comment) it appears likely that a similar signal will be needed for stdin box to fix a related issue with cell not being scrolled to when user types into the stdin box. Maybe it is better to rename this signal to a generic scrollRequested?

[krassowski]

Change made in 438e829

[brichet]

Thanks @krassowski for making this PR.

I probably missed some use case, but I wonder if we need such complexity.
When the cell is 'hidden' because it's outside of the viewport, it is not really hidden but its height is set to 0px. However the editor keep a height, so the scroll request should work even if the cell is not in the viewport.

It seems to work with:

• the signal only sending a boolean (whether the cell is in the viewport or not), and that boolean used to call (or not) scrollToItem from the Notebook widget. No need for the function scrollWithinCell in the signal.
• _scrollHandlerExtension always returns true to scroll in the editor anyway.

Again I may have missed some use case where it wouldn't work as expected.

[krassowski]

However the editor keep a height, so the scroll request should work even if the cell is not in the viewport.

No, it will not work. CodeMirror will attempt to scroll the container in which it is embedded (see the code of scrollRectIntoView function in CM6 - it attempts to scroll each parent container to make the editor visible). It cannot scroll that container correctly because that container does not have a defined height in general in the windowing situation.

the signal only sending a boolean (whether the cell is in the viewport or not), and that boolean used to call (or not) scrollToItem from the Notebook widget. No need for the function scrollWithinCell in the signal.

So the thing I think you are missing is the fact that a cell may be longer than the viewport. In that case after we scrolled to a cell, we may have scrolled to a beginning of a cell. But if the user's cursor was at the end of the cell, the user will be confused.

Similarly, if the cell is longer than the height of the viewport but user types in the output (e.g. after running input() in a long code cell), we do not want to scroll just to the cell but to the cell output - which is tracked in #16007 but the design of scrollWithinCell() in this PR will enable it.

On some level, passing the exact location within cell to avoid scrolling twice would be ideal, but is not achievable with CodeMirror API as of today, and would be way more complex on the level of calculating the appropriate scroll delta.

_scrollHandlerExtension always returns true to scroll in the editor anyway.

I do not understand this point.

[krassowski]

Here are two screen recording showing when the logic for scrollWithinCell kicks in:

1. Pressing arrow key moves cursor: (a) if the cursor is in the top part of the cell no second scroll is made (b) if the cursor is in the bottom part immediately after scrolling to cell scrollWithinCell activates scrolling the cursor into view

2. With page down/up it is even more obvious: it first scrolls to cell and then within cell.

[brichet]

So the thing I think you are missing is the fact that a cell may be longer than the viewport. In that case after we scrolled to a cell, we may have scrolled to a beginning of a cell. But if the user's cursor was at the end of the cell, the user will be confused.

Thanks for the clarification and records, this was indeed the case I was missing.

[brichet]

Looks good to me, thanks @krassowski