Amp-wp: Enable user to create combination animations

[MackenzieHartung]

Feature description

As a user I want to be able to enhance my animations to be more complex than what exists in the editor today. I would like the ability to add combination animations.

---

_Do not alter or remove anything below. The following sections will be managed by moderators only._

Acceptance criteria

• AC1: Implement the, already supported, combination animation within the stories editor.

Implementation brief

General overview:

Combining multiple animations means being able to add more than one animation to a single block. Since not all animations go together well then the additional animation should be added based on that. We can design the solution so that it would allow adding more than one additional animation, however, for making it simpler and cleaner and easier to test with users, we can initially allow just one additional animation.

For avoiding needing to deprecate all blocks supporting animation, the implementation should ensure that for previously saved blocks with animation there is no changing in saving to the post content.

Combined animations will share the "Begin After" value, however, they will have their own Delay and Duration.

Allowed combinations

• Any animation combinations are allowed (except for combining two identical combinations, since that doesn't seem to have any effect). E.g. combining drop with drop.

User Interface (in words only)

If a user has chosen Animation for a block, a button "Add a second Animation" appears under "Delay". When choosing an available second animation, "Duration" and "Delay" will appear for the second animation.

Technical details

A new block setting animations will be replacing the current ampAnimationType, ampAnimationDuration, ampAnimationDelay, ampAnimationAfter settings.

For avoiding deprecating all the existing blocks, we will need to add migration logic which would check if the previous settings are set. If yes, we should migrate it to the new settings, and remove the value from the previous settings.

The animations setting will be an array of animations (objects). Initially, it will include only up to two animations.

Each item in the array will include type and delay, duration, beginAfter if applicable.

Based on the documentation, then combining multiple animations can be done by wrapping elements, for example:

<div animate-in="fly-in-left">
   <div animate-in="fade-in">
     I will fly-in and fade-in!
   </div>
</div>

This can be done in wrap-block-in-grid-layer filter and the additional animations will be added as child elements for .amp-story-block-wrapper, the "main" animation will be wrapping the other animation(s).

For being able to preview the animations in the editor, we can use with-wrapper-props filter and add a child element to .amp-page-child-block, if a second animation is chosen.

QA testing instructions

Demo

Changelog entry

[miina]

Note: Starting looking into the Implementation.

[miina]

I've added the initial Implementation Brief. Some questions/thoughts that I had while looking into this:

1. If you look at the implementation brief you'll see that I've mentioned that combined animations shouldn't allow combining any animations. Any thoughts on this? I was testing a little bit and although initially I thought that perhaps it wouldn't make sense to combine animations such as drop and fly-in-left but as it came out, then with using delay it might make sense. For example:

1. For simplicity and for initial "round", I thought that perhaps we can start with adding just a second animation for combining. Then we can do some user testing and detect if more could be useful. Thoughts?

Implementation is ready for review.

cc @barklund @spacedmonkey @swissspidy

[barklund]

For avoiding needing to deprecate all blocks supporting animation, the implementation should ensure that for previously saved blocks with animation there is no changing in saving to the post content.

For future proofing this, can we make sure it will also smoothly allow moving to 3+ animations?

Combined animations will share the "Begin After" value, however, they will have their own Delay and Duration.

Why? Let's say you have three images on a page. Maybe you want them all to fade in on page load, and then later zoom up individually after each other?

Can't we copy the whole logic including "Begin after" for the extra animation? And then in the "Begin after "dropdown have "Image 1, Animation 1 (Fade)", "Image 1, Animation 2 (fly-in)"?

Also, you'd often want to chain the two animations directly, so it should at least be ease to say "begin animation 2 after animation 1" on the individual element.

Allowed combinations

• If a similar type of animation has already been assigned, it should not be allowed as a second animation. For example, if fly-in-left has been assigned then any other flying-in animations shouldn't be allowed.
• Pan and Zoom animations can only be combined with fade-in / pulse
• No animation can be combined with itself.

Do we need to add any such restrictions? Wouldn't it be simpler to just allow everything and let users police themselves? If it looks weird, they're probably not going to do it?

Technical details

For avoiding deprecating all the existing blocks, we will need to create a new block setting in addition to ampAnimation* settings.

The new setting will be combinedAnimations which is an array of animations. Initially it will include only a second animation.

Wouldn't animations make more sense? And then if there is a single animation from earlier, move it here as a sort of migration (I don't know how migrations work in this context)?

For being able to preview the animations in the editor, we can use with-wrapper-props filter and add a child element to .amp-page-child-block, if a second animation is chosen.

👍

If you look at the implementation brief you'll see that I've mentioned that combined animations shouldn't allow combining any animations. Any thoughts on this? I was testing a little bit and although initially I thought that perhaps it wouldn't make sense to combine animations such as drop and fly-in-left but as it came out, then with using delay it might make sense. For example:

As mentioned, I don't want to police editors like that. They might want to make something really weird, and that's okay. Or they are able to combine things to create something beautiful, that you find impossible and have blocked.

For simplicity and for initial "round", I thought that perhaps we can start with adding just a second animation for combining. Then we can do some user testing and detect if more could be useful. Thoughts?

I'm good with that as long as it's both backwards and forwards compatible.

[miina]

Combined animations will share the "Begin After" value, however, they will have their own Delay and Duration.

Why? Let's say you have three images on a page. Maybe you want them all to fade in on page load, and then later zoom up individually after each other?
Can't we copy the whole logic including "Begin after" for the extra animation? And then in the "Begin after "dropdown have "Image 1, Animation 1 (Fade)", "Image 1, Animation 2 (fly-in)"?
Also, you'd often want to chain the two animations directly, so it should at least be ease to say "begin animation 2 after animation 1" on the individual element.

The effect of starting one animation after the other could be done with adding a delay as well. Having "Begin animation 2 after 1" option might be a good idea though!

Note that combining two animations with "Begin after" might get confusing since the animations are actually applied on two different elements. An example:
if I have an image and it has 1st animation set to fly-in and the 2nd animation to fade-in with the 2nd animation beginning after the 1st, then it would mean that at first, the wrapping element would "fly in" but since the image inside it hasn't faded in yet then we wouldn't actually see the fly-in at all and would only see the fade-in. This makes the "Begin after" option not straightforward and the user would have to _know_ the logic of it.

A visual example of this code:

Although at first it "drops" we can't see that since the inner blue square which is the actual content hasn't done its "twirl-in" yet and is hidden.

So in conclusion: I'd be careful with the "Begin after" for combined animations not to cause a bad/confusing user experience. Thoughts?

Do we need to add any such restrictions? Wouldn't it be simpler to just allow everything and let users police themselves? If it looks weird, they're probably not going to do it?

That was my question as well. Generally when discussing issues the feeling that I've gotten is that we're trying to reduce the "horrible user experience" options as much as possible. However, I'm really not an expert on creating animations so it's likely that someone could do cool things with it.
It's OK to me to restrict just having a duplicate as the second animation -- this doesn't seem to have an effect based on testing.

Wouldn't animations make more sense? And then if there is a single animation from earlier, move it here as a sort of migration (I don't know how migrations work in this context)?

Hmmm... This would mean that we would have the previous settings + the new animations. We could migrate the values to animations, then we'd just always have to check if the previous values exist for either delay or type or duration. Looks like we could even remove the settings from addAMPAttributes since they're saved separately and not mapped against the attributes in the post content. We could try that, we'd just have to make sure that the migration would work correctly with previous versions as well, e.g. with deprecated filters, that would need some testing. Should work though 🤷‍♀

[spacedmonkey]

Regarding, limits on animations.
I think it is unwise for us to allow users to do whatever they want here. It has a number of drawbacks.

• It allows many more edge cases to come into the code, making it much to write, maintain and debug.
• It adds much more overhead to testing. Both QA and E2E tests would have to have a wide range of tests scripts in place to test all the options.
  -What does an element with 15 animations look like. The UI would have to support this and we may have to work on some sort of timeline feature, to keep it clear for users.

A think 3 animations is more than enough for each block. These keep this as an MVP, enable mulitple animations, see if users are using multiple animations and then expand it later.

As for policing types of animations, I am not sure about this. What if you wanted a element to fly in, be on page for 5 seconds and then fly out. I dont think it will look good, but there is no accounting for taste.

[miina]

A think 3 animations is more than enough for each block. These keep this as an MVP, enable mulitple animations, see if users are using multiple animations and then expand it later.

If you go through the Implementation Brief / comments again then you'll see that the discussion was about allowing just 2 animations for now per block to keep it simple, designing it to allow more but allow just 2.

As for policing types of animations, I am not sure about this. What if you wanted a element to fly in, be on page for 5 seconds and then fly out. I dont think it will look good, but there is no accounting for taste.

This is not actually possible at all. Please see the previous comment with a visual example: https://github.com/ampproject/amp-wp/issues/3421#issuecomment-540651783 And specifically the part that explains how "Begin after" might not make sense for combined animations. Since the animation parameters are set to different elements then if you set the animations after each other with delay, and if the last (inner) animation includes the block not being visible at first, only the last animation will be seen at all.

[spacedmonkey]

If you go through the Implementation Brief / comments again then you'll see that the discussion was about allowing just 2 animations for now per block to keep it simple, designing it to allow more but allow just 2.

Of course I read the above brief. I discussion seems limiting between 2 but thinking about supporting 3+ in the future

For future proofing this, can we make sure it will also smoothly allow moving to 3+ animations?

My point was just saying, I don't believe we should ever support more than 3 animations on a block.

As for policing types of animations, I am not sure about this. What if you wanted a element to fly in, be on page for 5 seconds and then fly out. I dont think it will look good, but there is no accounting for taste.

I was simply making a point that I agree with @barklund that we shouldn't police types of animations as you commented here,

For example, if fly-in-left has been assigned then any other flying-in animations shouldn't be allowed.

I am finding it hard to comment on these implementation briefs. This would be much easier as a pull request with comments on a per line basis and not have to quote all the time.

[miina]

Of course I read the above brief. I discussion seems limiting between 2 but thinking about supporting 3+ in the future

Thanks for additional explanation, makes sense. Agreed that likely we won't need a lot of animations for a single block, especially since more often than not, the animations don't work together and adding an extra animation might hide the previous animations unless knowing what doing.

I was simply making a point that I agree with @barklund that we shouldn't police types of animations as you commented here,

Yep, thanks for your opinion, I don't have a strong preference for this and although I added it to the brief then also added a question for second opinions -- looks like so far it's more favorable to allow any kind of animations.

Will update the Brief, too.

[miina]

@barklund / @spacedmonkey I've updated the Brief based on your thoughts.

For now I still did stick sharing one beginAfter setting per block since:

• It seems to me that it might complicate the logic quite a bit for both the user (since it won't work in most of the cases visually as one would expect) and for development (extra time due to generally complicating the logic for getting the total amount of animations, detecting an "endless animation loop", etc.)
• It doesn't really work in most of the cases with combinations due to having a separate element for each animation (explained in more detail in a comment above)
• It could be added later without causing migration issues if it should become necessary or if we can figure out a way how to guide the user better for the "begin after" feature not feeling like a bug instead (since most of the time it wouldn't work as expected).

Let me know your thoughts.

[spacedmonkey]

I think that we should build a flexible data that would allow us to support for multiple animations and add fields.

Could we have the animations as an array of objects? An array would allow as to add as many animations as we wanted later on, if that becomes a requirement. Array also allow for easier ordering of animations.

What do you think?

[miina]

Could we have the animations as an array of objects? An array would allow as to add as many animations as we wanted later on, if that becomes a requirement. Array also allow for easier ordering of animations.

Actually, it is already mentioned as an array of animations under "Technical Details" in the Brief. So yep, agreed on that. Added an extra specification to the description in case it was not clear to be an array of objects.

[spacedmonkey]

Actually, it is already mentioned as an array of animations under "Technical Details" in the Brief. So yep, agreed on that. Added an extra specification to the description in case it was not clear to be an array of objects.

Sorry, been a couple of days since I read the brief. At least we agree :D

[miina]

@swissspidy Mind sharing your opinion on allowing beforeAfter per block vs per animation?
To me it currently seems like it should be per block, reasoning here, however, there have been different opinions, see the first part of the comment here.

That's currently the only remaining open topic on this issue.

[cathibosco]

Balancing the user's ability to have full creative flexibility with sensible technical options is always a challenge. Good decisions and discussions above to support creative exploration for editors while maintaining technical logic.

Doing a quick evaluation with Miina here in the next couple of hours to provide the UX feedback needed.

[cathibosco]

It will be useful for us to truly know what animations work together in sequence and what do not display due to the (wrapper/content) content not displaying. If editors have a bad experience because the process of adding a second animation to an element produces negative experiences for many of the animations we need to revisit this. Their experience would feel very buggy and frustrating. I’d like to map it out so we aren’t guessing about what will work together in sequencing animations and what does not.

I am also adding some UI options to our Figma mock ups and will share them too.
There are some tricky time-line issues that come up and the "Begin after" prompt will reference to the whole element in the drop down menu once a second animation is added to an element. Let's test that and make sure it makes good sense.

Also, we will need to look at how to remove a second animation as well...

[miina]

Just to clarify that adding two animations to the same block can work together in most cases, it's mainly using "Begin After" that has unexpected results. Meaning that using just one "Begin After" per block is completely okay and works as expected.

It also would probably make sense to have "Play Animation" for all the block animations together, since they're combined.

[swissspidy]

IB looks good, moving to estimation 👍