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

Scrollbar UI for virtually rendered notebook #15109

Closed
wants to merge 68 commits into from

Conversation

afshin
Copy link
Member

@afshin afshin commented Sep 12, 2023

This PR updates the WindowedList class that the notebook widget inherits from to have customizable renderers (which allows for adding aria labels to make windowed lists more accessible and allows for customization of the UI).

The renderer in turn is used to add a scrollbar component to replace the native scrollbar that jumps when virtual notebook rendering is enabled.

The scrollbar (and its toolbar button) only appear when "Windowing mode" is set to "full".

References

Code changes

  • Updates the WindowedList API to allow for custom renderers of windowed list subcomponents
  • Adds a virtual scrollbar UI to replace the native scrollbar when virtual rendering is turned on
  • Updates the command toolbar button to respect isEnabled, isVisible, isToggled.

User-facing changes

Adds an optional new scrollbar control for virtually rendered notebooks.

before

virtual-scrollbar-before.mov

after

virtual-scrollbar-after.mov

Backwards-incompatible changes

N/A

@afshin afshin added enhancement tag:Design and UX tag:Virtual Rendering Lazy and virtual rendering of notebook issues and PRs labels Sep 12, 2023
@afshin afshin added this to the 4.1.0 milestone Sep 12, 2023
@jupyterlab-probot
Copy link

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

@afshin afshin force-pushed the virtual-scrolling branch 4 times, most recently from 9cfb195 to e93dde0 Compare September 13, 2023 08:23
@afshin afshin force-pushed the virtual-scrolling branch 12 times, most recently from 232164f to 9bede31 Compare September 26, 2023 17:06
@krassowski
Copy link
Member

This is kind of hard to manage due to faults in GitHub UI. After every force-push I have to click 7 times to get the most recent discussion show up:

1-minute-long-srcolling-session

I also get two emails, one for the pull request and one for the commit with "fix plugin manager test @krassowski" message.

As a partial alleviation, can I kindly suggest to:

  • squash all the Update Playwright Snapshots commits as these do not provide any thing useful (in fact I would say lets revert all snapshots and correct the CSS styles so that there are no changes at all; something I started working in the evening but did not manage to get in time)
  • (minor) remove usernames from commit messages
  • remove all comments requesting bot snapshot updates, and stop issuing those. I think we do not need any single snapshot to change in this PR, it just makes the review harder.

@krassowski
Copy link
Member

I see how this could be merged soon if the style changes are limited to when the scrollbar is indeed active. It would ensure no breakages downstream. This could be confirmed by snapshots having no changes.

@krassowski
Copy link
Member

A few tests are failing not because of snapshots but because this class name in test file needs updating:

--- a/galata/test/jupyterlab/windowed-notebook.test.ts
+++ b/galata/test/jupyterlab/windowed-notebook.test.ts
@@ -40,7 +40,7 @@ async function getInnerHeight(panel: ElementHandle<Element>) {
 async function getWindowHeight(panel: ElementHandle<Element>) {
   return parseInt(
     await panel.$eval(
-      '.jp-WindowedPanel-window',
+      '.jp-WindowedPanel-viewport',
       node => (node as HTMLElement).style.minHeight
     ),
     10

Is this change documented in the migration guide? I wonder if this could impact downstream (themes). A safe path would be to keep both and deprecate -window variant, but possibly mentioning this in the migration guide could be enough.

@afshin afshin closed this Dec 14, 2023
krassowski added a commit that referenced this pull request Dec 15, 2023
* Updated input field with more descriptive aria-labels and placeholders

* Package integrity updates

* Run integrity

* Move ID generation to improve render flow

* Use ellipsis character for consistency

* Add error indicator in Table of Contents (#14784)

* feat: add error indicator in toc

* test: for error indicator

* Update Playwright Snapshots

* style: make the error symbol colorblind-accessible

* Update Playwright Snapshots

* fix: remove from _errorCells when cell is rerun

* perf: update error to -0.5 for api compatibility

* test: update error status to -0.5

* Update Playwright Snapshots

* Force text symbol

* Update Playwright Snapshots

* Update Playwright Snapshots

* Update Playwright Snapshots

* Revert notebook-panel-1 snapshot update

* Robustify test by defining notebook a priori

* Update Playwright Snapshots

---------

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: Michał Krassowski <5832902+krassowski@users.noreply.github.com>
Co-authored-by: Frédéric Collonval <fcollonval@users.noreply.github.com>

* Remove unnecessary requirement from servicesPlugin (#15362)

* [pre-commit.ci] pre-commit autoupdate (#15358)

* [pre-commit.ci] pre-commit autoupdate

updates:
- [github.com/pre-commit/pre-commit-hooks: v4.4.0 → v4.5.0](pre-commit/pre-commit-hooks@v4.4.0...v4.5.0)
- [github.com/python-jsonschema/check-jsonschema: 0.27.0 → 0.27.1](python-jsonschema/check-jsonschema@0.27.0...0.27.1)
- [github.com/psf/black: 23.9.1 → 23.10.1](psf/black@23.9.1...23.10.1)
- [github.com/astral-sh/ruff-pre-commit: v0.0.292 → v0.1.4](astral-sh/ruff-pre-commit@v0.0.292...v0.1.4)

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

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

* Alig ruff version

* Add ruff exception

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Frédéric Collonval <fcollonval@users.noreply.github.com>

* Update notebook window on resize if height changes (#15357)

* Add a test for resizing notebook (should fail)

* Update window on resize

* Polish the test

* Add the test notebook

* Use throttler to limit the number of updates

Using throttler over debouncer proposed in #15109 because
debouncer would not show cells progressively during resize
until user has stopped resizing.

* Open files from errors (#13390)

* Create renderer to render errors

* WIP tests scaffold

* Implement opening files from paths in error renderer

* Finish sentence in a comment

* Apply suggestions from code review

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

* Improve RFC 5147 adherence for ranges

* Switch to 0-based line numbers based on RFC5147 2.2.3:

"Line position counting starts with zero, so the line
position before the first line of a text/plain MIME
entity has the line position zero"

* Fix undefined check after adding `parseInt`

---------

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

* Bump tj-actions/changed-files from 39.2.0 to 40.0.2 (#15342)

Bumps [tj-actions/changed-files](https://github.com/tj-actions/changed-files) from 39.2.0 to 40.0.2.
- [Release notes](https://github.com/tj-actions/changed-files/releases)
- [Changelog](https://github.com/tj-actions/changed-files/blob/main/HISTORY.md)
- [Commits](tj-actions/changed-files@v39.2.0...v40.0.2)

---
updated-dependencies:
- dependency-name: tj-actions/changed-files
  dependency-type: direct:production
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Fix scrolling past long outputs in presence of un-rendered headings (#15356)

* Add a test for smooth scrolling over long outputs

* Do no scroll when setting the cursor position in md cells

* Adjust test name to clarify the test case

* Add the test notebook

* Clean up the test directory

* Fix overreactive scrolling to next cell after `Shift + Enter` (#15288)

* Add a test case for #14878

* Fix the smart/auto scrolling logic for long items

Items larger than the size of the viewport were not
considered by the scroll logic leading to bad UX when
smart/auto scroll was requested on certain operations.

* Use pixel-based, cell-height-derived threshold for scrolling

* Implement dual behaviour, simplify code, account for top padding

* Use greater or equal to avoid scrolling if item fully visible

* Update tests to reflect new expectations

* Clean up the temp directory after tests

* Fix scrolling when dragging files in the file browser (#15318)

* Fix scrolling on edges in file browser

* Add a simple test

* Fix update button in extension manager (#15331)

* Fix update button in extension manager

* Apply suggestions from code review

Make action options more flexible

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

* Fix docs and make install options optional

* Add test for update button

* Update Playwright Snapshots

* Revert spurious snapshot updates

---------

Co-authored-by: Frédéric Collonval <fcollonval@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: krassowski <5832902+krassowski@users.noreply.github.com>

* added default property (#15346)

* Exclude ipynb files in prettier pre-commit (#15378)

* Update @jupyter/ydoc in dev_mode (#15383)

* Define cells to run as independent of selection (#14996)

* Define cells to run as independent of selection

* Restore side effects of `runAll()`

* Skip optional `slice` argument

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

* Restore previous behaviour of `runAllBelow()`

by making last cell active.

---------

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

* Fix highlighting search in an out-of-viewport cell (#15376)

* Fix highlighting search in an out-of-viewport cell

* Update cells count

* Don't show default value for objects in Settings Editor (#15380)

* Don't show default value if schema type is `object`

* Prettier

* Bump axios from 1.3.4 to 1.6.1 (#15385)

Bumps [axios](https://github.com/axios/axios) from 1.3.4 to 1.6.1.
- [Release notes](https://github.com/axios/axios/releases)
- [Changelog](https://github.com/axios/axios/blob/v1.x/CHANGELOG.md)
- [Commits](axios/axios@v1.3.4...v1.6.1)

---
updated-dependencies:
- dependency-name: axios
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Fix connection loop issue with standalone foreign document in LSP (#15262)

* Populate

* Improve VirtualDocument

* Add test

* Update packages/lsp/test/document.spec.ts

Co-authored-by: Michał Krassowski <5832902+krassowski@users.noreply.github.com>

---------

Co-authored-by: Michał Krassowski <5832902+krassowski@users.noreply.github.com>

* Update to TypeScript 5.1 (#14638)

* Bump typescript to 5.1.6 and rimraf to 5.0.5

* Dedupe the yarn.lock

---------

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

* Resolved merge conflict

* Package integrity updates

Merged changes from main

* Package integrity updates

* Package integrity updates

* Update Playwright Snapshots

* Fixed integrity

* Update Playwright Snapshots

* Update Playwright Snapshots

* Remove spurious snapshot updates

* Update the test to reflect the new wording

---------

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: EC2 Default User <m158261@users.noreply.github.com>
Co-authored-by: Michał Krassowski <5832902+krassowski@users.noreply.github.com>
Co-authored-by: Tian Wang <wangtian0312@gmail.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: Frédéric Collonval <fcollonval@users.noreply.github.com>
Co-authored-by: Paul Kim <paulkim3151@gmail.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Divyansh Choudhary <divyanshchoudhary99@gmail.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Nate Bowditch <111072326+nbowditch-einblick@users.noreply.github.com>
Co-authored-by: LJMP <24670649+LJMP@users.noreply.github.com>
Co-authored-by: Nicolas Brichet <32258950+brichet@users.noreply.github.com>
Co-authored-by: firai <firai.admin@gmail.com>
Co-authored-by: Duc Trung Le <leductrungxf@gmail.com>
Co-authored-by: Jeremy Tuloup <jeremy.tuloup@gmail.com>
Co-authored-by: Frédéric Collonval <fcollonval@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Scrollbar jumps back in a virtualized notebook on scrolling
6 participants