Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve scrolling to heading #15386

Merged
merged 10 commits into from
Dec 15, 2023

Conversation

fcollonval
Copy link
Member

References

Fixes #15228
Fixes #15215 (@krassowski - would you mind confirm this I'm unsure)
Fixes #14684
Fixes #14591

Code changes

  • Emit onActiveHeadingChanged in toc model even when the value is not changing
  • Add a new attribute scrollToTop to the notebook ToC factory that is true by default but allows to configure the heading navigation scroll behavior
  • Switch to command mode when in edit mode and jumping to a heading in a md cell

User-facing changes

By default, jumping to a heading from the toc will not make the markdown cell turn in edit mode and will scroll it to the top of the view port.

Backwards-incompatible changes

None

@fcollonval fcollonval added the bug label Nov 10, 2023
Copy link

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

@krassowski
Copy link
Member

I guess this could be backported to 4.0.x but without the new setting?

@krassowski krassowski added this to the 4.0.x milestone Nov 12, 2023
@fcollonval
Copy link
Member Author

I guess this could be backported to 4.0.x but without the new setting?

I was thinking of backported it with the setting but changing the default value as lots of user consider scroll to top a better behavior.

Copy link
Member

@krassowski krassowski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if this fixes #15215 because that is not trivial to reproduce, but I did find a way to break this PR:

broke

packages/notebook-extension/src/index.ts Outdated Show resolved Hide resolved
galata/test/jupyterlab/toc-scrolling.test.ts Show resolved Hide resolved
@krassowski
Copy link
Member

Sorry accidentally clicked submit too early, here is the traceback:

DOMException: Failed to execute 'removeChild' on 'Node': The node to be removed is not a child of this node.
    at NotebookWindowedLayout.detachWidget (http://127.0.0.1:8889/static/lab/packages_notebook_lib_index_js.a0899e5b2c84c0887c94.js:9919:38)
    at NotebookWindowedLayout.removeWidget (http://127.0.0.1:8889/static/lab/packages_notebook_lib_index_js.a0899e5b2c84c0887c94.js:9806:18)
    at NotebookWindowedLayout.onChildRemoved (http://127.0.0.1:8889/static/lab/jlab_core.ac50a29a806e5e41bc1f.js:13723:14)
    at NotebookWindowedLayout.onChildRemoved (http://127.0.0.1:8889/static/lab/packages_notebook_lib_index_js.a0899e5b2c84c0887c94.js:9997:15)
    at NotebookWindowedLayout.processParentMessage (http://127.0.0.1:8889/static/lab/jlab_core.ac50a29a806e5e41bc1f.js:13522:22)
    at Notebook.notifyLayout (http://127.0.0.1:8889/static/lab/jlab_core.ac50a29a806e5e41bc1f.js:12938:26)
    at Notebook.processMessage (http://127.0.0.1:8889/static/lab/jlab_core.ac50a29a806e5e41bc1f.js:12918:22)
    at invokeHandler (http://127.0.0.1:8889/static/lab/jlab_core.ac50a29a806e5e41bc1f.js:9381:21)
    at Object.sendMessage (http://127.0.0.1:8889/static/lab/jlab_core.ac50a29a806e5e41bc1f.js:9135:13)
    at set parent [as parent] (http://127.0.0.1:8889/static/lab/jlab_core.ac50a29a806e5e41bc1f.js:12581:72)
exceptionHandler @ index.es6.js:377

followed by:

windowedlist.ts:1126 Uncaught Error: Inconsistent dataset index
    at Notebook._update (windowedlist.ts:1126:15)
    at windowedlist.ts:1005:16

@krassowski
Copy link
Member

The error above is unrelated (it also happens in 4.0.8 and 4.1.0a3), I tracked it down to "Revert" breaking windowed notebooks, here is the issue: #15415.

@krassowski
Copy link
Member

The failing test appears to reflect change in the behaviour and likely needs to be updated (or change in test settings):

Expected Actual
Screenshot from 2023-11-24 14-01-38 Screenshot from 2023-11-24 14-01-35

@fcollonval
Copy link
Member Author

bot please update the galata snapshots

@krassowski
Copy link
Member

krassowski commented Dec 13, 2023

bot please update galata snapshots

Copy link
Contributor

Galata snapshots updated.

Copy link
Member

@krassowski krassowski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @fcollonval!

@krassowski krassowski merged commit 98d1e06 into jupyterlab:main Dec 15, 2023
77 of 79 checks passed
@krassowski
Copy link
Member

@meeseeksdev please backport to 4.0.x

Copy link

lumberbot-app bot commented Dec 15, 2023

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

  1. Checkout backport branch and update it.
git checkout 4.0.x
git pull
  1. Cherry pick the first parent branch of the this PR on top of the older branch:
git cherry-pick -x -m1 98d1e06796906ce622559146ed127075a7a46f71
  1. You will likely have some merge/cherry-pick conflict here, fix them and commit:
git commit -am 'Backport PR #15386: Improve scrolling to heading'
  1. Push to a named branch:
git push YOURFORK 4.0.x:auto-backport-of-pr-15386-on-4.0.x
  1. Create a PR against branch 4.0.x, I would have named this PR:

"Backport PR #15386 on branch 4.0.x (Improve scrolling to heading)"

And apply the correct labels and milestones.

Congratulations — you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon!

Remember to remove the Still Needs Manual Backport label once the PR gets merged.

If these instructions are inaccurate, feel free to suggest an improvement.

krassowski pushed a commit to krassowski/jupyterlab that referenced this pull request Dec 26, 2023
* 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)
krassowski added a commit that referenced this pull request Dec 29, 2023
* 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment