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] Check for reduced motion frame names #2451

Merged
merged 6 commits into from
Jan 23, 2024

Conversation

bassettsj
Copy link
Contributor

@bassettsj bassettsj commented Jan 22, 2024

Brings parity to the android for the changes in iOS airbnb/lottie-ios/pull/2110

This PR adds support for respecting the system "reduction motion" option.

Animations Enabled

screen-20240122-155931.mp4

Animations Disabled

screen-20240122-160034.mp4

- brings parity to the android for the changes in iOS github.com/airbnb/lottie-ios/pull/2110
setFrame((int) (getSpeed() < 0 ? getMinFrame() : getMaxFrame()));
Marker markerForAnimationsDisabled = getMarkerForAnimationsDisabled();
if (markerForAnimationsDisabled != null) {
setFrame((int) markerForAnimationsDisabled.startFrame);
Copy link
Member

Choose a reason for hiding this comment

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

On iOS we also support reduced motion markers with both a start frame and an end frame. If the marker provides an end frame, we play the animation from the start / end of the reduced motion marker as opposed to just only displaying the first frame.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@calda Thanks for the quick reply, Is there a way to play from the start to end frame? Sorry for not knowing from looking at this code.

Copy link
Member

@calda calda Jan 22, 2024

Choose a reason for hiding this comment

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

@gpeal should be able to help :) just giving my drive-by review from an iOS perspective

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gpeal How would I play the animation from start to end here per @calda 's comment?

@@ -794,14 +795,52 @@ public void playAnimation() {
}
}
if (!animationsEnabled()) {
setFrame((int) (getSpeed() < 0 ? getMinFrame() : getMaxFrame()));
Marker markerForAnimationsDisabled = getMarkerForAnimationsDisabled();
if (markerForAnimationsDisabled != null) {
Copy link
Member

Choose a reason for hiding this comment

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

What should determine if "reduced motion" mode is enabled? Reading the code it seems like the reduced motion marker is always used if present, as opposed to only using it when the app / OS "reduced motion" toggle is enabled.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@calda It seems odd that we don't check to see if the animations are disabled in accessibility settings, it seems like it is set by the consumer application with: https://github.com/airbnb/lottie-android/blob/master/lottie/src/main/java/com/airbnb/lottie/LottieDrawable.java#L1190

Copy link
Collaborator

Choose a reason for hiding this comment

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

It defaults to this which can be set if battery saver is on or if you turn off animations in developer settings. It's not an a11y setting on Android afaik though.

Copy link
Member

@calda calda Jan 22, 2024

Choose a reason for hiding this comment

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

Oh I see, I missed that this is gated by a animationsEnabled() check, which can be configured via setSystemAnimationsAreEnabled.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It doesn't feel quite right to take a specific a11y setting on iOS and use the same name on Android when it maps to different things.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gpeal In the a11y community, "reduced motion" is the more common term even if it comes from Apple. I think the same is applied to lottie-web as well, so designers can produce one asset to share across platform.

Copy link
Contributor Author

@bassettsj bassettsj Jan 22, 2024

Choose a reason for hiding this comment

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

It's not an a11y setting on Android afaik though.

It is available under accessibility then Color and Motion
Color and Motions settings screen

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that's reasonable. It's worth noting that this would affect non a11y use cases like battery saver and developer settings but this behavior may still be better if the designer was explicit about it.

Could you:

  1. Double check that the utils function I linked to returns 0 for animation scale when this a11y setting is enabled
  2. Add documentation for setSystemAnimationsAreEnabled in case anybody else calls it so they can learn about this without reading the source?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Checked Accessibility setting in the utils function, it returns false when running in the debugger.

Screenshot 2024-01-23 at 1 18 54 PM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Add documentation for setSystemAnimationsAreEnabled in case anybody else calls it so they can learn about this without reading the source?

I added the document comment, thanks for the suggestion

Copy link
Member

@calda calda left a comment

Choose a reason for hiding this comment

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

Thanks for working on this! Looks great from an iOS perspective, especially after adding support for markers with an end frame

Copy link
Collaborator

@gpeal gpeal left a comment

Choose a reason for hiding this comment

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

Thanks for doing this. I also raised this with the lottie spec team and they created lottie/lottie-spec#7 for visibility and awareness for other platforms.

lottie/src/main/java/com/airbnb/lottie/LottieDrawable.java Outdated Show resolved Hide resolved
Copy link

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

@gpeal
Copy link
Collaborator

gpeal commented Jan 23, 2024

If you want to create a ReducedMotionTestCase, that would be great 😄
https://github.com/airbnb/lottie-android/tree/master/snapshot-tests/src/androidTest/java/com/airbnb/lottie/snapshots/tests

@bassettsj
Copy link
Contributor Author

@gpeal I think I added the test case that might work. Wasn't able to run locally, but we will see with the workflow here.

@gpeal
Copy link
Collaborator

gpeal commented Jan 23, 2024

Could you apply this patch to fix the test:

diff --git a/snapshot-tests/src/androidTest/java/com/airbnb/lottie/snapshots/SnapshotTestCaseContext.kt b/snapshot-tests/src/androidTest/java/com/airbnb/lottie/snapshots/SnapshotTestCaseContext.kt
index c5693a82..92df041d 100644
--- a/snapshot-tests/src/androidTest/java/com/airbnb/lottie/snapshots/SnapshotTestCaseContext.kt
+++ b/snapshot-tests/src/androidTest/java/com/airbnb/lottie/snapshots/SnapshotTestCaseContext.kt
@@ -56,7 +56,7 @@ suspend fun SnapshotTestCaseContext.withDrawable(
     assetName: String,
     snapshotName: String,
     snapshotVariant: String,
-    callback: (LottieDrawable) -> Unit,
+    callback: suspend (LottieDrawable) -> Unit,
 ) {
     val result = LottieCompositionFactory.fromAssetSync(context, assetName)
     val composition = result.value ?: throw IllegalArgumentException("Unable to parse $assetName.", result.exception)
@@ -254,4 +254,4 @@ private suspend fun View.awaitFrame() {
             cont.resume(Unit)
         }
     }
-}
\ No newline at end of file
+}
diff --git a/snapshot-tests/src/androidTest/java/com/airbnb/lottie/snapshots/tests/DisabledAnimationsTestCase.kt b/snapshot-tests/src/androidTest/java/com/airbnb/lottie/snapshots/tests/DisabledAnimationsTestCase.kt
index f535ae8e..b669635b 100644
--- a/snapshot-tests/src/androidTest/java/com/airbnb/lottie/snapshots/tests/DisabledAnimationsTestCase.kt
+++ b/snapshot-tests/src/androidTest/java/com/airbnb/lottie/snapshots/tests/DisabledAnimationsTestCase.kt
@@ -3,17 +3,23 @@ package com.airbnb.lottie.snapshots.tests
 import com.airbnb.lottie.snapshots.SnapshotTestCase
 import com.airbnb.lottie.snapshots.SnapshotTestCaseContext
 import com.airbnb.lottie.snapshots.withDrawable
+import kotlinx.coroutines.Dispatchers
+import kotlinx.coroutines.withContext
 
 class DisabledAnimationsTestCase : SnapshotTestCase {
     override suspend fun SnapshotTestCaseContext.run() {
         withDrawable("Tests/ReducedMotion.json", "System Animations", "Disabled") { drawable ->
-            drawable.setSystemAnimationsAreEnabled(false)
-            drawable.playAnimation()
+            withContext(Dispatchers.Main) {
+                drawable.setSystemAnimationsAreEnabled(false)
+                drawable.playAnimation()
+            }
         }
 
         withDrawable("Tests/ReducedMotion.json", "System Animations", "Enabled") { drawable ->
-            drawable.setSystemAnimationsAreEnabled(false)
-            drawable.playAnimation()
+            withContext(Dispatchers.Main) {
+                drawable.setSystemAnimationsAreEnabled(false)
+                drawable.playAnimation()
+            }
         }
     }
 }

@bassettsj bassettsj marked this pull request as ready for review January 23, 2024 21:35
Copy link

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

Copy link
Collaborator

@gpeal gpeal left a comment

Choose a reason for hiding this comment

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

Looks good! Just one small last comment

lottie/src/main/java/com/airbnb/lottie/LottieDrawable.java Outdated Show resolved Hide resolved
Copy link

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

@gpeal gpeal changed the title [a11y] check for reduced motion frame names [a11y] Check for reduced motion frame names Jan 23, 2024
@gpeal gpeal merged commit 6185eda into airbnb:master Jan 23, 2024
6 checks passed
@bassettsj bassettsj deleted the a11y-remove-animations branch January 24, 2024 17:07
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.

3 participants