[a11y] Changing logic of how reduce motion options are set to match it with lottie iOS #2536
Conversation
…iOS API Changing logic of how reduce motion options are set to match it with iOS API, this allows apps to provide custom implementation for reduce motion which can rely on either system settings or some other logic.
There was a problem hiding this comment.
Thanks for looking into this!
I'm not totally sure all of the different ways this is used today. It's possible that somebody is intentionally using this on a per-view basis and this PR removes the ability to control it at that level.
Paired with my other comment about backwards compatibility, it would be good to potentially solve both with a setting at the view level that falls back to the global config when not set.
| * | ||
| * @param ignore if true animations will run even when they are disabled in the system settings. | ||
| */ | ||
| public void setIgnoreDisabledSystemAnimations(boolean ignore) { |
There was a problem hiding this comment.
Removing an existing public API is tricky. At the very least, we'll need to deprecate this and provide a graceful fallback. Ideally, its behavior would remain the same (scope IgnoreDisabledSystemAnimations() to this view if possible).
It can be fully removed after a few releases.
There was a problem hiding this comment.
@gpeal make sense, added the original API back. I also marked it as deprecated so folks can switch to new API
putting the orignal API back to maintain backward compatibility.
|
@gpeal wondering if this looks good can we merge it ? |
| return (systemAnimationsEnabled || ignoreSystemAnimationsDisabled) && | ||
| L.getReducedMotionOption().getCurrentReducedMotionMode(getContext()) == ReducedMotionMode.STANDARD_MOTION; |
There was a problem hiding this comment.
If I'm reading this correctly, if you set ignoreSystemAnimationsDisabled but the system scale is 0 then:
- You would expect animationsEnabled to return true
- systemAnimationsEnable: false
- ignoreSystemAnimationsDisabled: true
- LottieConfig: REDUCED
The predicate becomes (false || true) && (REDUCED == STANDARD) which would be false.
I think this may also read easier if you treat ignoreSystemAnimationsDisabled == true as higher priority with:
if (ignoreSystemAnimationsDisabled) {
return true;
}
return L.getReducedMotionOption().getCurrentReducedMotionMode(getContext()) == ReducedMotionMode.STANDARD_MOTION;
Just off the top of my head, I think this is correct and easier to read. Is that right?
There was a problem hiding this comment.
you are right. Ideally getCurrentReducedMotionMode should not even be evaluated when ignoreSystemAnimationsDisabled is set to true. Let me fix it.
There was a problem hiding this comment.
I changed it to this
private boolean animationsEnabled() {
if (ignoreSystemAnimationsDisabled) {
return true;
}
return systemAnimationsEnabled &&
L.getReducedMotionOption().getCurrentReducedMotionMode(getContext()) == ReducedMotionMode.STANDARD_MOTION;
}
We still need to consider old systemAnimationsEnabled flag that can be set on drawable directly for backward compatibility
updating logic to consider `ignoreSystemAnimationsDisabled` first before running other evaluation.
|
@gpeal thanks for merging. Can you share what is the release cadence we follow ? this will help me plan downstream changes. |
I just release 6.5.1 with your change! |
|
@gpeal thanks. I am testing the integration and I think i miss something. The logic is not working when i am using From what i can understand looking into the code, when using |
That's correct. I should have remembered to check for that in your PR. lottie-compose uses LottieDrawable but uses its own animator. |
|
make sense, let me try creating a new PR for compose then. |
#2542) 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. https://github.com/user-attachments/assets/eb33333f-86b8-46fa-9bbb-82bff8a8c7fe Co-authored-by: Pranay Airan <[email protected]>
|
Heads up our UI tests caught this crash this morning in |
#2546) When calling reduce motion check we need to pass context, we read context in LottieDrawable using `getContext` method. getContext method can return a nullable context, since the code is in java we don't get any compile time error when passing the null context around. This resulted in issue where we end up calling `getContentResolver` on a null object in case where context is null. #2536 This PR fixes it by adding a null check before calling `Utils.getAnimationScale(context)` Co-authored-by: Pranay Airan <[email protected]>
This PR introduces a new way to determine if reduce motion is enabled or not. It matches iOS Lottie API airbnb/lottie-ios#2110.
This change provides the ability for apps integrating Lottie to have custom reduce motion behavior which doesn't need to rely solely on system settings.
This change also allows reduce motion to work regardless of usage of LottieAnimationView, LottieAnimation in compose or straight LottieDrawable.
Testing