Fix rendering performance bottleneck in carousel

[patrick-mcdougle]

Fixes #2280

Before

After

Notice how none of the carousels on the right side have any synchronous Layout anymore, but the browser does one big layout once in the requestAnimationFrame callback.

NOTE: The scales on these screenshots are not the same...but the key point to notice is there there aren't as many tiny red dog-ears on each of the individual carousels which is the concern I was trying to fix.

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated (UTC)
shoelace	✅ Ready (Inspect)	Visit Preview	Dec 3, 2024 5:43pm

[patrick-mcdougle]

I did some light testing in chrome by using the override feature and it seemed to work alright, but I must warn, this will convert something that used to be synchronous to be async, so there might be some assumptions hiding somewhere which could be the source of bugs.

Someone (and I could be willing to do this), should probably audit all of the callers of goToSlide to see if any of the code after it's called could possibly expect that the actual scrolling has been completed. Biggest likely offender is that state property this.pendingSlideChange which would now be guaranteed to still be true in this frame, but not in this task/tick of the event loop.

[claviska]

@alenaksu any concerns before we merge this?

[alenaksu]

I've checked the changes and I don't believe it will cause any issue. The pendingSlideChange property is used to prevent slide synchronization when a slide change is triggered by user interaction, such as clicking a navigation dot, so it is not an issue in this case.

However, there could be a side effect causing multiple synchronizations during the initial render. I left a comment wwith a suggestion to overcome this, please take a look and let me know what you think.

[alenaksu]

To keep the original behaviour of the pendingSlideChange, we could set the value before the raf callback and then update it within if needed:

Suggested change

	const doWork = () => {
	const scrollContainer = this.scrollContainer;
	const scrollContainerRect = scrollContainer.getBoundingClientRect();
	const nextSlideRect = slide.getBoundingClientRect();
	const nextLeft = nextSlideRect.left - scrollContainerRect.left;
	const nextTop = nextSlideRect.top - scrollContainerRect.top;
	if (nextLeft || nextTop) {
	this.pendingSlideChange = true;
	scrollContainer.scrollTo({
	left: nextLeft + scrollContainer.scrollLeft,
	top: nextTop + scrollContainer.scrollTop,
	behavior
	});
	}
	this.pendingSlideChange = true;
	const doWork = () => {
	const scrollContainer = this.scrollContainer;
	const scrollContainerRect = scrollContainer.getBoundingClientRect();
	const nextSlideRect = slide.getBoundingClientRect();
	const nextLeft = nextSlideRect.left - scrollContainerRect.left;
	const nextTop = nextSlideRect.top - scrollContainerRect.top;
	if (nextLeft || nextTop) {
	scrollContainer.scrollTo({
	left: nextLeft + scrollContainer.scrollLeft,
	top: nextTop + scrollContainer.scrollTop,
	behavior
	});
	} else {
	this.pendingSlideChange = false;
	}

[patrick-mcdougle]

This would make pendingSlideChange=true more often than it is today.

Today it's only true if either the left or top is non zero, but here we would always be marking it has pending until the next frame.

I'm ok with that, but wanted to point out that it's also not a clean refactor / preservation of existing behavior.

[patrick-mcdougle]

See 68be53c

[alenaksu]

you can avoid this check and call rAF directly since it's supported by all browsers

    requestAnimationFrame(() => {
      ...
   });

[patrick-mcdougle]

ok, wasn't sure what minimum browser support was, so played it cautious. I can clean this up.

[patrick-mcdougle]

see dea63b3

[patrick-mcdougle]

this might also fix #2296 since the render making scrollContainer exist will happen before the rAF callback does