Avoid style recalculation of all elements on each scroll event

[Inwerpsel]

Description

Addresses laggy scrolling resulting from unnecessary style recalculations.

The most important thing about this PR is that this listener is removed.

document.addEventListener("scroll", evt => {
	root.style.setProperty("--scrolltop", root.scrollTop);
}, {passive: true});

Without the listener, the main thread is almost completely idle during scrolling. With the listener, the main thread is fully occupied with tasks that take hundreds of milliseconds on lower end devices. That means scrolling at a few FPS, 100% of the time.

The only thing this listener is doing is make an element behave as though it has position: sticky. So the goal of the PR is to convert it to position: sticky and remove the need for any style recalculation.

Using position: sticky also has better behavior in other ways. The previous code results in the bottom items not being reachable on long lists (such as /spaces).

Probably the best solution

The markup should probably be changed to be similar to the /api page.

For position: sticky to work, you need to have a container that spans the height you want the element to travel. So you'd want a container which is positioned next to the content and is equally high.

However, in the current markup the TOC element is inside of the content, so position: sticky cannot produce the intended effect of flowing next to the content.

This PRs CSS only solution

When trying out this markup change, I noticed it would cascade into too many other CSS changes and associated regression risk. I preserved a simple attempt at modifying the markup on a separate branch.

So it seemed warranted to try removing the performance issue with an as minimal change as possible while only changing the visual appearance for the better (i.e. it does the same except when the previous behavior was unintended, like the behavior near the bottom of the page).

The "trick":

• position #toc absolutely, to allow putting it next to the content
• give it 100% the height of its container
• also push it 100% to the right.
• This should reliably put the element into the position the previous code was putting it.

Performance improvement

The following screenshot compares performance during scrolling on the /spaces pages, before and after these changes. Both are on desktop, with no CPU throttling, so the improvement on lower end devices will be much greater.

• Time spent on style recalculation: 90% -> 0% (remaining purple is other rendering tasks)
• About 1 major GC / 200ms scrolling -> no major GC due to scrolling
• 25fps and main thread fully occupied -> 144fps and >50% idle time left

[netlify]

✅ Deploy Preview for colorjs ready!

Name	Link
🔨 Latest commit	e81446a
🔍 Latest deploy log	https://app.netlify.com/sites/colorjs/deploys/66fffbc7d52e8900081a55f2
😎 Deploy Preview	https://deploy-preview-592--colorjs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[LeaVerou]

If we have to set a fixed width, maybe set it in a font-relative unit so we can make CSS changes without having to adjust it.

[Inwerpsel]

If we have to set a fixed width, maybe set it in a font-relative unit so we can make CSS changes without having to adjust it.

Yeah I tried to preserve the previous behavior as much as possible, the previous fixed value was a quick fix.

I was now able to make it have the same behavior as before. Not by making fit-content work (it doesn't), but by using max-content and also giving it a max-width. I didn't quite figure out yet which spacing value to reference for the 3em, or if there is any.

[Inwerpsel]

@LeaVerou Are the current changes an acceptable way to address the performance issue? I added some info about the improvement in the description.

I did not find any regressions due to these changes. So it seems like a safe way to improve performance for all users, while removing the urgency of changing the markup to be similar to the /api page so it can accommodate sticky elements in a cleaner way.

Personally, I wouldn't worry too much about it using this 3em, or the exact spacing values, if no regressions are observed. If any later CSS changes need other dynamic values / variables to be in that place, that is something that can also be figured out at that time, and it will be more obvious what needs to go there. Also keep in mind that changing the markup (the better solution) will further simplify the CSS and remove many of the spacing values it now uses.

Of course, feel free to solve this in any other way. I mostly created this PR to show what the problem is and that it can be addressed. As long as the solution removes the listener, you will see the same performance improvement.

[DmitrySharabin]

I really like the proposed fix—It solves the issue elegantly. 👍

I would suggest minor changes to make the TOC even more responsive to the different screen sizes. Something like this:

@media (min-width: 1480px) {
+	--_gap: 1rem;

+	box-sizing: border-box;

	/* Stretch out #toc in the margin, so it can be a container for the sticky list. */
	position: absolute;
	right: 100%;
	height: 100%;
+	padding-inline: var(--_gap);
-	/* Still sorting what to use for the 3em. */
-	max-width: calc(var(--page-margin) - 3em);
+	max-width: calc(var(--page-margin) - 2 * var(--_gap));
	width: max-content;

	& > ul {
		position: sticky;
-		top: 1em;
+		top: var(--_gap);
	}
}

I didn't want to suggest them in the code so that we could discuss them here, but if it's more convenient for you, I can do that. I hope you don't mind. 🙃

TOC.mp4

[Inwerpsel]

Thanks @DmitrySharabin! Those changes look good to me, I added them to the PR.

I guess there are still some reasons to (eventually) use the same approach (or same code) as the TOC elements on the /api page. Those are independently scrollable and also work on smaller screens. They come from an API generator, so perhaps you can find and extract the template it uses for that.

[DmitrySharabin]

Thanks @DmitrySharabin! Those changes look good to me, I added them to the PR.

Great!

I guess there are still some reasons to (eventually) use the same approach (or same code) as the TOC elements on the /api page.

I would do it in another PR—it seems a bit orthogonal to the issue we are trying to solve here. Even with the changes you made, the website works and feels much better. So, I wouldn't hold this PR and ask @LeaVerou to review it. Let's see what she will tell us. 🙂

[Inwerpsel]

I would do it in another PR

Yes definitely, just mentioning it as a suggestion in case future changes happen to the TOC 🙂

[LeaVerou]

LGTM, thank you all!!