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.

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: use transform over left/right to eradicate CLS on 'wrap around' #1150

Merged
merged 1 commit into from
Dec 19, 2021

Conversation

redjam13
Copy link
Contributor

@redjam13 redjam13 commented Mar 15, 2021

Issue Description

Resolves #1087 (at least for the issue I am seeing; perhaps there are others).

Flickity instances using the wrapAround option are currently causing layout shift when 'wrapping'. As noted on the issue, horizontal layout shift is being caused by manipulation of the left and right positioning values. May also be an issue for free scrolling carousels?

🚨 As also noted, action is required to resolve this issue to avoid all web pages using Flickity with wrapAround: true being penalised for having a poor Cumulative Layout Shift (CLS) field score. From May 2021, Google search ranking will be affected for all pages with poor 'Core Web Vitals' (of which CLS is one). 🚨

Resolution

Replace usage of left/right positioning with the correct transform: translateX(value) (which is CLS-friendly).

Testing

Tested to have no CLS when using 'wrap around'. Couldn't find any regressions using the sandbox.

Functional testing and feedback from others would be very welcome here... or maybe others have a better solution? 🙂

@kajman-cz
Copy link

Is the final file (dist/flickity.pkgd.js) somewhere to test it?

@felixranesberger
Copy link

Are there any updates on this PR?
We have similar issues with CLS and Google PageSpeed.

@aaronstezycki
Copy link

Im not quite sure what the options that are being used here for Flickity... but I'm pretty sure that out of the box, the current version uses transforms instead of position absolute left/right. @desandro Can confirm this?

I personally think the CLS issues are due to Flickity removing the DOM on init, and then adding the nodes <div class=flickity-viewport> & <div class=flickity-slider> to the DOM, and then re-adding the slider items back in. As of yet, the only way to fix this, is to add a height to slider div so it doesnt jump around on init. There's another discussion around this here.

#1087

Should be given top priority, as this bug will cause websites to loose rankings as CLS is now contributing factor to search index positions.

@redjam13 redjam13 changed the title fix: use transform over left/right to eradicate CLS fix: use transform over left/right to eradicate CLS on 'wrap around' Apr 19, 2021
@redjam13
Copy link
Contributor Author

redjam13 commented Apr 19, 2021

Thanks for your comment @aaronstezycki


Im not quite sure what the options that are being used here for Flickity... but I'm pretty sure that out of the box, the current version uses transforms instead of position absolute left/right.

If you look at the diff of this PR, you will see that the renderPosition function is currently manipulating the left and right value (i.e. the deleted lines). Example.


I personally think the CLS issues are due to Flickity removing the DOM on init

I imagine different types of usage and context are causing layout shift in different ways. This particular PR is addressing the 'cumulative' problem as opposed to any issue on initialization. For example, a Flickity instance with options autoPlay: true and wrapAround: true will cumulatively increase the CLS score relative to the length of time the offending carousel is open - which can feasibly result in a CLS score being recorded in double digits.

If there are issues with layout shift on initialization, which cannot be addressed by suggestions made by e.g. @desandro and @LimeWub on #1087 then likely there is a need for another PR to fix that as well.

@aaronstezycki
Copy link

@redjam13 Thanks for the clarification! It makes sense now, I didn't notice the application of position left on individual slider items! and had only noticed the slider transform values on the parent. I stand corrected! 👍

@desandro desandro merged commit 75fe5d7 into metafizzy:master Dec 19, 2021
@desandro
Copy link
Member

MERGED Thank you for this PR! Please email yo@metafizzy.co so I can send you some goodies. This code will be included in the next release.

desandro added a commit that referenced this pull request Dec 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Flickity results in significant Cumulative Layout Shift (CLS)
5 participants