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.

Sign up for GitHub

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

Jump to bottom

Rendered A4A ad is never assigned the "amp-animate" CSS class #7846

Closed
lexandera opened this issue Feb 28, 2017 · 29 comments
Closed

Rendered A4A ad is never assigned the "amp-animate" CSS class #7846

lexandera opened this issue Feb 28, 2017 · 29 comments
Labels
Component: A4A fast fetch P2: Soon Stale Inactive for one year or more Type: Bug WG: monetization
Projects
Ads & Analytics issue triaging
Milestone
Backlog Bugs

Comments

@lexandera
Copy link

I managed to successfully serve an a4a ad using Cloudflare's network implementation. The ad validates and renders in a friendly iframe, but animations never start because in a4a they only run when the "amp-animate" CSS class is present, and this class is never added by amp runtime.

Searching through amphtml code base reveals that the string "amp-animate" does not appear outside of a4a validator code. Has adding of this class to rendered a4a creatives not been implemented yet?
@jasti
Copy link
Contributor

jasti commented Mar 1, 2017

to @dvoytenko
CC @ampproject/a4a

@jasti jasti added this to the Backlog Bugs milestone Mar 1, 2017
@jasti jasti added the Type: Bug label Mar 1, 2017
@jasti jasti modified the milestones: Pending Triage, Backlog Bugs Mar 1, 2017
@lexandera
Copy link
Author

What is not clear from the a4a spec is whether the amp-animate class should be added by amp at runtime once the ad has been loaded, or if this class should already be present in the ad html and the runtime just has the option of removing it if it wants to stop animations. It could be just my misunderstanding of intended behavior.

@cramforce
Copy link
Member

Lets clarify in the spec that this SHOULD be present in the markup.

@lannka While we don't yet remove this, we should at least add it.

@lannka
Copy link
Contributor

lannka commented Mar 2, 2017

@cramforce on which element do we recommend to put this amp-animate class? Actually wouldn't it be better for runtime to insert amp-animate to body element when the creative is ready to animate?

@cramforce
Copy link
Member

cramforce commented Mar 2, 2017 via email

@lannka lannka modified the milestones: New FRs, Pending Triage Mar 14, 2017
@lannka lannka assigned alanorozco and unassigned dvoytenko and jasti Mar 24, 2017
@lannka lannka modified the milestones: Sprint H1 April, New FRs Mar 24, 2017
@lannka lannka added this to Sprint Candidate in AMP Advertising Mar 24, 2017
@lannka
Copy link
Contributor

lannka commented Mar 24, 2017

@cramforce to let AMP have full control over what and when to animate, shall we actually enforce "no amp-animate class in any elements" in the document?

@dvoytenko
Copy link
Contributor

An alternative concept:

  1. We can start (e.g. via bolierplate or re-serialization) with always setting * {animaton-name: none !important}. When ready, this CSS will be removed by AMP Runtime.
  2. Similarly, if we wanted to pause all animations at any time (as opposed to reset), we can set * {animation-play-state: paused} and remove it to continue.

@cramforce
Copy link
Member

We should switch to the concept recommended by @dvoytenko. This is just way better.

@lannka lannka mentioned this issue Mar 30, 2017
Closed
@alanorozco
Copy link
Member

@aghassemi mentioned that setting animation-play-state: paused won't work on iOS when also setting -webkit-overflow-scrolling: touch (which we always do).

See this example, which works properly everywhere but on iOS.

@alanorozco alanorozco mentioned this issue Apr 10, 2017
Closed
@lannka lannka modified the milestones: Sprint H1 August, Backlog Bugs Aug 7, 2017
@lannka
Copy link
Contributor

lannka commented Aug 7, 2017

@alanorozco @dvoytenko do you want to summarize the conclusion and action items here?

@dvoytenko
Copy link
Contributor

@alanorozco Will test couple more situations. But we are leaning toward wild-card control that's completely automatic.

@alanorozco
Copy link
Member

alanorozco commented Aug 14, 2017 •
edited

Apologies for moving slowly on this. As this change involves low-level changes to the runtime, we've been trying to make this decision with care as not to affect usage or performance in a significant way.

We ran benchmarks on the performance of three different types of lock selectors. The methodology for the benchmarks is described here and a full comparison table is here.

We've decided to go for a wildcard lock. This means that no special considerations (like marking elements with a specific classname) need to be made from the document author's side. The validator does not yet reflect this, so it will need to be updated before CSS animations are fully supported.

@dvoytenko
Copy link
Contributor

🥇

@jasti
Copy link
Contributor

jasti commented Sep 22, 2017

@alanorozco would you like to close this and create a new issue for validator changes?
Also, are there any runtime changes for this decision?

sklobovskaya added a commit to sklobovskaya/amphtml that referenced this issue Sep 25, 2017
@sklobovskaya
Added ampproject#7846 ref in runtime enable TODOs.
30996a4
@zhucl
Copy link
Contributor

zhucl commented Sep 27, 2017

May I ask when the validator change will happen?

@jasti
Copy link
Contributor

jasti commented Sep 27, 2017

To @alanorozco

dvoytenko pushed a commit that referenced this issue Oct 3, 2017
@sklobovskaya
amp-gwd-animation 0.1 (initial) (#11122)
1cda17b 
* Initial commit.

* Perform animationend listener registration only once (for easier debugging).

(cherry picked from commit 8cf7cab2b09b5ac3ea0216266e69e05a3d5f9983)

* Simplify enable/disable.

(cherry picked from commit d5c2343ef4642d9e828ebb2faf9c81bfbb3cd321)

* Send original event detail with triggered timeline event.

(cherry picked from commit 7f6463eda5ca2cd6cf9a2cf2b340bd67c885817d)

* Missed test updates for timeline events.

* Sync/updates with upstream. gotoAndPause bug fix.

* Added attr spec to validator rules.

* Removing initial setEnabled(false) call, pending integration.

* Minor style cleanup in test.

* Added experiment flag.

(cherry picked from commit 7c1426bc0f6c131cdfea7cfba01b3b0cdf9cd3d0)

* Renamed imported constants.

(cherry picked from commit ce42c0c02478795851c70232c00a54ea60565726)

* Misc. cleanup for PR.

* Moved validator rules up one directory.

* Comment/spacing fixes.

* Fixed linter errors.

* Removed protoascii file for now, pending tests.

* Reverted package metadata file changes.

* Added spec and cleanup issue to experiment metadata.

* Unit test improvements and cleanup.

* amp-gwd-animation style and misc. cleanup. Dispatch runtime events on root node instead of win.

* Whitelisted amp-gwd-animation for amp4ads and added validator rules file.

* Disable event animations too.

* Use AMP.registerServiceForDoc to register the amp-gwd-animation service.

* Added ActionService.setActions method. Misc. cleanup.

* Fixed flaky gotoAndPause unit test.

* Fixed linter error.

* Added #7846 ref in runtime enable TODOs.
zhucl added a commit to zhucl/amphtml that referenced this issue Oct 19, 2017
@zhucl
Update the a4a spec to require amp-animation based on
02eb787 
ampproject#7846 (comment)
@zhucl zhucl mentioned this issue Oct 20, 2017
Merged
@jasti jasti moved this from Current Sprint to Blocked in AMP Advertising Oct 27, 2017
@Gregable
Copy link
Member

Gregable commented Nov 8, 2017

Validator changes are live.

@ampprojectbot
Copy link
Member

This issue hasn't been updated in awhile. @alanorozco Do you have any updates?

@lannka lannka mentioned this issue May 10, 2018
Closed
2 tasks
@robmurtagh
Copy link

Hi, I'm seeing an issue with A4A animations not running on iOS (#15260) . Can't quite work out whether it is related to this issue, so thought I'd flag it. Thanks

@ampprojectbot
Copy link
Member

This issue hasn't been updated in awhile. @alanorozco Do you have any updates?

@lannka lannka added this to Needs triage in Ads & Analytics issue triaging via automation Jul 2, 2019
@lannka lannka removed this from Blocked in AMP Advertising Jul 2, 2019
@lannka lannka moved this from Needs triage to Triaged in Ads & Analytics issue triaging Jul 2, 2019
@stale
Copy link

stale bot commented Dec 23, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the Stale Inactive for one year or more label Dec 23, 2020
@stale stale bot closed this as completed Dec 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Component: A4A fast fetch P2: Soon Stale Inactive for one year or more Type: Bug WG: monetization
Projects
No open projects
Ads & Analytics issue triaging
  
Triaged
Milestone
Backlog Bugs
Development

No branches or pull requests