Add virtual scrollbar component to windowed lists. #15533
Conversation
|
Thanks for making a pull request to jupyterlab! |
github-actions
bot
added
documentation
pkg:notebook
tag:Examples
tag:Testing
pkg:ui-components
tag:CSS
|
@krassowski I wasn't sure how to do the git shenanigans you described, so I've made a new branch and a clean PR. I hope this is okay! |
krassowski
left a comment
krassowski
left a comment
There was a problem hiding this comment.
Thank you! There are two blocking regression left:
- a) scroll past end is no longer working
- b) the logic for enabling the scrollbar seems flowed; it auto-enables in our visual tests when windowing mode is set to full, making them fail because native scrolling does not work; I would expect it to only show up when explicitly enabled by the user
The remaining comments are the bugs in the new feature rather than regressions so I do not want to block on them as we can iterate in beta/RC cycle.
Snapshots
Can you please revert all snapshot updates by running:
git checkout 98d1e06796906ce622559146ed127075a7a46f71 galata/test/*/*.ts-snapshots/
and committing changes?
This way:
- we do not inflate the repository size unnecessarily
- we know the snapshot changes are due to random encoding differences (which I believe is the case) and not some tiny shifts in layout
- review becomes easier as I do not have to analyse one-pixel or smaller differences on the snapshots each time
I wold suggest not calling the bot for snapshot updates for now.
I did this on a sub-branch and while on this branch there are 19+1 failures, my branch has 18+0 failures which suggests that the snapshot updates so far are not relevant. Instead they are all due to the faulty logic for toggling the scrollbar.
UX
I really miss an indication on where my viewport is on the scrollbar and will probably not use this this feature until it shows the contour of the visible cells but this can be addressed in the future.
| this.node.style.width = 'calc(100% + 25px)'; | ||
| const scrollbar = this.node.querySelector('.jp-WindowedPanel-scrollbar'); | ||
| const offset = scrollbar!.getBoundingClientRect().width + 15; | ||
| this._innerElement.style.width = `calc(100% - ${offset}px)`; |
There was a problem hiding this comment.
Layout trashing. Resizing does not look pretty:
I guess this is not blocking, but I would like to find a solution which does not involve update/query/update sequence as this is really bad for performance.
packages/notebook/src/widget.ts
Outdated
| layout: new NotebookWindowedLayout() | ||
| layout: new NotebookWindowedLayout(), | ||
| renderer: options.renderer ?? WindowedList.defaultRenderer, | ||
| scrollbar: windowingActive |
There was a problem hiding this comment.
I think we should make this a setting instead of forcing users of windowed mode to use this (or toggle of each time).
This ensures that overlay scrollbar does not show up, and when normal scrollbars are shown that the outer one is hidden, while also ensuring that toggling the virtual scrollbar restores the original styles.
| /** | ||
| * Render virtual scrollbar. | ||
| */ | ||
| private _renderScrollbar(): void { |
There was a problem hiding this comment.
Note for future: there is no guard preventing regeneration when nothing changed, this means it is recreated on every cursor movement in resize.
There was a problem hiding this comment.
I started to tackle this problem by creating a scroll state variable that holds the list of selected items and the index of the active item. But the trouble is that part of the "state" of the scrollbar is out of our hands because the renderer that generates each item in the scrollbar might change arbitrarily, for example it could include a timestamp in it or something else that is nondeterministic.
So I think one approach this would be to use a virtual DOM (whether React or vdom, etc.) and let that decision be made by the tree-diffing algorithm.
Throttling it is probably a good idea, though.
|
I experimented a bit to address issues that I consider blocking highlighted in the review above: krassowski#80 - feel welcome to cherry-pick (for some reason I cannot open a PR against your branch). There are some improvements that could be made like only creating the resize observer if needed. I think that a way forward would be to disable this feature by default (rather than enable it always when windowed mode is on) and then add a setting (which would only take effect in windowed mode is on). Once we have the setting, we could flip it on by default once we feel this is ready. Having a setting would also make it easy to disable it for specific UI tests. |
… virtual-scrolling
…uery selector invocations
as the scroll container (rather than `jp-Notebook` as before)
krassowski
left a comment
krassowski
left a comment
There was a problem hiding this comment.
Thank you @afshin, this looks good now! I pushed a few commits to fix the galata tests and update the extension migration guide.
I plan to merge this PR tomorrow on 20th Dec at 13:00 UTC (unless changes are requested before this time) and follow with immediate release of 4.1.0.beta0.
|
Congrats @afshin on getting this one merged, and thanks @krassowski for the review! |
3 participants
This PR updates the
WindowedListclass that the notebook widget inherits from to have customizable renderers (which allows for addingarialabels 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
WindowedListAPI to allow for custom renderers of windowed list subcomponentsisEnabled,isVisible,isToggled.User-facing changes
Adds an optional visual scrollbar component for "full" windowing mode notebooks.
virtual-scrollbar.mov
Backwards-incompatible changes
.jp-WindowedPanel-windowis now called.jp-WindowedPanel-viewport