From c246ecdf8e8d564410698e86cc07e853269ebab8 Mon Sep 17 00:00:00 2001 From: Tong Mu Date: Mon, 3 Jun 2024 18:06:22 -0700 Subject: [PATCH] Fix the scrolling layout deviation of `CupertinoActionSheet` (#149439) This PR changes `CupertinoActionSheet`'s layout algorithm, so that the actions section always has the lower priority to take up vertical space with a fixed minimum height of 84.3 px. Currently `CupertinoActionSheet` uses a very complicated rule the determine the sizes of various sections: 1. If there are <= 3 buttons and a cancel button, or 1 button without a cancel button, then the actions section should never scroll. 2. Otherwise, then the content section takes priority to take over spaces but must leave at least `actionsMinHeight` for the actions section. This has been the case since `CupertinoActionSheet` was first introduced ([first PR](https://github.com/flutter/flutter/pull/19232)). Maybe it was the case before, but it's no longer reproducible on the latest SwiftUI. The following images for comparison (left to right: Current Flutter, Flutter after this PR, SwiftUI). Pay attention to whether the actions section scrolls. (There are also some height/padding issues, which will be fixed in a future PR.) Two buttons: image Three buttons: image Four buttons: image In SwiftUI, the action section seems to always have a minimum height of ~84 pixels regardless of the number of buttons. Therefore this PR also fixed this behavior, and adjusted the related tests. --- .../flutter/lib/src/cupertino/dialog.dart | 40 +++++-------------- .../test/cupertino/action_sheet_test.dart | 4 +- 2 files changed, 11 insertions(+), 33 deletions(-) diff --git a/packages/flutter/lib/src/cupertino/dialog.dart b/packages/flutter/lib/src/cupertino/dialog.dart index c49f2a56934e..0459c4e8e767 100644 --- a/packages/flutter/lib/src/cupertino/dialog.dart +++ b/packages/flutter/lib/src/cupertino/dialog.dart @@ -631,7 +631,6 @@ class _CupertinoActionSheetState extends State { child: _ActionSheetMainSheet( scrollController: _effectiveActionScrollController, hasContent: hasContent, - hasCancelButton: widget.cancelButton != null, contentSection: Builder(builder: _buildContent), actions: widget.actions, dividerColor: _kActionSheetButtonDividerColor, @@ -918,7 +917,6 @@ class _ActionSheetMainSheet extends StatefulWidget { required this.scrollController, required this.actions, required this.hasContent, - required this.hasCancelButton, required this.contentSection, required this.dividerColor, }); @@ -926,7 +924,6 @@ class _ActionSheetMainSheet extends StatefulWidget { final ScrollController? scrollController; final List? actions; final bool hasContent; - final bool hasCancelButton; final Widget contentSection; final Color dividerColor; @@ -980,25 +977,16 @@ class _ActionSheetMainSheetState extends State<_ActionSheetMainSheet> { bool _hasActions() => (widget.actions?.length ?? 0) != 0; - // If `aggressivelyLayout` is true, then the content section takes as much - // space as needed up to `maxHeight`. - // - // If `aggressivelyLayout` is false, then the content section takes whatever - // space is left by the other sections. - Widget _buildContent({ - required bool hasActions, - required bool aggressivelyLayout, - required double maxHeight, - }) { - if (hasActions && aggressivelyLayout) { - return ConstrainedBox( - constraints: BoxConstraints( - maxHeight: maxHeight, - ), + Widget _buildContent({required bool hasActions, required double maxHeight}) { + if (!hasActions) { + return Flexible( child: widget.contentSection, ); } - return Flexible( + return ConstrainedBox( + constraints: BoxConstraints( + maxHeight: maxHeight, + ), child: widget.contentSection, ); } @@ -1019,16 +1007,8 @@ class _ActionSheetMainSheetState extends State<_ActionSheetMainSheet> { @override Widget build(BuildContext context) { - // The layout rule: - // - // 1. If there are <= 3 buttons and a cancel button, or 1 button without a - // cancel button, then the actions section should never scroll. - // 2. Otherwise, then the content section takes priority to take over spaces - // but must leave at least `actionsMinHeight` for the actions section. - final int numActions = widget.actions?.length ?? 0; - final bool actionsMightScroll = - (numActions > 3 && widget.hasCancelButton) || - (numActions > 1 && !widget.hasCancelButton) ; + // The content section takes priority for vertical space but must leave at + // least `_kActionSheetActionsSectionMinHeight` for the actions section. final Color backgroundColor = CupertinoDynamicColor.resolve(_kActionSheetBackgroundColor, context); return LayoutBuilder( builder: (BuildContext context, BoxConstraints constraints) { @@ -1037,7 +1017,6 @@ class _ActionSheetMainSheetState extends State<_ActionSheetMainSheet> { children: [ _buildContent( hasActions: _hasActions(), - aggressivelyLayout: actionsMightScroll, maxHeight: constraints.maxHeight - _kActionSheetActionsSectionMinHeight - _kDividerThickness, ), if (widget.hasContent && _hasActions()) @@ -1046,7 +1025,6 @@ class _ActionSheetMainSheetState extends State<_ActionSheetMainSheet> { hidden: false, ), Flexible( - flex: actionsMightScroll ? 1 : 0, child: Stack( children: [ Positioned.fill( diff --git a/packages/flutter/test/cupertino/action_sheet_test.dart b/packages/flutter/test/cupertino/action_sheet_test.dart index aa7f2d7d9a56..90a285678ab6 100644 --- a/packages/flutter/test/cupertino/action_sheet_test.dart +++ b/packages/flutter/test/cupertino/action_sheet_test.dart @@ -719,7 +719,7 @@ void main() { await tester.tap(find.text('Go')); await tester.pump(); - expect(findScrollableActionsSectionRenderBox(tester).size.height, moreOrLessEquals(112.3)); + expect(findScrollableActionsSectionRenderBox(tester).size.height, moreOrLessEquals(84.3)); }); testWidgets('3 action buttons with cancel button', (WidgetTester tester) async { @@ -753,7 +753,7 @@ void main() { await tester.tap(find.text('Go')); await tester.pump(); - expect(findScrollableActionsSectionRenderBox(tester).size.height, moreOrLessEquals(168.6)); + expect(findScrollableActionsSectionRenderBox(tester).size.height, moreOrLessEquals(84.3)); }); testWidgets('4+ action buttons with cancel button', (WidgetTester tester) async {