From 2a4a6db1e6ce505f3b2d91f7762dee4d633e407d Mon Sep 17 00:00:00 2001 From: Patrick McDougle Date: Fri, 22 Nov 2024 16:17:49 -0600 Subject: [PATCH 1/3] Fix rendering performance bottleneck in carousel --- src/components/carousel/carousel.component.ts | 35 +++++++++++-------- 1 file changed, 21 insertions(+), 14 deletions(-) diff --git a/src/components/carousel/carousel.component.ts b/src/components/carousel/carousel.component.ts index 55cbbc1aa..8ecf3d59f 100644 --- a/src/components/carousel/carousel.component.ts +++ b/src/components/carousel/carousel.component.ts @@ -506,20 +506,27 @@ export default class SlCarousel extends ShoelaceElement { } private scrollToSlide(slide: HTMLElement, behavior: ScrollBehavior = 'smooth') { - 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 - }); + 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 + }); + } + } + if ('requestAnimationFrame' in window) { + window.requestAnimationFrame(doWork); + } else { + doWork(); } } From dea63b33880608a80140491c3d2cc7d19b741308 Mon Sep 17 00:00:00 2001 From: Patrick McDougle Date: Tue, 3 Dec 2024 11:35:52 -0600 Subject: [PATCH 2/3] Remove check before using RAF --- src/components/carousel/carousel.component.ts | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/src/components/carousel/carousel.component.ts b/src/components/carousel/carousel.component.ts index 8ecf3d59f..33bff07ff 100644 --- a/src/components/carousel/carousel.component.ts +++ b/src/components/carousel/carousel.component.ts @@ -506,7 +506,7 @@ export default class SlCarousel extends ShoelaceElement { } private scrollToSlide(slide: HTMLElement, behavior: ScrollBehavior = 'smooth') { - const doWork = () => { + window.requestAnimationFrame(() => { const scrollContainer = this.scrollContainer; const scrollContainerRect = scrollContainer.getBoundingClientRect(); const nextSlideRect = slide.getBoundingClientRect(); @@ -522,12 +522,7 @@ export default class SlCarousel extends ShoelaceElement { behavior }); } - } - if ('requestAnimationFrame' in window) { - window.requestAnimationFrame(doWork); - } else { - doWork(); - } + }); } render() { From 68be53c29e98e4589495781c8ee25bded25b64ec Mon Sep 17 00:00:00 2001 From: Patrick McDougle Date: Tue, 3 Dec 2024 11:41:44 -0600 Subject: [PATCH 3/3] Optimistically mark the slide change as pending until the rAF callback runs --- src/components/carousel/carousel.component.ts | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/components/carousel/carousel.component.ts b/src/components/carousel/carousel.component.ts index 33bff07ff..98c6e93f3 100644 --- a/src/components/carousel/carousel.component.ts +++ b/src/components/carousel/carousel.component.ts @@ -506,6 +506,9 @@ export default class SlCarousel extends ShoelaceElement { } private scrollToSlide(slide: HTMLElement, behavior: ScrollBehavior = 'smooth') { + // Since the geometry doesn't happen until rAF, we don't know if we'll be scrolling or not... + // It's best to assume that we will and cleanup in the else case below if we didn't need to + this.pendingSlideChange = true; window.requestAnimationFrame(() => { const scrollContainer = this.scrollContainer; const scrollContainerRect = scrollContainer.getBoundingClientRect(); @@ -515,12 +518,16 @@ export default class SlCarousel extends ShoelaceElement { const nextTop = nextSlideRect.top - scrollContainerRect.top; if (nextLeft || nextTop) { + // This is here just in case someone set it back to false + // between rAF being requested and the callback actually running this.pendingSlideChange = true; scrollContainer.scrollTo({ left: nextLeft + scrollContainer.scrollLeft, top: nextTop + scrollContainer.scrollTop, behavior }); + } else { + this.pendingSlideChange = false; } }); }