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.

Sign up for GitHub

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

Fix rendering performance bottleneck in carousel #2281

Merged
merged 3 commits into from
Dec 4, 2024
+24 −15
Merged

Fix rendering performance bottleneck in carousel #2281

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2a4a6db
Fix rendering performance bottleneck in carousel
patrick-mcdougle Nov 22, 2024
dea63b3
Remove check before using RAF
patrick-mcdougle Dec 3, 2024
68be53c
Optimistically mark the slide change as pending until the rAF callbac…
patrick-mcdougle Dec 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 21 additions & 14 deletions src/components/carousel/carousel.component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
});
}
Copy link
Collaborator

@alenaksu alenaksu Dec 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See 68be53c

}
if ('requestAnimationFrame' in window) {
window.requestAnimationFrame(doWork);
} else {
doWork();
}
claviska marked this conversation as resolved.
Show resolved Hide resolved
}

Expand Down