amp-carousel scrolls to weird position by prev/next button ?

ka2jun8 commented 3 years ago

What's the issue?

We use amp-carousel (type carousel) for displaying multiple components horizontally. When amp-carousel-button-next is clicked, it is scrolled the same as width of carousel component (i-amphtml-carousel-scroll) to right. On the other hand, if the carousel component width and carousel item width are different, carousel moving position seems weird.

Before click next button
After click next button

The moveScrollOneViewport_ function is using el./*OK*/ scrollLeft += el./*OK*/ offsetWidth * forwardsMultiplier * directionMulitplier;.

https://github.com/ampproject/amphtml/blob/master/extensions/amp-carousel/0.2/amp-carousel.js#L315-L316

I'd like to modify to use this.currentIndex_ like this.goToSlide(this.currentIndex_) ?

How do we reproduce the issue?

Open amp.dev playground (https://playground.amp.dev/).
Paste this code.

<!doctype html>
<html ⚡ lang="en">
<head>
<meta charset="utf-8">
  <title>amp-carousel</title>
  <script async src="https://cdn.ampproject.org/v0.js"></script>
  <!-- ## Setup -->
  <!-- Import the carousel component in the header. -->
  <script async custom-element="amp-carousel" src="https://cdn.ampproject.org/v0/amp-carousel-0.2.js"></script>
  <link rel="canonical" href="https://amp.dev/documentation/examples/components/amp-carousel/index.html">
  <meta name="viewport" content="width=device-width">
  <style amp-boilerplate>body{-webkit-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-moz-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-ms-animation:-amp-start 8s steps(1,end) 0s 1 normal both;animation:-amp-start 8s steps(1,end) 0s 1 normal both}@-webkit-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-moz-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-ms-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-o-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}</style><noscript><style amp-boilerplate>body{-webkit-animation:none;-moz-animation:none;-ms-animation:none;animation:none}</style></noscript>
  <style amp-custom>
    .blue-box, .red-box, .green-box {
      width: 100%;
      height: 280px;
    }
    .blue-box {
      background: blue;
      color: #fff;
    }
    .green-box {
      background: green;
      color: #fff;
    }
    .red-box {
      background: red;
      color: #111;
    }
    #custom-button .amp-carousel-button-prev {
      left: 5%;
      background-image: url('https://amp.dev/static/samples/img/carousel-arrow-left.png');
    }
    #custom-button .amp-carousel-button-next {
      right: 5%;
      background-image: url('https://amp.dev/static/samples/img/carousel-arrow-right.png');
    }
  </style>
</head>
<body>
  <amp-carousel height="300" layout="fixed-height" type="carousel" role="region" aria-label="Basic usage carousel">
    <amp-img src="https://i.picsum.photos/id/100/300/200.jpg?hmac=ONiK6jtlF-h6EIOqhPFFyhyjF3SFqQ10MYWslZmlNyI" width="280" height="300" alt="a sample image"></amp-img>
    <amp-img src="https://i.picsum.photos/id/100/300/200.jpg?hmac=ONiK6jtlF-h6EIOqhPFFyhyjF3SFqQ10MYWslZmlNyI" width="280" height="300" alt="another sample image"></amp-img>
    <amp-img src="https://i.picsum.photos/id/100/300/200.jpg?hmac=ONiK6jtlF-h6EIOqhPFFyhyjF3SFqQ10MYWslZmlNyI" width="280" height="300" alt="and another sample image"></amp-img>
  </amp-carousel>
</body></html>

Click amp-carousel next button.
Image is shown on weird position.

What browsers are affected?

All browsers

Which AMP version is affected?

Recently version AMP
amp-carousel v0.2

micajuine-ho commented 3 years ago

Hi @ka2jun8. Thank you for your bug report. amp-carousel type=carousel slides the carousel by 1 viewport when the prev/next button is clicked. This is the intended behavior. If you'd like there to always be one slide shown in the carousel, you can use type=slides. Additionally, if you'd like to have multiple slides in view on a carousel, you can try <amp-base-carousel visible-count="2">. See test/manual/amp-base-carousel/grouping-autoadvance.amp.html as an example and https://amp.dev/documentation/components/amp-base-carousel/?format=websites#visible-count for documentation.

kristoferbaxter commented 3 years ago

Closing this issue since this behaviour is as specified. Feel free to comment again with other thoughts and we can reopen if needed.

ka2jun8 commented 3 years ago

@micajuine-ho

Thank you for your advices :)

you can use type=slides

Sorry, I think our use case is not like that case. I want to display a next carousel image a little bit because it causes to be interested for users.

However this is a hint for me, Because the option visible-count can be set a decimal number like visible-count="1.5".

I had no idea to use amp-base-carousel, instead of amp-carousel. I think it is useful for me, thank you!

ampproject / amphtml