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

[a11y] Adding reduce motion logic to LottieAnimation for compose usage #2542

Merged
merged 2 commits into from
Aug 30, 2024

Conversation

pranayairan
Copy link
Contributor

Our changes in #2536 didn't work for LottieAnimation in compose. While LottieAnimation does use LottieDrawable, it has it's own animator and draws on canvas directly.

This PR addresses that issue by making some logic of reduce motion readable outside LottieDrawable and consuming that logic in LottieAnimation

I tested this by adding a sample lottie file with reduce motion marker locally, see the attached video.

Screen_recording_20240829_125041.mp4

adding reduce motion logic to LottieAnimation for compose usage
Comment on lines 136 to 137
if (!drawable.animationsEnabled(context) && drawable.markerForAnimationsDisabled != null) {
drawable.progress = drawable.markerForAnimationsDisabled!!.startFrame
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if (!drawable.animationsEnabled(context) && drawable.markerForAnimationsDisabled != null) {
drawable.progress = drawable.markerForAnimationsDisabled!!.startFrame
val markerForAnimationsDisabled = drawable.markerForAnimationsDisabled
if (!drawable.animationsEnabled(context) && markerForAnimationsDisabled != null) {
drawable.progress = markerForAnimationsDisabled.startFrame

Let's avoid !! here with a smart cast

@@ -853,7 +853,7 @@ public void playAnimation() {
*
* @return The first non-null marker from the list of allowed reduced motion markers, or null if no such marker is found.
*/
private Marker getMarkerForAnimationsDisabled() {
public Marker getMarkerForAnimationsDisabled() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you add a @RestrictTo(Library) on this one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link

Snapshot Tests
API 23: Report Diff
API 31: Report Diff

@RestrictTo(RestrictTo.Scope.LIBRARY)
@gpeal gpeal merged commit 66bc2fb into airbnb:master Aug 30, 2024
5 checks passed
Copy link

Snapshot Tests
API 23: Report Diff
API 31: Report Diff

@pranayairan pranayairan deleted the pa-reduce-motion-marker-compose branch August 30, 2024 23:11
@pranayairan
Copy link
Contributor Author

@gpeal thanks for merging, when you get time can you cut a . release with this change.

@gpeal
Copy link
Collaborator

gpeal commented Sep 3, 2024

@gpeal thanks for merging, when you get time can you cut a . release with this change.

Do you think you could look into this before I do that?

@pranayairan
Copy link
Contributor Author

yes, i saw this error for compose code but didn't realize it is happening for non compose, let me check and put a fix

@pranayairan
Copy link
Contributor Author

@gpeal opened up a PR for the NPE #2546

@gpeal
Copy link
Collaborator

gpeal commented Sep 4, 2024

@gpeal opened up a PR for the NPE #2546

@pranayairan I just released 6.5.2

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

Successfully merging this pull request may close these issues.

2 participants