Skip to content

Commit

Permalink
fix: prevent loop from animated scrolling
Browse files Browse the repository at this point in the history
It happened quite a lot on touch devices, that when crossing boundary
of genuine items, it would reposition the slider but with animation on.
It happened because, yes, I shifted the slider and only then enabled the
animations again, even in a setTimeout, but that in and of itself doesn't
guarantee that the animation enabling runs after the shift.

The event loop can queue multiple macrotasks (also setTimeout's) before
it repaints and that's what was happening in my case - the shift was
scheduled but so was also enabling the animations.

The solution was to make sure I wait for after the next paint before
re-enabling animations, which I achieved with a combo of
requestAnimationFrame and setTimeout in this order.

requestAnimationFrame makes sure my code is executed just before
the next paint,
where the shift is going to happen, and setTimeout "jumps over" to the next
frame where and only then the animation is enabled again.
  • Loading branch information
daelmaak committed Aug 13, 2023
1 parent 1015d84 commit 97a8f68
Show file tree
Hide file tree
Showing 3 changed files with 25 additions and 12 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@
#itemsRef
*ngFor="let item of displayedItems; let i = index"
media
[class.viewer-item--selected]="i === selectedIndex"
[attr.tabindex]="itemTabbable(i)"
[attr.aria-label]="item.alt"
[attr.aria-describedby]="'viewer-aria-description-' + i"
Expand Down
19 changes: 12 additions & 7 deletions libs/gallery/src/lib/components/viewer/viewer.component.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -358,6 +358,7 @@ describe('ViewerComponent', () => {
expect(index).toBe(-1);
expect(tweak).toBe((ITEM_WIDTH / 3) * 2);

tick(16); // requestAnimationFrame
flush(); // give time for centering to happen
({ index, tweak } = getSlidePx());
expect(index).toBe(-1);
Expand All @@ -371,9 +372,10 @@ describe('ViewerComponent', () => {
slideByMouse(usersShiftDistance);

let { index, tweak } = getSlidePx();
expect(index).toBe(-2);
expect(tweak).toBe((ITEM_WIDTH / 3) * 2);
expect(index).toBe(-1);
expect(tweak).toBe(-ITEM_WIDTH / 3);

tick(16); // requestAnimationFrame
flush(); // give time for centering to happen
({ index, tweak } = getSlidePx());
expect(index).toBe(-2);
Expand All @@ -393,6 +395,7 @@ describe('ViewerComponent', () => {
expect(index).toBe(-1);
expect(tweak).toBe((ITEM_WIDTH / 3) * 2);

tick(16); // requestAnimationFrame
flush(); // give time for centering to happen
({ index, tweak } = getSlidePx());
expect(index).toBe(-1);
Expand All @@ -404,6 +407,7 @@ describe('ViewerComponent', () => {
const usersShiftDistance = -(ITEM_WIDTH * 3);

slideByTouch(usersShiftDistance);
tick(16); // requestAnimationFrame
flush();

const { index, tweak } = getSlidePx();
Expand All @@ -418,9 +422,10 @@ describe('ViewerComponent', () => {
slideByTouch(usersShiftDistance);

let { index, tweak } = getSlidePx();
expect(index).toBe(-2);
expect(tweak).toBe((ITEM_WIDTH / 3) * 2);
expect(index).toBe(-1);
expect(tweak).toBe(-ITEM_WIDTH / 3);

tick(16); // requestAnimationFrame
flush(); // give time for centering to happen
({ index, tweak } = getSlidePx());
expect(index).toBe(-2);
Expand Down Expand Up @@ -608,16 +613,16 @@ describe('ViewerComponent', () => {
function getSlidePx() {
const { transform } = component.itemListRef.nativeElement.style;

// console.log(transform);

if (!transform) {
return {
index: 0,
tweak: 0,
};
}

const [, index, tweak] = transform.match(/3d\(calc\((-?\d+).*\s(\d+)px\)/)!;
const [, index, tweak] = transform.match(
/3d\(calc\((-?\d+).*\s(-?\d+)px\)/
)!;

return {
index: +index,
Expand Down
17 changes: 13 additions & 4 deletions libs/gallery/src/lib/components/viewer/viewer.component.ts
Original file line number Diff line number Diff line change
Expand Up @@ -430,10 +430,19 @@ export class ViewerComponent implements OnChanges, OnInit, AfterViewInit {
? desiredIndex + this.items.length
: desiredIndex - this.items.length;

setTimeout(() => {
this.noAnimation = false;
this.center();
});
// NOTE: This is needed so that animation is reactivated really only after the loop shift
// happened. Without the requestAnimationFrame, the setTimeout is often not enough, as it
// can happen still before the next frame is painted, which would cause the loop shift
// to be animated.
// But, requestAnimationFrame is not enough as its callback is called BEFORE the next paint,
// not after. Second requestAnimationFrame would also be possible, but setTimeout is better
// as it's called right after the next paint happens.
requestAnimationFrame(() =>
setTimeout(() => {
this.noAnimation = false;
this.center();
})
);
}

private updateDimensions = () => {
Expand Down

0 comments on commit 97a8f68

Please sign in to comment.