metafizzy / flickity

:leaves: Touch, responsive, flickable carousels
https://flickity.metafizzy.co
7.52k stars 605 forks source link

Flickity results in significant Cumulative Layout Shift (CLS) #1087

Closed frankv12 closed 2 years ago

frankv12 commented 4 years ago

Perhaps unavoidably so, Flickity drags my site down with a significant Cumulative Layout Shift (CLS) score during Lighthouse and web.dev tests. Has anyone found a way around this?

ghost commented 3 years ago

How did you solved it?

codetot commented 3 years ago

I think he doesn't found any solution yet.

Here is our result from Google PageSpeed.

Screenshot_3
MLDMoritz commented 3 years ago

Problem is that flickity seems to remove the dom and then add it again, thus shifting the layout. Shouldn't be closed imho.

BMarques commented 3 years ago

I agree with MLDMoritz, this shouldn't be closed as it's causing CLS issues in our sites.

desandro commented 3 years ago

If layout shift is an issue for you, you can address it by manually setting your own carousel height by disabling setGallerySize

setGallerySize: false
/* carousel height */
.carousel {
  height: 160px;
}

/* cell inherit height from carousel */
.carousel-cell {
  height: 100%;
}
finex commented 3 years ago

If layout shift is an issue for you, you can address it by manually setting your own carousel height by disabling setGallerySize

setGallerySize: false
/* carousel height */
.carousel {
  height: 160px;
}

/* cell inherit height from carousel */
.carousel-cell {
  height: 100%;
}

Hi, would be possible to use "vh" on CSS and make the carousel content respect that height?

desandro commented 3 years ago

Hi, would be possible to use "vh" on CSS and make the carousel content respect that height?

Yes! You can set height on .carousel {} how you like, with vh, vw, px, or whatever 😄

kajman-cz commented 3 years ago

The tool Core Web Vitals (in Search Console or as Chrome plugin) reports very bad CLS even though it is invisible by human eyes if the wrapAround: true option is used on the carousel.

This is poorly tuned, as the plugin only reports a problem in 10% of cases. But Google is drawing attention to this problem.

Maybe with wrapAround option it might help to move the last items to the left during the first user interaction (click, tap) - then it shouldn't count in CLS (if the element movement is completed within 500 ms).

Edit: Test page (not on codepen - I can't make a test page there without an iframe.) screenshot of test page

Garnet-Fox commented 3 years ago

kajman-cz, it seems to be true. And that's a big problem. on mobiles where "autoPlay": false and "wrapAround": true, cls in Search Console = 0.02 on desktop where "autoPlay": true and "wrapAround": true, cls in Search Console red zone = 1 :(

kajman-cz commented 3 years ago

I had Flickity set "autoPlay": false and "wrapAround": true. In Google Search Console was CLS 0.35 on Desktop. After turning off wrapAround (no other change ) they fixed all the problem pages in a two weeks: CLS warning in Search Console

I think Flickity has done well. From the user's point of view, there is no shift. It looks more like a Chrome bug. I tried to report it, but they haven't been able to reproduce it yet.

redjam13 commented 3 years ago

@kajman-cz I don't think this is a Chrome bug - Flickity with ~autoPlay: true and~ wrapAround: true causes demonstrable layout shift in different browsers.

Don't forget that CLS scoring penalties apply to horizontal shift as well as vertical, which I believe is what we are seeing here.

I have adapted your screenshot into a gif, which I think demonstrates the problem quite clearly.

flickity

Notice that the left value is being manipulated without any user interaction, which is against the rules 🚓

This is a serious issue. From May 2021, websites using Flickity with these options are liable to see their Google search ranking tank as CLS becomes a page experience signal 📉

codetot commented 3 years ago

My suggestion is to remove autoPlay when init first time with Flickity, then when the user scrolls into a viewport that saw a slider, you will destroy and create it again with autoPlay is true. Not a full test, but my method seems to work.

You can try to see the result here: https://maychuviet.vn

SimonMayerhofer commented 3 years ago

Problem is that flickity seems to remove the dom and then add it again, thus shifting the layout. Shouldn't be closed imho.

I think this is a problem which is unaddressed here so far. This is one of our major problems which optimizing. We need to add wrappers with min-height around every flickity slider because otherwise it gets removed and added again and result in a significant layout shift.

LimeWub commented 3 years ago

Problem is that flickity seems to remove the dom and then add it again, thus shifting the layout. Shouldn't be closed imho.

I think this is a problem which is unaddressed here so far.

Are you saying flickity is editing (adding/removing) the child DOM elements? I'm working on a fix for this and I haven't noticed child elements being added/removed. Instead what has been triggering layout shift in my experience (wrap-around examples) has been changing the left position on the elements. AFAIK the elements in the DOM stay the same, it's the positioning of them that changes with the left attribute and causes CLS.

Unless you are talking about flickity initialisation where it adds the wrapper and whatnot?

SimonMayerhofer commented 3 years ago

Unless you are talking about flickity initialisation where it adds the wrapper and whatnot?

this. I managed to get a recording of the behaviour:

Screen Recording

LimeWub commented 3 years ago

@SimonMayerhofer This is not was I was working to fix, sorry.

However, I am pretty sure I had encountered this before and managed to fix with CSS - I don't remember the exact solution but I see this in our code which could be relevant:

.flickity-slider {
        display: flex;
        position: relative; // (*) flickity resize bug part fix
        height: auto; // (*)
    }

I think the problem you are seeing is the carousel calculating a wrong height when if first starts up but I believe you can fix this with CSS ^^ Good luck

SimonMayerhofer commented 3 years ago

I think the problem you are seeing is the carousel calculating a wrong height when if first starts up but I believe you can fix this with CSS ^^ Good luck

Thanks, we always fix it with something like

.container {
    width: 100%;
    display: flex;
    overflow-x: auto;
    min-height: 300px;

    &.flickity-enabled {
        display: block;
        overflow-x: visible;
    }
}

But this still leads to layout shifts inside the container.

aaronstezycki commented 3 years ago

Any update to this... May is fast approaching (when Google launches its page experience update) and this could do with fixing as a priority.

I think that @SimonMayerhofer has identified the problem, it's definitely the DOM manipulation that occurs on init. The way I have fixed this, is by adding an explicit height in the css to the slider parent node. However this isn't great practise, as most sliders (especially Flickity) have been built to allow variable height thats dependant on the content within the slider i.e.: if you've used adaptiveHeight: True.

What would be ideal, or better way of doing this, is getting the height of the DOM node it transforms and setting that height before Flickity has done anything, that way the height is maintained throughout initialisation, and there is no CLS. However this means the slider component needs to be the same size un-initialised as it is when Flickity has loaded and init.

kajman-cz commented 3 years ago

I tried example after some time and the new chrome version already calculates the CLS only very low.

goranhorvathr commented 1 year ago

This is not addressed and closed unnecessary without a valid solution. First of all this is happening because timing between the Init of the slider and the dom load is different which is natural and how web is working. For that brief moment the slider is not initialized and the elements take natural height in their container. There are several solutions for this and this one https://github.com/metafizzy/flickity/issues/1087#issuecomment-750383838 is not one of them for me at least. We want to use the autoHeight and we do not want to hardcode the height of the container nor the elements.

The solution for this is pretty simple and effective.

If we have div elements and structure like this

<div class="main-carousel flex">
  <div class="carousel-cell">...</div>
  <div class="carousel-cell">...</div>
  <div class="carousel-cell">...</div>
</div>

FLEX here will automatically assign correct width and height to the div you are using however you need to remove this when the slider initializes because its not gonna function correctly since slider js adds additional elements and stylings do not stack.

var elem = document.querySelector(".main-carousel");
var flkty = new Flickity(elem, {
  cellAlign: "left",
  contain: true,
  prevNextButtons: false,
  on: {
    ready: function () {
      this.element.classList.remove("flex");
      this.resize();
    },
  },
});

Ready fires immediately we remove the flex that we used to fix our CLS and force a resize of the slider to correctly initialize. While this can work https://github.com/metafizzy/flickity/issues/1087#issuecomment-750383838 to properly do it you need to have minimum of 4-5 media queries.

Float left, inline block, flex, grid whatever should have been implemented for that brief period of time between the dom load & initialization of the slider and all Javascript work that is being done to avoid content jump and CLS

william-keller commented 1 year ago

I am happy to announce that i could solve my CLS problem doing this:

image

image

image

image

jsfgreen commented 4 months ago

I've been using this for a while:

.flickity-carousel:not(.flickity-enabled) {
  overflow: hidden;
  .flickity-slider {
    display: flex;
    flex-wrap: nowrap;
  }
  .carousel-cell {
    visibility: hidden;
  }
}

But honestly lighthouse isn't reporting much CLS with flickity anymore even without this hack. This is more for actual optics, I mean it's not just about appeasing lighthouse/insights it's also about what's pleasing to the eye. I use this trick because the sudden flicker that's visible to the naked eye is kind of jarring and this [imperfect but] pure CSS snippet has helped with that.