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

Poor Scrolling Performance on CSS Demo/Documentation Page in Chrome/Safari #6

Closed
ericdrobinson opened this issue Nov 16, 2018 · 9 comments
Labels
documentation Because documentation is important and shouldn't be broken

Comments

@ericdrobinson
Copy link

ericdrobinson commented Nov 16, 2018

Expected Behavior

Scrolling through demos/documentation should be buttery smooth.

Actual Behavior

Scrolling is very jerky, making for a poor experience.

Reproduce Scenario (including but not limited to)

Visit the documentation and scroll in certain browsers.

Steps to Reproduce

  1. Open the documentation using Chrome or Safari.
  2. Navigate to the demo/documentation page.
  3. Scroll the page at high speed.

* Firefox 63 does not exhibit this issue, though its Performance developer tools may provide further insights when investigating.

Browser name/version/os

  • Chrome Version 70.0.3538.77 (Official Build) (64-bit)
  • Safari Version 12.0.1 (14606.2.104.1.1)

Both tested on macOS 10.14.1.

Spectrum-CSS version

2.5.1

Sample Code that illustrates the problem

N/A

Screenshots (if applicable)

N/A

@filmaj
Copy link
Contributor

filmaj commented Nov 20, 2018

Indeed, can confirm this is an issue.

I did a quick-n-dirty Chrome devtools performance recording while scrolling. Here's a screencap:

screen shot 2018-11-20 at 3 08 48 pm

It seems like the docs.js set-hash-from-scroll scroll event handler is causing the slowdown. I'll take a look at possibly optimizing this.

@filmaj
Copy link
Contributor

filmaj commented Nov 20, 2018

Note that on my Macbook Pro, I can't reproduce these bottleneck events unless I set the CPU throttling to 4x via the Chrome Devtools Performance tab.

filmaj added a commit to filmaj/spectrum-css that referenced this issue Nov 20, 2018
@filmaj filmaj added documentation Because documentation is important and shouldn't be broken performance labels Nov 21, 2018
@ericdrobinson
Copy link
Author

I don't believe that you've quite narrowed down the issue correctly. The performance issue on my machine is NOT a CPU issue. It's a Rendering issue. Take a look at this screenshot:

chromeperf

I'm on a 2018 15" MBP laptop. My CPU speed is not an issue.

The issue may be related to updating the URL hash. If that were the case then I would expect it to be browser-dependent: Chrome, for instance, may trigger a repaint when the hash is changed. If that turns out to be the case then a valid approach to fixing this may be to only update the hash when scrolling ends.

I would highly recommend digging into what could cause Rendering issues here. This may be a good starting point.

That said, why aren't you guys using the Intersection Observer API [polyfill] for this?

Also, as a bit of extra feedback, updating the history as you do is a usability issue in-and-of itself. You break the browser's back button in this manner. Not only does hitting the back button NOT jump you back to the section in question, but you have to actually hit the back button a whole lot to get back to the page that linked you to the documentation if you did any serious scrolling. This is why most pages don't update the hash automatically but provide links that allow you to copy that specific location in case you need to send someone there from an external location. The ExpressJS API page is a shining example of just such a setup.

@ericdrobinson
Copy link
Author

As a side note, I've noticed that scrolling in Chrome is super speedy when the page is first rendered. It only slows down dramatically once the Markup is loaded.

Wouldn't you know it? The Markup appears at the same instant that the URL hash updating turns on. Hopefully this is helpful in tracking down the source of the issue.

@benjamind
Copy link
Contributor

There are a couple of issues at play here. @filmaj has done some work to reduce the use of the scroll hash updating until after 100ms after scrolling is complete which definitely helps a little as the update was causing a relayout through use of offsetTop. This is a good step, but I do agree with you @ericdrobinson that the hash should not update during scrolling as it pollutes the browser history.

I did some other preliminary analysis of rendering performance and it appears that most time is spent in Update Layer Tree during scroll. This step in the rendering pipeline appears to be taking a long time because the layer tree is very large with so many DOM nodes on the page. This is actually made worse once the markup loads since it involves a lot more DOM to render the styled markup.

Ultimately the docs site will probably need to be split up into sections to help alleviate this, or some kind of virtual scrolling needs to be implemented so everything can remain on the single page, but the number of DOM nodes is reduced.

@filmaj
Copy link
Contributor

filmaj commented Nov 21, 2018

Thanks @ericdrobinson ! I am just getting my feet wet with the devtools performance stuff. I appreciate all the links and tips you gave, I will do some more exploring, and with @benjamind's experience and help here, I think we can get to a solution.

@ericdrobinson
Copy link
Author

ericdrobinson commented Nov 21, 2018

@benjamind I disagree. I think the fix is extremely simple:

Remove the setURLParams function.

I just did this in the console while on the site:

setURLParams = function () {};

Then I went back and began scrolling in Chrome. The performance issues completely disappeared. Not only that, but the back button was usable. I can still get the links to the specific sections by clicking on their headers (or right-clicking and selecting the "Copy Link Address" option).

I would highly recommend removing the auto-hash update "feature" and replace it with a link icon (🔗) that users can use to grab a link to the header of their interest. This fixes two issues at once: usability and scrollability.

@ericdrobinson
Copy link
Author

I just tested that change in Safari 12.0.1 and it doesn't seem to really improve things there. I suspect that something else on the page is causing the slowdown for that browser (whatever the cause, it may affect iOS browsers in a similar fashion...).

@filmaj
Copy link
Contributor

filmaj commented Nov 21, 2018

Regardless of the impact on performance for setting the hash, I still like the suggestion of replacing auto-updating hash w/ anchor links - if only to stop messing with history. I will spin that out into its own issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Because documentation is important and shouldn't be broken
Projects
None yet
Development

No branches or pull requests

4 participants