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

🐛 Stories: Disable animations when user prefers-reduced-motion #34081

Merged
merged 8 commits into from Apr 29, 2021

Conversation

alanorozco
Copy link
Member

@alanorozco alanorozco commented Apr 29, 2021

Native animations will not run with prefers-reduced-motion. Disable the AnimationManager altogether so that pre-animation styling is not applied, and elements correctly remain in their end-state.

Fixes #34062

@alanorozco alanorozco marked this pull request as ready for review April 29, 2021 15:14
@amp-owners-bot
Copy link

amp-owners-bot bot commented Apr 29, 2021

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

extensions/amp-story-panning-media/0.1/amp-story-panning-media.js
extensions/amp-story/1.0/amp-story-page.js
extensions/amp-story/1.0/test/test-amp-story-page.js

Hey @processprocess! These files were changed:

extensions/amp-story-panning-media/0.1/amp-story-panning-media.js

Hey @Enriqe! These files were changed:

extensions/amp-story/1.0/amp-story-page.js
extensions/amp-story/1.0/test/test-amp-story-page.js

Copy link
Contributor

@processprocess processprocess left a comment

Choose a reason for hiding this comment

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

This looks awesome to me. Thanks for making this fix so fast and setting up/updating the tests for it.

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.

Can you cleanup the maybeFinishAnimations_ code?

@processprocess
Copy link
Contributor

processprocess commented Apr 29, 2021

Can you cleanup the maybeFinishAnimations_ code?

It's used in vertical rendering and called in onUIStateUpdate_. We might need to keep it.

@processprocess
Copy link
Contributor

cc @kristoferbaxter for owners approval.

Copy link
Contributor

@Enriqe Enriqe left a comment

Choose a reason for hiding this comment

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

nice fix! one comment, plz fix before merging

Copy link
Contributor

@kristoferbaxter kristoferbaxter left a comment

Choose a reason for hiding this comment

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

LGTM, nice work!

@@ -26,6 +26,7 @@ import {Services} from '../../../src/services';
import {closest, whenUpgradedToCustomElement} from '../../../src/dom';
import {deepEquals} from '../../../src/json';
import {dev, user} from '../../../src/log';
import {prefersReducedMotion} from '../../../src/utils/media-query-props';
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Potentially followup with moving this to src/core.

@alanorozco alanorozco merged commit faeb7fb into ampproject:main Apr 29, 2021
@alanorozco alanorozco deleted the prefers-reduced-motion branch April 29, 2021 18:51
rochapablo pushed a commit to rochapablo/amphtml that referenced this pull request Aug 30, 2021
…project#34081)

Native animations do not run with `prefers-reduced-motion`.

Disable the `AnimationManager` altogether so that pre-animation styling is not applied. Elements correctly remain in their end-state when the animation itself doesn't run.

Fixes ampproject#34062
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] Animation does not work on IOS 14
6 participants