Support disabled animations #20354

jonahwilliams · 2018-08-08T20:27:41Z

Adds configuration to Tickers to allow them to ignore an active timeDilation by reversing the computation.
Adds a TimeDilationBehavior enum and a getter to the SingleTickerProviderStateMixin to allow specific widgets to opt out of disabled animation. This is necessary for certain scroll physics and other features that use the Ticker but aren't strictly an "Animation".

Alternatives considered:

Add configuration enum to the AnimationController.
Manual adjustment of animation durations in specific widgets.

The Ticker is the source of updates for all Animation controllers. Adjusting timing here effects all downstream animations without requiring code changes for most widgets. For special cases, there is an easy opt out with a single line of code. Since multiple animations are created from a single ticker, and we don't (?) combine tickers, the adjustment to the passage of time shouldn't break anything. Time is still monotonic, just not in sync globally.

On the other hand, AnimationController is often created by widgets themselves, meaning each individual widget would have to look up and pass this configuration to the constructor correctly.

Manually adjusting durations based on the setting is opt in, meaning most widgets will never correctly conform, and a also tremendous amount of work.

Both cases requiring reading a global setting, but the former is much less of a bucket brigade. The advantage of being configurable per subtree that manual adjustment gives isn't worth it.

This doesn't address how to handle certain indefinite animations. It might be correct for accessibility purposes to ignore the timeDilation. Otherwise it seems like they should react specifically by showing a specific keyframe animation - this PR doesn't attempt to address this beyond making sure they don't flash/spin at 20x normal speed.

Work towards #4827
Fixes #11436

EDIT:

I have updated the PR to use a configuration enum for the animation controller which controls how the _InterpolationSimulation used for most animations and the SpringSimulation used by controller.fling behave under the presences of disable animations.

For the former I decrease the duration, and for the latter I increase the velocity.

jonahwilliams · 2018-08-08T20:28:16Z

cc @tvolkert will this break tests? Not this change specifically, but exposing this behavior

tvolkert · 2018-08-08T21:24:45Z

I don't think exposing this will intrinsically break any tests, but I defer to @Hixie on the review.

jonahwilliams · 2018-08-08T22:38:58Z

WIP for tests

HansMuller

This all looks good but it seems to be missing tests for everything that was added.

HansMuller · 2018-08-10T15:41:58Z

packages/flutter/lib/src/scheduler/ticker.dart

+  TimeDilationBehavior get timeDilationBehavior => TimeDilationBehavior.normal;
+}
+
+/// Configures how a [Timer] behaves in the presence of time dilation.


The doc for this enum and its values seems correct but still a bit difficult to understand.

I think it would be useful to be more expansive, explain what the overall goal is, what the typical use cases are, etc.

I've expanded the documentation significantly, I think it reads a lot better now.

HansMuller · 2018-08-10T15:42:29Z

packages/flutter/lib/src/scheduler/ticker.dart

+
+/// Configures how a [Timer] behaves in the presence of time dilation.
+enum TimeDilationBehavior {
+  /// The adjusted timestampts are accepted.


typo: timestampts

HansMuller · 2018-08-10T15:45:25Z

packages/flutter/lib/src/scheduler/ticker.dart

@@ -36,6 +36,20 @@ abstract class TickerProvider {
  ///
  /// The kind of ticker provided depends on the kind of ticker provider.
  Ticker createTicker(TickerCallback onTick);
+
+  /// The behavior of the created [Ticker]s in the presence of time dilation.
+  ///


Please explain (in laymans terms) what effect the two TimeDilationBehavior values will have on the ticker

Partially covered by the expanded enum doc comments I believe

HansMuller · 2018-08-10T15:46:14Z

packages/flutter/lib/src/scheduler/ticker.dart

@@ -59,7 +73,7 @@ class Ticker {
  ///
  /// An optional label can be provided for debugging purposes. That label
  /// will appear in the [toString] output in debug builds.


This seems like a good place to mention why you might want to override the default value of the timeDilationBehavior parameter.

HansMuller · 2018-08-10T15:51:04Z

packages/flutter/lib/src/scheduler/ticker.dart

-
-    _onTick(timeStamp - _startTime);
+    if (timeDilationBehavior == TimeDilationBehavior.unscaled && timeDilation != 1.0) {
+      final double scale = 1 / timeDilation;


assert(timeDilation > 0.0)

1.0 / timeDilation instead of 1 / ...

HansMuller · 2018-08-10T15:52:43Z

packages/flutter/lib/src/scheduler/ticker.dart

+    if (timeDilationBehavior == TimeDilationBehavior.unscaled && timeDilation != 1.0) {
+      final double scale = 1 / timeDilation;
+      final Duration delta = timeStamp - _startTime;
+      final Duration tick = new Duration(microseconds: (delta.inMicroseconds / scale).round());


Maybe we should clamp the microseconds value to a reasonable range?

HansMuller · 2018-08-10T15:56:39Z

packages/flutter/lib/src/widgets/binding.dart

@@ -378,6 +378,10 @@ abstract class WidgetsBinding extends BindingBase with SchedulerBinding, Gesture
  @override
  void handleAccessibilityFeaturesChanged() {
    super.handleAccessibilityFeaturesChanged();
+    if (ui.window.accessibilityFeatures.disableAnimations)
+      timeDilation = 0.05;


An implementation comment like // see Ticker.timeDilationBehavior might help. If 0.05 was a local constant var you could explain why 0.05 in the var's doc.

HansMuller · 2018-08-10T15:57:59Z

packages/flutter/lib/src/widgets/scrollable.dart

@@ -264,6 +264,9 @@ class ScrollableState extends State<Scrollable> with TickerProviderStateMixin
  ScrollBehavior _configuration;
  ScrollPhysics _physics;

+  @override
+  TimeDilationBehavior get timeDilationBehavior => TimeDilationBehavior.unscaled;


If you had a standard comment about why timeDilationBehavior is unscaled, you could copy it around with these ...unscaled overrides.

HansMuller · 2018-08-10T16:01:14Z

packages/flutter/test/rendering/proxy_box_test.dart

@@ -261,6 +264,9 @@ class _FakeTicker implements Ticker {
  @override
  String get debugLabel => null;

+  @override
+  TimeDilationBehavior timeDilationBehavior;


Perhaps it would be no less of a fake ticker, but why not return ...normal here?

HansMuller · 2018-08-10T16:05:15Z

packages/flutter/test/scheduler/ticker_test.dart

@@ -83,6 +83,54 @@ void main() {
    expect(ticker.toString(debugIncludeStack: true), contains('testFunction'));
  });

+  testWidgets('Ticker can be sped up with time dilation', (WidgetTester tester) async {


Shouldn't there be tests for the timeDilationBehavior stuff?

These are tests for the time dilation stuff :D

…into disable_animation

Hixie · 2018-08-12T07:51:32Z

We shouldn't drive this from timeDilation. That's a debugging aid that is intended to make time in general slow down by actually changing the global clock used internally in the app. We should have a separate dilation factor for this that is applied by the AnimationController code, IMHO.

jonahwilliams · 2018-08-13T17:52:06Z

@Hixie I refactored this to have a similar effect using AnimationController config. It requires a bit more plumbing, as the logic will have to be slightly different for each Simulation - I'm not sure if there is a better way to short-circuit that while still preserving their overall behavior - for example, making sure when a spring simulation is dragged it still feels like a spring.

jonahwilliams · 2018-08-22T17:44:40Z

Friendly ping @Hixie , @HansMuller

HansMuller

This all seems fine. I had some trouble sorting out the docs and some tests area needed.

HansMuller · 2018-08-22T18:36:57Z

packages/flutter/lib/src/animation/animation_controller.dart

@@ -38,6 +38,31 @@ const Tolerance _kFlingTolerance = Tolerance(
  distance: 0.01,
 );

+/// Configures how an [AnimationController] behaves when animations are disabled.
+///
+/// When [AccessibilityFeatures.disableAnimations] is enabled, animations are


Maybe replace "enabled" with "true" since "disabledFoo is enabled" is a little bit difficult to parse.

It might also be helpful to smoosh in the idea that "AccessibilityFeatures.disableAnimations" corresponds to the device asking Flutter to disable animations.

HansMuller · 2018-08-22T18:41:19Z

packages/flutter/lib/src/animation/animation_controller.dart

+/// Configures how an [AnimationController] behaves when animations are disabled.
+///
+/// When [AccessibilityFeatures.disableAnimations] is enabled, animations are
+/// speed up or modified to remove most of the visual component. This enum is


speed => sped up (shouldn't this be slowed down?)

to remove most of the visual changes?

I think the overall point is that if the system asks us to disable animations we're going to try and slow down or eliminate animated visual transitions that are just decorative anyway.

rephrased this to be cleared that we reduce the duration and number of frames for an animation

HansMuller · 2018-08-22T18:41:51Z

packages/flutter/lib/src/animation/animation_controller.dart

+/// the purpose of preserving some visual animation or simulation behavior for
+/// accessibility.
+///
+/// For example, the [AnimationController which controls the physics simulation


[AnimationController => [AnimationController]

HansMuller · 2018-08-22T18:43:03Z

packages/flutter/lib/src/animation/animation_controller.dart

+/// for a scrollable list will have [AnimationBehavior.unscalable] so that when a
+/// user attempts to scroll it does not jump to the end/beginning too quickly.
+enum AnimationBehavior {
+  /// The [AnimationController] will reduce or remove the visual component by


"the visual component" isn't the clearest way to express this because component.

Since the other value is called "unscalable", it might be helpful to describe this value in terms of scaling.

Based on this description, I don't really understand what the AnimationController is going to do.

HansMuller · 2018-08-22T19:06:07Z

packages/flutter/lib/src/animation/animation_controller.dart

@@ -170,6 +197,17 @@ class AnimationController extends Animation<double>
  /// identifying animation controller instances in debug output.
  final String debugLabel;

+  /// The behavior of the animation or simulation when animations are disabled.


If this property isn't just about the AnimationController, it would help to explain how it affects anything else.

HansMuller · 2018-08-22T19:14:21Z

packages/flutter/lib/src/animation/animation_controller.dart

@@ -170,6 +197,17 @@ class AnimationController extends Animation<double>
  /// identifying animation controller instances in debug output.
  final String debugLabel;

+  /// The behavior of the animation or simulation when animations are disabled.
+  ///
+  /// Defaults to [AnimationBehavior.normal] for the [new AnimationBehavior()]


I didn't know that [new Foo] was required to identify a constructor. It looks (grepping through our sources) like the '()' isn't needed, so just [new AnimationBehavior]

HansMuller · 2018-08-22T19:40:09Z

packages/flutter/lib/src/animation/animation_controller.dart

+    if (ui.window.accessibilityFeatures.disableAnimations) {
+      switch (behavior) {
+        case AnimationBehavior.normal:
+          scale = 0.05;


The descriptions of AnimationBehavior.normal might as well point out the similarity with timeDilation=0.05 as well as explaining what the actual effect is in, uh, layman's terms.

HansMuller · 2018-08-22T19:42:02Z

packages/flutter/lib/src/animation/animation_controller.dart

+
+  /// The [AnimationController] will preserve its behavior.
+  ///
+  /// This is the default for repeating animations in order to prevent them from


This value is typically used by repeating animations, like indeterminate progress indicators, to prevent the AnimationController from ticking at all?

Also: since (as far as I can tell) this value means that we don't do anything when the device requests that animations be disabled, it would be helpful to say as much.

I renamed the enum to "preserve" since this captures the concept better, scaling was more related to the time dilation stuff.

…ble_animation

HansMuller

LGTM

HansMuller · 2018-08-22T21:53:02Z

packages/flutter/lib/src/animation/animation_controller.dart

@@ -170,6 +197,18 @@ class AnimationController extends Animation<double>
  /// identifying animation controller instances in debug output.
  final String debugLabel;

+  /// The behavior of the animation when [AccessibilityFeatures.disableAnimations]


the animation => the controller

Same as the last "See also" line

HansMuller · 2018-08-22T21:54:55Z

packages/flutter/lib/src/widgets/binding.dart

+  /// The currently active set of [AccessibilityFeatures].
+  ///
+  /// This is initialized the first time [runApp] is called and updated whenever
+  /// the a flag is changed.


Fixed, but also moved to renderer binding to fix circular dep

jonahwilliams · 2018-08-23T16:22:24Z

@HansMuller I made some changes - I could not expose the disableAnimations flag on either the WidgetsBinding or the RendererBinding since both of those depend on the animation layer, and doing so caused a circular import error. To resolve the situation I create a new SemanticsBindings, which we've been meaning to do anyway, but didn't move anything to it yet.

HansMuller · 2018-08-23T18:28:24Z

SemanticsBindings sounds rather highfalutin but I don't think you could have called it anything else :-).

Still looks good to me.

Hixie · 2018-08-24T19:35:42Z

packages/flutter/lib/src/animation/animation_controller.dart

+  ///
+  /// Defaults to [AnimationBehavior.normal] for the [new AnimationBehavior]
+  /// constructor, and [AnimationBehavior.preserve] for the
+  /// [new AnimationBehavior.unbounded] constructor.


some of these AnimationBehaviors are presumably AnimationControllers?

Hixie · 2018-08-24T19:36:32Z

packages/flutter/lib/src/animation/animation_controller.dart

+  TickerFuture _animateToInternal(double target, { Duration duration, Curve curve = Curves.linear, AnimationBehavior animationBehavior }) {
+    final AnimationBehavior behavior = animationBehavior ?? this.animationBehavior;
+    double scale = 1.0;
+    if (SemanticsBinding.instance.disableAnimations) {


we should get this via the TickerProvider or via the Ticker, not directly from the binding, so that it is easier to override.

Hixie · 2018-08-24T19:37:07Z

packages/flutter/lib/src/animation/animation_controller.dart

@@ -441,11 +487,22 @@ class AnimationController extends Animation<double>
  /// The most recently returned [TickerFuture], if any, is marked as having been
  /// canceled, meaning the future never completes and its [TickerFuture.orCancel]
  /// derivative future completes with a [TickerCanceled] error.
-  TickerFuture fling({ double velocity = 1.0 }) {
+  TickerFuture fling({ double velocity = 1.0, AnimationBehavior animationBehavior}) {


nit: inconsistent spacing (space after { but no space before })

Hixie · 2018-08-24T19:37:46Z

packages/flutter/lib/src/animation/animation_controller.dart

+    if (SemanticsBinding.instance.disableAnimations) {
+      switch (behavior) {
+        case AnimationBehavior.normal:
+          scale = 200.0;


this and the 0.05 constant earlier are presumably intentional related. We should derive them from the same constant.

Hixie · 2018-08-24T19:39:01Z

packages/flutter/lib/src/animation/animation_controller.dart

@@ -398,7 +444,7 @@ class AnimationController extends Animation<double>
    }
    assert(simulationDuration > Duration.zero);
    assert(!isAnimating);
-    return _startSimulation(new _InterpolationSimulation(_value, target, simulationDuration, curve));
+    return _startSimulation(new _InterpolationSimulation(_value, target, simulationDuration, curve, scale));


why not just multiply the duration here rather than pass the value and then have the constructor multiply?

Hixie · 2018-08-24T19:39:42Z

packages/flutter/lib/src/semantics/binding.dart

+import 'package:flutter/foundation.dart';
+import 'package:flutter/services.dart';
+
+


nit: extraneous blank line

Hixie · 2018-08-24T19:40:19Z

packages/flutter/lib/src/semantics/binding.dart

+  /// To listen to changes to accessibility features, create a
+  /// [WidgetsBindingObserver] and listen to [didChangeAccessibilityFeatures].
+  ui.AccessibilityFeatures get accessibilityFeatures => _accessibilityFeatures;
+  ui.AccessibilityFeatures _accessibilityFeatures;


you could just make this a ValueListenable...

Hixie · 2018-08-24T19:40:34Z

packages/flutter/lib/src/semantics/binding.dart

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+import 'dart:ui' as ui show AccessibilityFeatures, window;


since you expose AccessibilityFeatures in your API, you should re-export it

Hixie · 2018-08-24T19:40:54Z

packages/flutter/lib/src/semantics/binding.dart

+
+/// The glue between the semantics layer and the Flutter engine.
+// TODO(jonahwilliams): move the remaining semantic related bindings here.
+class SemanticsBinding extends BindingBase with ServicesBinding {


why do you need ServicesBinding?

Hixie · 2018-08-24T19:42:17Z

packages/flutter/lib/src/animation/animation_controller.dart

+  TickerFuture _animateToInternal(double target, { Duration duration, Curve curve = Curves.linear, AnimationBehavior animationBehavior }) {
+    final AnimationBehavior behavior = animationBehavior ?? this.animationBehavior;
+    double scale = 1.0;
+    if (SemanticsBinding.instance.disableAnimations) {


also wherever you call into SemanticsBinding.instance, handle the case of it being null (which might happen if someone has code that they haven't updated to mix in the new binding).

Hixie · 2018-08-24T19:44:04Z

modulo some minor stuff, let me know if you want me to look at it again after the changes

tvolkert · 2018-10-04T04:03:16Z

packages/flutter_test/lib/src/binding.dart

@@ -127,6 +128,9 @@ abstract class TestWidgetsFlutterBinding extends BindingBase
  @protected
  bool get checkIntrinsicSizes => false;

+  @override
+  bool disableAnimations = false;


Apparently docs aren't inherited from mixins...

https://docs.flutter.io/flutter/flutter_test/TestWidgetsFlutterBinding/disableAnimations.html

Tested locally, and it's not about mixins - it's about overriding a getter with a member variable. Filed dart-lang/dartdoc#1779

jonahwilliams added 2 commits August 7, 2018 16:12

map disable animation to timeDilation

8b2662a

add time dilation behavior

9385fa3

googlebot added the cla: yes label Aug 8, 2018

jonahwilliams added 2 commits August 8, 2018 13:32

fix scrollable

fc2b1e0

fix missing implements

652e562

jonahwilliams added 2 commits August 8, 2018 14:37

remove indicator hacks and configure demo to behave

a8a33fc

remove whitespace

1b777a5

jonahwilliams requested a review from Hixie August 8, 2018 22:26

jonahwilliams changed the title ~~[WIP] Support disabled animations using time dilation~~ Support disabled animations using time dilation Aug 8, 2018

jonahwilliams changed the title ~~Support disabled animations using time dilation~~ [WI] Support disabled animations using time dilation Aug 8, 2018

jonahwilliams changed the title ~~[WI] Support disabled animations using time dilation~~ [WIP] Support disabled animations using time dilation Aug 8, 2018

jonahwilliams and others added 7 commits August 9, 2018 10:14

Added time dilation tests

9f4e576

remove extra member from animation controller constructor

06ffa75

remove specific timedilation overrides

d125e75

undo extra changes

503231f

remove even more extra changes

f3c502c

add fix for reorderable list

5befaec

Merge branch 'master' into disable_animation

b0e6a9c

jonahwilliams changed the title ~~[WIP] Support disabled animations using time dilation~~ Support disabled animations using time dilation Aug 9, 2018

jonahwilliams requested a review from HansMuller August 9, 2018 20:22

jonahwilliams and others added 2 commits August 9, 2018 13:44

dont disable animations for scrollables

4722141

Update reorderable_list.dart

0d2550f

HansMuller reviewed Aug 10, 2018

View reviewed changes

jonahwilliams added 3 commits August 10, 2018 17:15

address some review comments

b8b0cb0

Merge branch 'disable_animation' of github.com:jonahwilliams/flutter …

2e8fc1e

…into disable_animation

make fake ticker less fake

f496b5b

Remove whitespace

7699337

HansMuller reviewed Aug 22, 2018

View reviewed changes

jonahwilliams added 3 commits August 22, 2018 13:12

Merge branch 'master' of https://github.com/flutter/flutter into disa…

fc8e59b

…ble_animation

Address comment responses

1449f6c

yay add tests

df8c15d

HansMuller approved these changes Aug 22, 2018

View reviewed changes

jonahwilliams added 2 commits August 22, 2018 18:45

Address comments and fix circular dep

343b23c

work around circular import by introducing SemanticsBindings a bit early

f518e08

jonahwilliams merged commit e235ccd into flutter:master Aug 23, 2018

jonahwilliams deleted the disable_animation branch August 23, 2018 18:50

Hixie reviewed Aug 24, 2018

View reviewed changes

jonahwilliams added a commit to jonahwilliams/flutter that referenced this pull request Aug 29, 2018

address follow up to flutter#20354

7b0cccb

jonahwilliams added a commit that referenced this pull request Aug 29, 2018

Address follow up to #20354 (#21181)

0e9cfe7

tvolkert reviewed Oct 4, 2018

View reviewed changes

creativecreatorormaybenot mentioned this pull request May 30, 2021

Animations should disable or degrade when battery saver mode is enabled (Android) #11436

Closed

github-actions bot locked as resolved and limited conversation to collaborators Aug 9, 2021

		import 'package:flutter/foundation.dart';
		import 'package:flutter/services.dart';

Support disabled animations #20354

Support disabled animations #20354

Uh oh!

Conversation

jonahwilliams commented Aug 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonahwilliams commented Aug 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tvolkert commented Aug 8, 2018

Uh oh!

jonahwilliams commented Aug 8, 2018

Uh oh!

HansMuller left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hixie commented Aug 12, 2018

Uh oh!

jonahwilliams commented Aug 13, 2018

Uh oh!

jonahwilliams commented Aug 22, 2018

Uh oh!

HansMuller left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonahwilliams commented Aug 8, 2018 •

edited

Loading

jonahwilliams commented Aug 8, 2018 •

edited

Loading