Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Fix AccessibilityFeatures.disableAnimations flag on Android 12+ - #53428

Merged
auto-submit[bot] merged 3 commits into
flutter-team-archive:mainfrom
hakonber:fix_disable_animations_flag
Jun 27, 2024
Merged

auto-submit[bot] merged 3 commits into
flutter-team-archive:mainfrom
hakonber:fix_disable_animations_flag

Conversation

@hakonber

Copy link
Copy Markdown
Contributor

This PR fixes the problem where Flutter would not respect the "remove animation" accessibility setting on Android 12+.

Please see this issue for details: flutter/flutter#130976

As mentioned by horsemankukka, the problem has to do with reading Settings.Global.TRANSITION_ANIMATION_SCALE as a string instead of a float. Flutter would compare it to the string "0" to determine if animations should be disabled. Presumably, this worked because the settings app did indeed use the string "0" or "1" for this setting. But as of Android 12 it's instead written using float representation ("0.0" or "1.0"), at least on Samsung devices. The documentation also states that this setting should be read as a float, which is what this PR does.

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
  • I listed at least one issue that this PR fixes in the description above.
  • I added new tests to check the change I am making or feature I am adding, or the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the CLA.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@jonahwilliams jonahwilliams left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM!

@jonahwilliams
jonahwilliams requested a review from hannah-hyj June 17, 2024 15:54

@reidbaker reidbaker left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you check on a pre android 12 device to make sure that this code does not need to branch?
For example if before it stored "0" instead of 0 (and now stores 0.0)

@hakonber

hakonber commented Jun 18, 2024 •

Copy link
Copy Markdown
Contributor Author

Thanks for reviewing @jonahwilliams and @reidbaker. I actually discovered there's a second bug in AccessibilityBridge.java, which prevents animations from being disabled when the app is launched. setBoldTextFlag() contains a bit twiddling bug which causes all the other flags to be cleared. On app launch the following happens, provided the user has set the following accessibility settings: "Remove animations" on, "Bold text" off.

  1. animationScaleObserver.onChange() sets the DISABLE_ANIMATION flag.
  2. Immediately after, setBoldTextFlag() clears all flags due to the bit twiddling bug (it was obviously meant to only clear the BOLD_TEXT flag).

I've pushed a second commit that fixes this as well, and I now believe flutter/flutter#130976 is fully resolved. To verify, I have created a simple Flutter app that displays the value of MediaQuery.disableAnimationsOf(context), and I have checked that it is correct both on launch, and when changing the accessibility setting while the app is running. I've attached screen recordings from a Pixel 4 API 29 (Android 10), a Pixel 6 API 33 (Android 13) and a Samsung Galaxy S20 API 33 (Android 13). Note that when changing the "Remove animations" setting, it takes a few seconds before the flag updates after switching back to the app. This happens in native apps as well, so I believe it's just Android being Android, and not a problem with the Flutter engine.

Pixel.4.API.29.webm
Pixel.6.API.33.webm
s20.API.33.mp4

Test app:

import 'package:flutter/material.dart';

void main() {
  runApp(const MyApp());
}

class MyApp extends StatelessWidget {
  const MyApp({super.key});

  @override
  Widget build(BuildContext context) {
    return MaterialApp(
      title: 'Flutter Demo',
      theme: ThemeData(
        colorScheme: ColorScheme.fromSeed(seedColor: Colors.deepPurple),
        useMaterial3: true,
      ),
      home: Scaffold(
        body: Center(
          child:
              Text('MediaQueryData.disableAnimations=${MediaQuery.disableAnimationsOf(context)}'),
        ),
      ),
    );
  }
}

@reidbaker reidbaker left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@hakonber this looks great thanks for the follow up. The last thing I would ask of you is to modify itSetsBoldTextFlagCorrectly (~line 1325) To check another flag value was not cleared.

@hakonber
hakonber force-pushed the fix_disable_animations_flag branch from fc332f5 to ee06b26 Compare June 19, 2024 08:44
@hakonber

Copy link
Copy Markdown
Contributor Author

@reidbaker, I’ve expanded the itSetsBoldTextFlagCorrectly test to verify that clearing the bold text flag doesn’t affect other flags. This change was a bit involved because AccessibilityBridge only manipulates the bold text flag in its constructor, so I had to recreate the instance using setUpBridge.

@hakonber
hakonber requested a review from reidbaker June 26, 2024 08:31
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

autosubmit Merge PR when tree becomes green via auto submit App platform-android

Development

Successfully merging this pull request may close these issues.

4 participants