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

Add content-visibility hide mode #497

Merged
merged 2 commits into from Dec 24, 2022

Conversation

krassowski
Copy link
Member

@krassowski krassowski commented Dec 23, 2022

See #496 (comment).

Once support for content-visibility is better across browsers we should make it the default as we have exactly the use case that it was intended for (see point two in specification).

Performance-wise, I re-run the benchmark for three tabs (as in the issue: two test notebooks and launcher) after full integration and it works as expected:

Google Chrome 108

hidden mode IQM [Q1 - Q3] mean min
display 3043.3 [2944.3 – 3193.1] ms 3051.3 ms 2390.9 ms
content-visibility 631.6 [606.4 – 694.6] ms 687.7 ms 566.5 ms
scale 548.6 [531 – 561.3] ms 566.2 ms 506.6 ms

but differently to scale, it does not cause unintended issues in Chromium browsers where performance issues in presence of big notebooks are most pronounced, nor does it degrade in performance once there are too many tabs open.

Note: the benchmarks above were run with latest dev versions of JupyterLab (4.0alpha) and lumino from this PR, so these account for any potential changes to due windowed notebooks feature (full windowing mode).

Notes on current state in Firefox below. TLDR, we are good to merge IMO.

For anyone trying this out in Firefox: it may appear that it already "works" in Firefox 108 but it does not support content-visibility yet (although some basic support was already merged in 3 months ago, possibly pending bug fixes like this one). It "works" because in absence of content-visibility support it reduces to leaving the tab visible but hidden under the others due to z-index change. It means that we can confidently ship because it is in a way safer than existing scale mode (if user chooses to enable it in a browser which does not support content-visibility, their workspace will still work; scale breaks things in Chromium browsers). The performance in this case is fine for few tabs (the three test notebooks scenario):

hidden mode IQM [Q1 - Q3] mean min
display 1288.7 [1222.8 – 1351.8] ms 1304.6 ms 1132.1 ms
z-index 960.6 [928.8 – 1004.9] ms 1019.5 ms 871.2 ms
scale 925 [896.3 – 947.9] ms 933.2 ms 811.2 ms

But it is worse when there are many notebooks (each 20 code cells):

hidden mode 10 notebooks 30 notebooks
display 1982.4 [1833.4 – 2145.6] 12061.3 [11591.2 – 12455.9]
z-index 2517.6 [2435.2 – 2603.1] 21574.9 [21158.4 – 22010.4]
scale 2007.3 [1912 – 2113.4] 17439.5 [16916 – 18083.8]

Interestingly, scale in Firefox is only better for heavy notebooks, not for many small notebooks.

@krassowski krassowski added the enhancement New feature or request label Dec 23, 2022
@krassowski krassowski added this to the 1.x milestone Dec 23, 2022
Copy link
Member

@fcollonval fcollonval left a comment

Choose a reason for hiding this comment

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

Thanks a lot @krassowski

@fcollonval fcollonval merged commit fa048e1 into jupyterlab:main Dec 24, 2022
@fcollonval
Copy link
Member

@meeseeksdev please backport to 1.x

@lumberbot-app
Copy link

lumberbot-app bot commented Dec 24, 2022

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 1.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 fa048e1680cce138af24ee1aa964f2f02b5e61f6
  1. You will likely have some merge/cherry-pick conflict here, fix them and commit:
git commit -am 'Backport PR #497: Add content-visibility hide mode'
  1. Push to a named branch:
git push YOURFORK 1.x:auto-backport-of-pr-497-on-1.x
  1. Create a PR against branch 1.x, I would have named this PR:

"Backport PR #497 on branch 1.x (Add content-visibility hide mode)"

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 added a commit to krassowski/lumino that referenced this pull request Dec 24, 2022
@krassowski krassowski deleted the add-content-visibility branch December 24, 2022 14:28
fcollonval pushed a commit that referenced this pull request Dec 25, 2022
* Backport PR #497: Add content-visibility hide mode

* Patch `CSSStyleDeclaration` to avoid bumping TypeScript version on 1.x
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants