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

✨ [amp-animation] Adds a new attr that allows to override pan scaling factor. #37965

Conversation

jingfei
Copy link
Contributor

@jingfei jingfei commented Mar 26, 2022

Currently amp-story animation preset scales the pan effect image by checking if the target is smaller than the story page; however, for the target that is not designed as full-bleed, it causes an unexpected scaling. For example, a small image in the story page with pan- animation might be scaled to 2x while the expectation is not to scale.

The PR provides a new attribution, pan-static-scale, that allows the pan animation to override the scaling factor with a static value, and it would fixes #37932.

/cc @newmuis

@amp-owners-bot amp-owners-bot bot requested a review from Enriqe March 26, 2022 17:02
@amp-owners-bot
Copy link

amp-owners-bot bot commented Mar 26, 2022

Hey @gmajoulet, @newmuis! These files were changed:

extensions/amp-story/1.0/animation-presets.js
extensions/amp-story/1.0/test/test-full-bleed-animations.js
extensions/amp-story/1.0/test/validator-amp-story-animations.html
extensions/amp-story/1.0/test/validator-amp-story-animations.out
extensions/amp-story/amp-story.md
extensions/amp-story/validator-amp-story.protoascii

Hey @ampproject/wg-caching! These files were changed:

extensions/amp-story/1.0/test/validator-amp-story-animations.html
extensions/amp-story/1.0/test/validator-amp-story-animations.out
extensions/amp-story/validator-amp-story.protoascii

@gmajoulet gmajoulet requested review from gmajoulet and removed request for Enriqe March 28, 2022 18:09
Copy link
Contributor

@gmajoulet gmajoulet left a comment

Choose a reason for hiding this comment

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

Thank you for opening a PR! Could you post an example where the image is scaled 2x? I'd like to understand how bad the issue can get.
The one thing I'm worried about is that most publishers have historically not been able to test their layouts/animations on enough devices to get them to be always displayed correctly. I'm afraid for us to open this setting and publishers getting it wrong for some devices.

extensions/amp-story/1.0/animation-presets.js Outdated Show resolved Hide resolved
@jingfei
Copy link
Contributor Author

jingfei commented Mar 29, 2022

Sure no problem. As long as the image is not full-bleed it will be scaled to fit the story page if pan effect is added.

Here's a quick example, the 3 pages in the story are the same except for pan animation

  • page 1: no animation
  • page 2: pan-right effect --> it scales the image to be same as the page width/height, which is not expected in our use case
  • page 3: zoom-in effect --> as the scaling factor is provided by user, it works as expected in our use case.

With the new added attr, it's able to be animated with pan effect and custom scaling factor.

Let me know if you have questions, happy to discuss more in details. Thank you.

<!doctype html>
<html  lang="en">
  <head>
    <meta charset="utf-8">
    <meta name="viewport" content="width=device-width">
    <meta name="description" content="This is the AMP Boilerplate.">
    <link rel="preload" as="script" href="https://cdn.ampproject.org/v0.js">
    <link rel="preload" as="script" href="https://cdn.ampproject.org/v0/amp-story-1.0.js">
    <script async src="https://cdn.ampproject.org/v0.js"></script>
    <script async custom-element="amp-story" src="https://cdn.ampproject.org/v0/amp-story-1.0.js"></script>
    <!-- Import other AMP Extensions here -->
    <style amp-custom>
    /* Add your styles here */
    </style>
    <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>
    <link rel="canonical" href=".">
    <title>My AMP Page</title>
  </head>
  <body>
    <amp-story standalone
               publisher="The Publisher"
               title="My Story"
               publisher-logo-src="logo.png"
               poster-portrait-src="poster-portrait.png"
               poster-square-src="poster-square.png"
               poster-landscape-src="poster-landscape.png">
      <amp-story-page id="page1">
        <amp-story-grid-layer template="vertical">
          <h1>Hello World</h1>
          <div style="width:420px;height:315px;overflow:hidden">
            <amp-img layout="responsive" height="960" width="1280" src="https://upload.wikimedia.org/wikipedia/commons/thumb/b/b6/Image_created_with_a_mobile_phone.png/1280px-Image_created_with_a_mobile_phone.png" />
          </div>
        </amp-story-grid-layer>
      </amp-story-page>
      <amp-story-page id="page2">
        <amp-story-grid-layer template="vertical">
          <h1>Hello Animation</h1>
          <div style="width:420px;height:315px;overflow:hidden">
            <amp-img animate-in="pan-right" layout="responsive" height="960" width="1280" src="https://upload.wikimedia.org/wikipedia/commons/thumb/b/b6/Image_created_with_a_mobile_phone.png/1280px-Image_created_with_a_mobile_phone.png" />
          </div>
        </amp-story-grid-layer>
      </amp-story-page>
      <amp-story-page id="page3">
        <amp-story-grid-layer template="vertical">
          <div style="width:420px;height:315px;overflow:hidden">
          <amp-img animate-in="zoom-in" layout="responsive" height="960" width="1280" src="https://upload.wikimedia.org/wikipedia/commons/thumb/b/b6/Image_created_with_a_mobile_phone.png/1280px-Image_created_with_a_mobile_phone.png" />
          </div>
        </amp-story-grid-layer>
      </amp-story-page>
      <amp-story-bookend src="bookend.json" layout=nodisplay></amp-story-bookend>
    </amp-story>
  </body>
</html>

@jingfei jingfei force-pushed the jingfeiyang-allow-to-override-animation-pan-scaling branch from ad52c24 to f0cdc8f Compare March 29, 2022 12:11
@gmajoulet
Copy link
Contributor

cc @mszylkowski can you make sure this is the right fix to this problem and review? Thank you!

@gmajoulet
Copy link
Contributor

Also, please don't forget to udpate the validation rules so you can add the attribute without making your stories invalid:

attrs: {
name: "animate-in"
value: "drop"
value: "fade-in"
value: "fly-in-bottom"
value: "fly-in-left"
value: "fly-in-right"
value: "fly-in-top"
value: "pan-down"
value: "pan-left"
value: "pan-right"
value: "pan-up"
value: "pulse"
value: "rotate-in-left"
value: "rotate-in-right"
value: "scale-fade-down"
value: "scale-fade-up"
value: "twirl-in"
value: "whoosh-in-left"
value: "whoosh-in-right"
value: "zoom-in"
value: "zoom-out"
}
attrs: { name: "animate-in-after" }
attrs: { name: "animate-in-delay" }
attrs: { name: "animate-in-duration" }
attrs: { name: "animate-in-timing-function" }

@jingfei
Copy link
Contributor Author

jingfei commented Mar 31, 2022

No problem! Validation rule added.
Thanks for the reminder.

Copy link
Contributor

@mszylkowski mszylkowski left a comment

Choose a reason for hiding this comment

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

I think Gabriel commented before, but it's super hard for creators to know how this attribute will behave for different page sizes (it's likely that 90% of that comes from how hard it is to understand the current logic).

There's an issue with this logic, which is that the images by default have pixel sizes (the value on width and height) and the scaling factor helps to correct that, although it's really bad at it (it's even harder with different layout types).

The correct way to fix this would be to check the size of the container instead of the page, so that smaller containers will not require such large zoom levels. The fast way to fix this is to multiply the scaling factor calculated by the attribute that you want to pass here, so that there's still smarter scaling, but you can specify the "ratio" that you want to use (eg: 0.5 for an element that occupies 50% of the page)

extensions/amp-story/1.0/animation-presets.js Outdated Show resolved Hide resolved
@jingfei
Copy link
Contributor Author

jingfei commented Apr 3, 2022

As discussed with @mszylkowski offline, will keep the CL as-is to apply to our use case. At the same time, no further CSS change (e.g. to remove full-bleed CSS) will make to reduce the additional complication.

Also, in the latest changes, I updated the validation rules with tests.

PTAL. Thank you.

Copy link
Contributor

@mszylkowski mszylkowski left a comment

Choose a reason for hiding this comment

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

In other areas of the codebase we use the term scaling-factor for these types of things, it would make more sense to call the attribute pan-scaling-factor for consistency. Otherwise looks good.

@jingfei
Copy link
Contributor Author

jingfei commented Apr 4, 2022

Sure, updated the attr name to pan-scaling-factor.

@jingfei jingfei force-pushed the jingfeiyang-allow-to-override-animation-pan-scaling branch from dba9c89 to 5db8c8e Compare April 4, 2022 16:22
Copy link
Contributor

@MichaelRybak MichaelRybak left a comment

Choose a reason for hiding this comment

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

Approving validator changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[amp-story] Provide an option in animation preset to disable image scaling for pan effects.
5 participants