Use AnimationStyle curve and reverseCurve in ModalBottomSheet animation#181403
Conversation
github-actions
bot
added
framework
![gemini-code-assist[bot]](proxy.php?url=https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request successfully updates the ModalBottomSheet to utilize AnimationStyle for its animation curves, which is a positive step towards more flexible animation customization. New tests have been added to cover this functionality, ensuring its correctness. However, there appears to be an unintended change in the modal barrier's animation behavior.
I am having trouble creating individual review comments. Click here to see my feedback.
packages/flutter/lib/src/material/bottom_sheet.dart (1140-1142)
The ColorTween for the modal barrier no longer chains the CurveTween(curve: barrierCurve). This means the barrierCurve property, if still present and intended for configuration, is no longer being applied to the barrier's animation. This could be an unintended change in behavior for the modal barrier's animation, as the PR description focuses on the sheet's animation.
If barrierCurve is still a configurable property of ModalBottomSheetRoute, its removal here without an alternative mechanism to apply it would be a regression in customization.
|
|
||
| final proxy = route.animation! as ProxyAnimation; | ||
|
|
||
| final curved = proxy.parent! as CurvedAnimation; |
There was a problem hiding this comment.
Not a big fan of using type casts to regression test instead of testing the actual widget behavior (e.g that the animation follows the passed-in curve rather than a default, etc.).
There was a problem hiding this comment.
I find it a bit confusing how tests for these scenarios are written. Is there an easier way to do this?
There was a problem hiding this comment.
I wonder if we can monitor the details of the animation. Maybe use a simple curve like Curves.linear and verify that the sheet is at a certain non-default height halfway through the animation (if the height of the sheet is what is animating) etc.
|
|
||
| // Advance the animation by 1/3 of the duration. | ||
| await tester.pump(const Duration(milliseconds: 100)); | ||
| expect(tester.getTopLeft(find.byKey(sheetKey)).dy, closeTo(547.3, 0.1)); |
There was a problem hiding this comment.
I'm scared of these magic numbers because they make the test prone to break (and difficult to debug) with the tiniest changes to the underlying implementation. I wonder if we can use a predictable curve such as Curves.linear and somehow express the sheet extent as a function of both the animation duration and the sheet's initial extent.
For example, at 100ms it's at x, at 200ms it's at 2x, etc. That way even if the sheet's internal implementation changes, the test still reflects the relationship between the provided curve and the sheet's extension value.
There was a problem hiding this comment.
Yes, exactly, that’s why I don’t like testing animations 😄 . I just followed other existing examples.
There was a problem hiding this comment.
This is a test I got working for curve:
Details
testWidgets('ModalBottomSheet uses AnimationStyle curve', (WidgetTester tester) async {
final Key sheetKey = UniqueKey();
await tester.pumpWidget(
MaterialApp(
home: Scaffold(
body: Builder(
builder: (BuildContext context) {
return GestureDetector(
onTap: () {
showModalBottomSheet<void>(
context: context,
sheetAnimationStyle: const AnimationStyle(curve: Curves.linear),
builder: (BuildContext context) {
return SizedBox.expand(
key: sheetKey,
child: FilledButton(
onPressed: () {
Navigator.pop(context);
},
child: const Text('Close'),
),
);
},
);
},
child: const Text('X'),
);
},
),
),
),
);
await tester.tap(find.text('X'));
await tester.pump();
// Advance the animation by 50ms.
await tester.pump(const Duration(milliseconds: 50));
final double openExtent1 = tester.getTopLeft(find.byKey(sheetKey)).dy;
// Advance the animation by an additional 50ms.
await tester.pump(const Duration(milliseconds: 50));
final double openExtent2 = tester.getTopLeft(find.byKey(sheetKey)).dy;
// Advance the animation by an additional 50ms.
await tester.pump(const Duration(milliseconds: 50));
final double openExtent3 = tester.getTopLeft(find.byKey(sheetKey)).dy;
// For the linear curve, the distance covered in each time interval should
// be the same.
expect(openExtent1 - openExtent2, closeTo(openExtent2 - openExtent3, 0.1));
await tester.pumpAndSettle();
// Dismiss the bottom sheet.
await tester.tap(find.widgetWithText(FilledButton, 'Close'));
await tester.pumpAndSettle();
});
Can you add a test for reverseCurve as well?
There was a problem hiding this comment.
I recommend using Curves.decelerate instead, which is predicatble for being simply a t^2 and also being able to distinguish between t and output.
|
I’m getting into a separate topic that we can open an issue for later, but testing widget animations in Flutter is painful. If we look at how animations are tested today, we can see that the approaches have a bit of a code smell. I’ve been thinking about a few ideas, but I don’t have anything concrete yet. I need to study the animation API in more depth to better understand how everything works. |
|
@victorsanni How should I proceed with this PR, given that Flutter doesn’t provide a very good way to write tests for curves? I also created a proposal addressing this, which can be found here: #181597. |
Mairramer
force-pushed
the
fix/modal-bottom-sheet-animation-curves
branch
from
857e8d9 to
6b0c7fb
Compare
dkwingsmt
left a comment
dkwingsmt
left a comment
There was a problem hiding this comment.
LGTM except for Victor's comment.
Mairramer
force-pushed
the
fix/modal-bottom-sheet-animation-curves
branch
from
6b0c7fb to
6842ae6
Compare
|
@dkwingsmt Google Testing failures look related to this fix |
Mairramer
force-pushed
the
fix/modal-bottom-sheet-animation-curves
branch
from
bbc0eb0 to
0179d5f
Compare
victorsanni
left a comment
victorsanni
left a comment
There was a problem hiding this comment.
Google testing is still failing, I will rerun again. The images look like only the barrier color is changing, not the vertical extent of the bottom sheet.
|
|
||
| final ValueNotifier<EdgeInsets> _clipDetailsNotifier = ValueNotifier<EdgeInsets>(EdgeInsets.zero); | ||
|
|
||
| CurvedAnimation? _animation; |
There was a problem hiding this comment.
_ModalBottomSheetState.handleDragEnd uses the const default curve, should it use the provided curves?
There was a problem hiding this comment.
IMHO, the behavior feels a bit odd. Unless someone explicitly wants this in the future, I don’t see a strong reason to change it.
There was a problem hiding this comment.
the behavior feels a bit odd.
Can you explain further here?
My assumption is the sheet should animate with the provided animation when the drag ends. Currently it just uses its default curve for all these animations, but now that this PR overrides the default with a user-provided curve I'm wondering if that can cause discrepancies with the sheet animating one way in a certain state and a different way in another state.
There was a problem hiding this comment.
I understand what you mean. Should it be reverseCurve in this case?
| return _animation ??= CurvedAnimation( | ||
| parent: super.createAnimation(), | ||
| curve: sheetAnimationStyle?.curve ?? _modalBottomSheetCurve, | ||
| reverseCurve: sheetAnimationStyle?.reverseCurve ?? _modalBottomSheetCurve, |
There was a problem hiding this comment.
Should the fallback here be _modalBottomSheetCurve or _modalBottomSheetCurve.reverse?
There was a problem hiding this comment.
The default animation is Curves.linear, right? I think we can use that.
There was a problem hiding this comment.
The default animation is Curves.linear
I can't find this default, can you point out where this is set?
There was a problem hiding this comment.
I might be mistaken, but the Material documentation mentions Emphasized decelerate. So we could use Easing.legacyDecelerate as the default.
There was a problem hiding this comment.
I don't think we want to change the defaults here, so it would be nice to confirm within the code what the current default is for both forward and reverse animations and ensure that this PR falls back to those defaults in all states.
There was a problem hiding this comment.
Based on the Material documentation:
https://m3.material.io/styles/motion/easing-and-duration/applying-easing-and-duration
and
https://m3.material.io/styles/motion/easing-and-duration/tokens-specs
The token for Enter the screen is md.sys.motion.easing.emphasized.decelerate. However, Flutter currently uses md.sys.motion.easing.legacy.decelerate.
From what I can see, these tokens were introduced in Flutter in #116525.
In Material 2, the documentation describes Decelerated easing (cubic-bezier(0.0, 0.0, 0.2, 1)), which corresponds to what Flutter currently exposes as legacyDecelerate.
Given that, using Easing.legacyDecelerate as the default is correct.
There was a problem hiding this comment.
Let me know if I missed anything.
There was a problem hiding this comment.
Out of scope of this PR, but we should change to the M3 spec in the future.
There was a problem hiding this comment.
Out of scope of this PR, but we should change to the M3 spec in the future.
Should we file an issue for this?
|
Google testing is still failing with a lighter modal barrier color even without setting a |
Failures are image diffs, I am ok with overriding (with code owner's approval) if we have a clear understanding of why the modal barrier color is changing mid animation due to this PR. |
victorsanni
left a comment
victorsanni
left a comment
There was a problem hiding this comment.
Hi @Mairramer, sorry for the delay here. I have dug deeper into the showModalBottomSheet implementation and I think we can land this change by localizing the application of the sheetAnimationStyle.curve/reverseCurve.
Before this PR, ModalBottomSheetRoute used the default super.createAnimation, which creates an un-curved basic AnimationController with only the duration/reverseDuration specified (from sheetAnimationStyle.duration/reverseDuration). Making this animation curved will affect other children animations (e.g barrier color animations) since Flutter will now stack the child curves on top of the createAnimation curve.
What we want to do here instead is to apply the curves provided by sheetAnimationStyle.curve/reverseCurve only to the ModalBottomSheet (and not the barrier, etc). We can do this by supplying sheetAnimationStyle into the _ModalBottomSheet returned in ModalBottomSheetRoute.buildPage. Then, in _ModalBottomSheet, we can just replace instances of _modalBottomSheetCurve with sheetAnimationStyle?.curve ?? _modalBottomSheetCurve.
auto-submit
bot
removed
the
autosubmit
|
autosubmit label was removed for flutter/flutter/181403, because The base commit of the PR is older than 7 days and can not be merged. Please merge the latest changes from the main into this branch and resubmit the PR. |
chunhtai
added
the
autosubmit
auto-submit
bot
removed
the
autosubmit
|
autosubmit label was removed for flutter/flutter/181403, because - The status or check suite Google testing has failed. Please fix the issues identified (or deflake) before re-applying this label. |
|
looks like there is something wrong in the google test ci currently. wait a bit before retrying |
dkwingsmt
added
the
autosubmit
flutter-dashboard
bot
removed
the
autosubmit
Roll Flutter from dad6f9d4107a to b31548feb941 (39 revisions) flutter/flutter@dad6f9d...b31548f 2026-02-25 [email protected] [web] Fix failure on Firefox 148 (flutter/flutter#182855) 2026-02-25 [email protected] Roll Fuchsia Linux SDK from KfPgw04T0OEADLJA5... to XI0Ax7fbtYE4XKYAQ... (flutter/flutter#182887) 2026-02-25 [email protected] Use AnimationStyle curve and reverseCurve in ModalBottomSheet animation (flutter/flutter#181403) 2026-02-25 [email protected] Roll Dart SDK from fd3dce5b6a4e to 5c57e75f1102 (9 revisions) (flutter/flutter#182801) 2026-02-25 98614782+auto-submit[bot]@users.noreply.github.com Reverts "refactor: remove material in context_menu_controller_test, icon_test, list_wheel_scroll_view_test, media_query_test, platform_menu_bar_test (#182697)" (flutter/flutter#182879) 2026-02-25 [email protected] Make sure that an AnimatedSlide doesn't crash in 0x0 environment (flutter/flutter#181535) 2026-02-24 [email protected] Reland Standardize on Test* widgets in *_tester.dart files (flutter/flutter#182632) 2026-02-24 [email protected] docs(Path): clarify that zero-length contours are excluded from computeMetrics (flutter/flutter#180165) 2026-02-24 [email protected] Fix typo in assert message (flutter/flutter#182843) 2026-02-24 [email protected] [win32] Fix overflow in TaskRunnerWindow. (flutter/flutter#182822) 2026-02-24 [email protected] feat: Add --no-uninstall flag to flutter test for integration tests (flutter/flutter#182714) 2026-02-24 [email protected] Rename noFrequencyBasedMinification to useFrequencyBasedMinification (flutter/flutter#182684) 2026-02-24 [email protected] [Impeller] Fix fail to render pixel buffer texture on Linux (flutter/flutter#181656) 2026-02-24 [email protected] Remove FlutterFramework app migration (flutter/flutter#182100) 2026-02-24 [email protected] Roll Packages from 12b43a1 to 062c8d4 (5 revisions) (flutter/flutter#182839) 2026-02-24 [email protected] [web] Run webparagraph tests in CI (flutter/flutter#182092) 2026-02-24 [email protected] Fix a race in EmbedderTest.CanSpecifyCustomUITaskRunner (flutter/flutter#182649) 2026-02-24 [email protected] flutter_tools: Use a super-parameter in several missed cases (flutter/flutter#182581) 2026-02-24 [email protected] Replace more references to `flutter/engine` with `flutter/flutter` (flutter/flutter#182654) 2026-02-24 [email protected] Carousel: Migration from Scrollable+Viewport to CustomScrollView (flutter/flutter#182475) 2026-02-24 [email protected] Refactor impellerc_main to better organize some of its logic (flutter/flutter#182783) 2026-02-24 [email protected] Remove unused `getPluginList ` (flutter/flutter#182660) 2026-02-24 [email protected] Refactor: Remove material from ticker provider test (flutter/flutter#181697) 2026-02-24 [email protected] Roll Skia from 26eebffe12bd to f44d7db68805 (3 revisions) (flutter/flutter#182821) 2026-02-24 [email protected] refactor: remove material in context_menu_controller_test, icon_test, list_wheel_scroll_view_test, media_query_test, platform_menu_bar_test (flutter/flutter#182697) 2026-02-24 [email protected] Roll Skia from 7dad66aae75a to 26eebffe12bd (5 revisions) (flutter/flutter#182810) 2026-02-24 [email protected] Update roadmap for 2026 (flutter/flutter#182798) 2026-02-24 [email protected] Marks Windows tool_tests_commands_1_2 to be unflaky (flutter/flutter#179670) 2026-02-23 [email protected] [web] scroll iOS iframe text input into view (flutter/flutter#179759) 2026-02-23 [email protected] Fix textscaler clamp assertion error (flutter/flutter#181716) 2026-02-23 [email protected] Roll Skia from 9a5a3c92c336 to 7dad66aae75a (4 revisions) (flutter/flutter#182779) 2026-02-23 [email protected] Move more getters from userMessages class to the appropriate places (flutter/flutter#182656) 2026-02-23 [email protected] Manual roll Dart SDK from f8fac50475b8 to fd3dce5b6a4e (6 revisions) (flutter/flutter#182768) 2026-02-23 [email protected] Copy Flutter framework to Add to App FlutterPluginRgistrant (flutter/flutter#182523) 2026-02-23 [email protected] Add progress indicator to artifact downloads (flutter/flutter#181808) 2026-02-23 [email protected] Clarify batch release mode requirements (flutter/flutter#182228) 2026-02-23 [email protected] [web] Remove --disable-gpu from flutter chrome tests (flutter/flutter#182618) 2026-02-23 [email protected] running-apps: update running-apps to use Duration.ago() (flutter/flutter#182172) 2026-02-23 [email protected] Refactor bin/ shell scripts for better performance and safety (flutter/flutter#182674) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. ...
…on (flutter#181403) Fixes flutter#38958 Fixes flutter#174857 ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [ ] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [ ] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. **Note**: The Flutter team is currently trialing the use of [Gemini Code Assist for GitHub](https://developers.google.com/gemini-code-assist/docs/review-github-code). Comments from the `gemini-code-assist` bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md --------- Co-authored-by: chunhtai <[email protected]>
|
fyi @chunhtai this increased the binary size of the framework 60KB |
|
@gaaclarke This is unexpected, I don't think this pr adds any assets. Is there possible change to the devices, for example xcode update |
|
@chunhtai yea I think this is a misfire from the performance bot. It also flagged this regression to a Dart roll, which is more likely the cause. |
6 participants
Fixes #38958
Fixes #174857
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.