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

Adjust CSS styles degrading performance in Chromium browsers #13159

Conversation

krassowski
Copy link
Member

@krassowski krassowski commented Oct 2, 2022

References

Fixes #9757 and fixes #13160.

Code changes

  • adds a test case which initially is expected to fail on main branch with:
    Detected CSS selectors that might cause performance issues in Chromium [
      '.jp-icon-hoverShow:not(:hover) svg',
      '.jp-CodeMirrorEditor .remote-caret:hover > div',
      '.jp-RunningSessions-shutdownAll.jp-mod-styled:hover > span',
      '.jp-RunningSessions-shutdownAll.jp-mod-styled.jp-mod-disabled:hover > span',
      '.f437k8z:hover div',
      '.MathJax_Hover_Arrow:hover span',
      '.MathJax_MenuClose:hover span'
    ]
    
  • removes or increases specificity of the above selectors

Follow-up tasks:

  • check other pseudo-classes
  • check if we can reduce time for other performance-critical tasks by the approach of dissecting style recalculation

How to test

  1. Download gh-9757-reproducer.ipynb
  2. Run all cells
  3. Open menu and compare how responsive it is to hover events before and after the patch
  4. (Optionally) record performance profile and compare the Recalculate Style event duration
Before After
Screenshot from 2022-10-02 21-52-39 Screenshot from 2022-10-02 21-49-56

User-facing changes

  • Menu interactions and other :hover actions should no longer be sluggish when many <div>, <span> or <svg> nodes are present.
  • Styles for caret are applied again, adapted to CodeMirror 6 y.js styling. The cursor is now 1px rather than 2px in 3.x since now there is a dot over the cursor to help with visibility.
    Before (on 4.0.0-alpha29) After (as it was on 3.x, adapted)
    Screenshot from 2022-10-02 20-13-32 Screenshot from 2022-10-02 20-13-16
  • jp-icon-hoverShow now requires the icon inside of the container to be marked with jp-icon-hoverShow-content class. Note that in JupyterLab codebase this is only used by the command palette headers to show the filter icon on hover, and these headers (.lm-CommandPalette-header) are hidden in the modal palette.

Backwards-incompatible changes

jp-icon-hoverShow-content requirement

This test case is expected to fail as of this commit with:

```
Detected CSS selectors that might cause performance issues in Chromium [
  '.jp-icon-hoverShow:not(:hover) svg',
  '.jp-CodeMirrorEditor .remote-caret:hover > div',
  '.jp-RunningSessions-shutdownAll.jp-mod-styled:hover > span',
  '.jp-RunningSessions-shutdownAll.jp-mod-styled.jp-mod-disabled:hover > span',
  '.f437k8z:hover div',
  '.MathJax_Hover_Arrow:hover span',
  '.MathJax_MenuClose:hover span'
]
```
@jupyterlab-probot
Copy link

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

also removing the unscoped tag (`div`) after pseudo-class
selector in order to improve performance in Chromium browsers.
performance problems in Chromium browsers.
rather than `svg` tag which was causing performance issues in Chrome
instead of generic `div` selector for shortcut title
to workaround Chromium performance issues.
@krassowski krassowski changed the title Add test case detecting CSS rules degrading performance in Chromium Adjust CSS styles degrading performance in Chromium browsers Oct 2, 2022
@krassowski krassowski marked this pull request as ready for review October 2, 2022 21:09
@krassowski
Copy link
Member Author

please run benchmark

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 @krassowski

I left an optional comment. But happy to merge as is.

galata/test/jupyterlab/styles.test.ts Outdated Show resolved Hide resolved
Co-authored-by: Frédéric Collonval <[email protected]>
@fcollonval
Copy link
Member

Merging to cut a final patch release for 3.4.x (with this and #13161) and start 3.5

@fcollonval fcollonval merged commit 3a7e1b8 into jupyterlab:master Oct 3, 2022
@fcollonval
Copy link
Member

@meeseeksdev please backport to 3.4.x

@lumberbot-app
Copy link

lumberbot-app bot commented Oct 3, 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 3.4.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 3a7e1b81ef3f3b5f69c10150310f93d20a460b9c
  1. You will likely have some merge/cherry-pick conflict here, fix them and commit:
git commit -am 'Backport PR #13159: Adjust CSS styles degrading performance in Chromium browsers'
  1. Push to a named branch:
git push YOURFORK 3.4.x:auto-backport-of-pr-13159-on-3.4.x
  1. Create a PR against branch 3.4.x, I would have named this PR:

"Backport PR #13159 on branch 3.4.x (Adjust CSS styles degrading performance in Chromium browsers)"

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
Copy link
Member Author

The backport will need to be manual commit-by-commit (e.g. there is no .jp-RunningSessions-shutdownAll.jp-mod-styled:hover > span and the tests will also fail on Blueprint CSS rules - I don't know how to address those yet). I started some work locally but waited on getting this approved; I can finish up in the evening.

@fcollonval
Copy link
Member

The backport will need to be manual commit-by-commit (e.g. there is no .jp-RunningSessions-shutdownAll.jp-mod-styled:hover > span and the tests will also fail on Blueprint CSS rules - I don't know how to address those yet). I started some work locally but waited on getting this approved; I can finish up in the evening.

Thanks @krassowski

fcollonval pushed a commit that referenced this pull request Oct 4, 2022
…rmance in Chromium browsers) (#13175)

* Add test case detecting CSS rules degrading performance in Chromium

This test case is expected to fail as of this commit with:

```
Detected CSS selectors that might cause performance issues in Chromium [
  '.jp-icon-hoverShow:not(:hover) svg',
  '.jp-CodeMirrorEditor .remote-caret:hover > div',
  '.jp-RunningSessions-shutdownAll.jp-mod-styled:hover > span',
  '.jp-RunningSessions-shutdownAll.jp-mod-styled.jp-mod-disabled:hover > span',
  '.f437k8z:hover div',
  '.MathJax_Hover_Arrow:hover span',
  '.MathJax_MenuClose:hover span'
]
```

(cherry picked from commit 9356447)

* Use `.jp-icon-hoverShow-content` to denote the icon to hide

rather than `svg` tag which was causing performance issues in Chrome

(cherry picked from commit baad021)

* Use class `.jp-ShortcutTitleItem-sortButton` selector

instead of generic `div` selector for shortcut title
to workaround Chromium performance issues.

(cherry picked from commit ddc0854)

* Disable MathJax styles deteriorating performance in Chromium browsers

(cherry picked from commit 286fd04)

* Limit the impact of `.remote-caret:hover div` on performance

in Chromium browsers by increasing specificity of the `div` part.

* Ignore problematic Blueprint CSS styles on 3.4.x

* Fix incorrect expect in the styles test
@fperez
Copy link
Contributor

fperez commented Oct 10, 2022

I just wanted to thank @krassowski for this - tracking down the details of this issue was a ton of work, and it's hugely, hugely appreciated!! These performance issues have lingered for a long time and can be quite frustrating when they happen, so I'm sure I speak for many with my gratitude :) Keep up the awesome work!!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CodeMirror RTC caret styles are not applied Sluggish UI when long, math-heavy notebooks are open.
3 participants