Repository navigation
Animate the child between sub-screens when the fold changes - #192721
Open
diegolopezrm wants to merge 2 commits into
Open
diegolopezrm wants to merge 2 commits into
diegolopezrm wants to merge 2 commits into
Conversation
A device that folds or unfolds changes which sub-screen
`DisplayFeatureSubScreen` puts its child in, and the child arrived there in
the frame the change was reported. On an 800 logical pixel screen with a 40
pixel fold that is a jump of 420 pixels in one frame, which every dialog,
bottom sheet, popup menu and picker inherits, since the three widgets that
build a `DisplayFeatureSubScreen` are `DialogRoute`, `ModalBottomSheetRoute`
and the Cupertino sheet route.
Measured with a widget test that samples every frame instead of settling:
before f0 380 f1 380 f2 380 f3 380 f4 380 f5 380 f6 380 f7 380
after f0 800 f1 795 f2 779 f3 750 f4 710 f5 660 f6 605 f7 547
The `Padding` becomes an `AnimatedPadding`, with `duration` and `curve`
exposed so an app can lengthen, shorten or switch it off with
`Duration.zero`. The default is 200ms on `Curves.easeInOut`.
The first position is not animated, because an implicitly animated widget
only animates later changes: a widget built into an already folded device
starts where it belongs. That is what keeps this from disturbing existing
tests, which set a fold up before pumping rather than changing it midway.
There is a test for it, so the property does not quietly regress.
Addresses the animation half of flutter#171448. The other half of that issue — a
bottom sheet that spans both sub-screens — is an API question and is not
touched here.
Verified: the 11 tests in `display_feature_sub_screen_test.dart`, plus the
1,169 in the suites that use display features or build one of the three
routes — Cupertino dialog, menu anchor and route, Material about, bottom
sheet, dialog, popup menu, date picker, date range picker and time picker,
and the widgets media query and routes tests.
Contributor
There was a problem hiding this comment.
Code Review
This pull request updates DisplayFeatureSubScreen to animate the transition of its child between sub-screens by introducing duration and curve properties and replacing Padding with AnimatedPadding. It also adds tests to verify this animation behavior. The feedback recommends overriding debugFillProperties to expose these new properties for diagnostics, adhering to the Flutter Style Guide.
Comment on lines
+103
to
+106
| final Duration duration; | ||
|
|
||
| /// The curve the [child] follows while it moves between sub-screens. | ||
| final Curve curve; |
Contributor
There was a problem hiding this comment.
To adhere to the Flutter Style Guide ("Features we expect every widget to implement"), all widgets should override debugFillProperties to expose their properties for diagnostics. Please update the debugFillProperties method in DisplayFeatureSubScreen to include the new duration and curve properties:
@override
void debugFillProperties(DiagnosticPropertiesBuilder properties) {
super.debugFillProperties(properties);
properties.add(DiagnosticsProperty<Offset>('anchorPoint', anchorPoint, defaultValue: null));
properties.add(DiagnosticsProperty<Duration>('duration', duration, defaultValue: _defaultDuration));
properties.add(DiagnosticsProperty<Curve>('curve', curve, defaultValue: Curves.easeInOut));
}References
- According to the Flutter Style Guide ('Features we expect every widget to implement'), all widgets should override
debugFillPropertiesto expose their properties. (link)
Contributor
Author
There was a problem hiding this comment.
Done in 6210909, with anchorPoint included since it was missing too.
One correction for whoever reads this later: .gemini/styleguide.md doesn't say widgets must override debugFillProperties. It's still the right call — SafeArea, which this widget's docs compare it to, reports its properties the same way. Defaults are filtered, so an ordinary dialog's diagnostics dump is unchanged.
…infos `DisplayFeatureSubScreen` now reports `anchorPoint`, `duration` and `curve`, the way `SafeArea` reports its sides. Defaults are filtered out, so a diagnostics dump of an ordinary dialog is unchanged; only a value someone set shows up. Two tests cover both cases. Also fixes three analyzer infos the previous commit introduced, which the repo's `--fatal-infos` would have rejected: - `package:flutter/animation.dart` was an unnecessary import, since `basic.dart` already provides `Curve` and `Curves`. - Two `final MediaQueryData flat = MediaQueryData.fromView(...)` locals spelled out a type `omit_obvious_local_variable_types` asks to omit. The 1,169 tests in the suites that build a `DisplayFeatureSubScreen` still pass, which matters here specifically: `showDialog` passes an `anchorPoint`, so any test comparing a diagnostics dump of a dialog would now see it.
10 of 11 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When a device folds or unfolds,
DisplayFeatureSubScreenmoves its child to a different sub-screen, and the child got there in the frame the change was reported. On an 800 logical pixel screen with a 40 pixel fold, that is a jump of 420 pixels in one frame. Every dialog, bottom sheet, popup menu and picker inherits it, because the three things that build aDisplayFeatureSubScreenareDialogRoute,ModalBottomSheetRouteand the Cupertino sheet route.Measured with a widget test that samples every frame instead of settling, dialog width per frame:
Apple's guidance for the iPhone Duo, which ships on 23 October, asks for exactly this: "Avoid extreme layout changes as people fold the device." The same page says "Components like alerts, context menus, and sheets automatically move to account for the fold" — which is what
DisplayFeatureSubScreenalready does, so this makes the move look the way the platform's own components do. (Designing for iPhone Duo)The
Paddingbecomes anAnimatedPadding, anddurationandcurveare exposed so an app can lengthen, shorten or turn it off withDuration.zero. The default is 200ms onCurves.easeInOut. All three properties,anchorPointincluded, are reported throughdebugFillProperties, with defaults filtered out. The default has to animate:showDialogand friends build the widget themselves and don't expose these, so a zero default would leave the animation unreachable for almost everyone.The first position is not animated, because an implicitly animated widget only animates later changes — a widget built into an already folded device starts where it belongs. That's what keeps this from disturbing existing tests, which set a fold up before pumping rather than changing it midway. There's a test for it so the property can't quietly regress.
Addresses the animation part of #171448, which the reporter confirmed an
AnimatedPaddingimproves. The other part of that issue, a bottom sheet spanning both sub-screens, is an API question and isn't touched here. The reporter also mentioned residual jank from theSafeAreainside the bottom sheet; that's separate too.Related, not fixed here: #192667,
MediaQuery.sizeOfreporting the whole screen inside a sub-screen.Tests run locally: the 13 in
display_feature_sub_screen_test.dart(5 new), and the 1,169 in every suite that sets display features or builds one of the three routes —cupertino/dialog_test,cupertino/menu_anchor_test,cupertino/route_test,material/about_test,material/bottom_sheet_test,material/dialog_test,material/popup_menu_test,material/date_picker_test,material/date_range_picker_test,material/time_picker_test,widgets/media_query_testandwidgets/routes_test. Formatted with the repo's ownbin/dart.Pre-launch Checklist
///).