From 3303973d99f6bf2437a7fd9d691991797fdf49ba Mon Sep 17 00:00:00 2001 From: Tong Mu Date: Wed, 31 Jul 2024 16:39:25 -0700 Subject: [PATCH] [CupertinoActionSheet] Make `_ActionSheetButtonBackground` stateless (#152283) This PR is a refactor that makes `_ActionSheetButtonBackground` widgets no longer record their own `pressed` state, but instead receive this state from their parent. In this way, `_ActionSheetButtonBackground` becomes a stateless widget. The children states are duplicate because the parent has to keep track of the state for rendering dividers. An obstacle with this change is that `_ActionSheetButtonBackground` needs an object that is persistent across rebuilds to provide to `Metadata.data`. Either it is kept as a stateful widget without any actual states, or it is made stateless and its `Element` as the object. After discussion, the first option is used. `_ActionSheetSlideTarget` is renamed to `_SlideTarget` since the alert dialog will soon use this class as well. This refactor shouldn't need additional tests. Still, one test is added for a behavior that I broke during development and found not covered by the unit tests then. --- .../flutter/lib/src/cupertino/dialog.dart | 171 +++++++++--------- .../test/cupertino/action_sheet_test.dart | 10 +- 2 files changed, 98 insertions(+), 83 deletions(-) diff --git a/packages/flutter/lib/src/cupertino/dialog.dart b/packages/flutter/lib/src/cupertino/dialog.dart index ffe7753a40..3c42b786cf 100644 --- a/packages/flutter/lib/src/cupertino/dialog.dart +++ b/packages/flutter/lib/src/cupertino/dialog.dart @@ -663,14 +663,14 @@ class _SlidingTapGestureRecognizer extends VerticalDragGestureRecognizer { // enters, leaves, or ends this `MetaData` widget, corresponding methods of this // class will be called. // -// Multiple `_ActionSheetSlideTarget`s might be nested. +// Multiple `_SlideTarget`s might be nested. // `_TargetSelectionGestureRecognizer` uses a simple algorithm that only // compares if the inner-most slide target has changed (which suffices our use // case). Semantically, this means that all outer targets will be treated as // identical to the inner-most one, i.e. when the pointer enters or leaves a // slide target, the corresponding method will be called on all targets that // nest it. -abstract class _ActionSheetSlideTarget { +abstract class _SlideTarget { // A pointer has entered this region. // // This includes: @@ -702,7 +702,7 @@ abstract class _ActionSheetSlideTarget { } // Recognizes sliding taps and thereupon interacts with -// `_ActionSheetSlideTarget`s. +// `_SlideTarget`s. class _TargetSelectionGestureRecognizer extends GestureRecognizer { _TargetSelectionGestureRecognizer({super.debugOwner, required this.hitTest}) : _slidingTap = _SlidingTapGestureRecognizer(debugOwner: debugOwner) { @@ -715,7 +715,7 @@ class _TargetSelectionGestureRecognizer extends GestureRecognizer { final _HitTester hitTest; - final List<_ActionSheetSlideTarget> _currentTargets = <_ActionSheetSlideTarget>[]; + final List<_SlideTarget> _currentTargets = <_SlideTarget>[]; final _SlidingTapGestureRecognizer _slidingTap; @override @@ -744,7 +744,7 @@ class _TargetSelectionGestureRecognizer extends GestureRecognizer { super.dispose(); } - // Collect the `_ActionSheetSlideTarget`s that are currently hit by the + // Collect the `_SlideTarget`s that are currently hit by the // pointer, check whether the current target have changed, and invoke their // methods if necessary. // @@ -755,27 +755,27 @@ class _TargetSelectionGestureRecognizer extends GestureRecognizer { // A slide target might nest other targets, therefore multiple targets might // be found. - final List<_ActionSheetSlideTarget> foundTargets = <_ActionSheetSlideTarget>[]; + final List<_SlideTarget> foundTargets = <_SlideTarget>[]; for (final HitTestEntry entry in result.path) { if (entry.target case final RenderMetaData target) { - if (target.metaData is _ActionSheetSlideTarget) { - foundTargets.add(target.metaData as _ActionSheetSlideTarget); + if (target.metaData is _SlideTarget) { + foundTargets.add(target.metaData as _SlideTarget); } } } // Compare whether the active target has changed by simply comparing the // first (inner-most) avatar of the nest, ignoring the cases where - // _currentTargets intersect with foundTargets (see _ActionSheetSlideTarget's + // _currentTargets intersect with foundTargets (see _SlideTarget's // document for more explanation). if (_currentTargets.firstOrNull != foundTargets.firstOrNull) { - for (final _ActionSheetSlideTarget target in _currentTargets) { + for (final _SlideTarget target in _currentTargets) { target.didLeave(); } _currentTargets ..clear() ..addAll(foundTargets); - for (final _ActionSheetSlideTarget target in _currentTargets) { + for (final _SlideTarget target in _currentTargets) { target.didEnter(fromPointerDown: fromPointerDown); } } @@ -791,14 +791,14 @@ class _TargetSelectionGestureRecognizer extends GestureRecognizer { void _onEnd(Offset globalPosition) { _updateDrag(globalPosition, fromPointerDown: false); - for (final _ActionSheetSlideTarget target in _currentTargets) { + for (final _SlideTarget target in _currentTargets) { target.didConfirm(); } _currentTargets.clear(); } void _onCancel() { - for (final _ActionSheetSlideTarget target in _currentTargets) { + for (final _SlideTarget target in _currentTargets) { target.didLeave(); } _currentTargets.clear(); @@ -949,6 +949,9 @@ class CupertinoActionSheet extends StatefulWidget { } class _CupertinoActionSheetState extends State { + int? _pressedIndex; + static const int _kCancelButtonIndex = -1; + ScrollController? _backupMessageScrollController; ScrollController? _backupActionScrollController; @@ -1004,6 +1007,20 @@ class _CupertinoActionSheetState extends State { ); } + void _onPressedUpdate(int actionIndex, bool state) { + if (!state) { + if (_pressedIndex == actionIndex) { + setState(() { + _pressedIndex = null; + }); + } + } else { + setState(() { + _pressedIndex = actionIndex; + }); + } + } + Widget _buildCancelButton() { assert(widget.cancelButton != null); final double cancelPadding = (widget.actions != null || widget.message != null || widget.title != null) @@ -1012,7 +1029,10 @@ class _CupertinoActionSheetState extends State { padding: EdgeInsets.only(top: cancelPadding), child: _ActionSheetButtonBackground( isCancel: true, - onPressStateChange: (_) {}, + pressed: _pressedIndex == _kCancelButtonIndex, + onPressStateChange: (bool state) { + _onPressedUpdate(_kCancelButtonIndex, state); + }, child: widget.cancelButton!, ), ); @@ -1106,6 +1126,8 @@ class _CupertinoActionSheetState extends State { child: BackdropFilter( filter: ImageFilter.blur(sigmaX: _kBlurAmount, sigmaY: _kBlurAmount), child: _ActionSheetMainSheet( + pressedIndex: _pressedIndex, + onPressedUpdate: _onPressedUpdate, scrollController: _effectiveActionScrollController, contentSection: _buildContent(context), actions: widget.actions ?? List.empty(), @@ -1208,16 +1230,16 @@ class CupertinoActionSheetAction extends StatefulWidget { } class _CupertinoActionSheetActionState extends State - implements _ActionSheetSlideTarget { - // |_ActionSheetSlideTarget| + implements _SlideTarget { + // |_SlideTarget| @override void didEnter({required bool fromPointerDown}) {} - // |_ActionSheetSlideTarget| + // |_SlideTarget| @override void didLeave() {} - // |_ActionSheetSlideTarget| + // |_SlideTarget| @override void didConfirm() { widget.onPressed(); @@ -1307,15 +1329,23 @@ class _CupertinoActionSheetActionState extends State // Renders the background of a button (both the pressed background and the idle // background) and reports its state to the parent with `onPressStateChange`. +// +// Although this class doesn't keep any states, it's still a stateful widget +// because the state is used as a persistent object across rebuilds to provide +// to [MetaData.data]. class _ActionSheetButtonBackground extends StatefulWidget { const _ActionSheetButtonBackground({ this.isCancel = false, + required this.pressed, this.onPressStateChange, required this.child, }); final bool isCancel; + /// Whether the user is holding on this button. + final bool pressed; + /// Called when the user taps down or lifts up on the button. /// /// The boolean value is true if the user is tapping down on the button. @@ -1330,9 +1360,7 @@ class _ActionSheetButtonBackground extends StatefulWidget { _ActionSheetButtonBackgroundState createState() => _ActionSheetButtonBackgroundState(); } -class _ActionSheetButtonBackgroundState extends State<_ActionSheetButtonBackground> implements _ActionSheetSlideTarget { - bool isBeingPressed = false; - +class _ActionSheetButtonBackgroundState extends State<_ActionSheetButtonBackground> implements _SlideTarget { void _emitVibration(){ switch (defaultTargetPlatform) { case TargetPlatform.iOS: @@ -1346,27 +1374,24 @@ class _ActionSheetButtonBackgroundState extends State<_ActionSheetButtonBackgrou } } - // |_ActionSheetSlideTarget| + // |_SlideTarget| @override void didEnter({required bool fromPointerDown}) { - setState(() { isBeingPressed = true; }); widget.onPressStateChange?.call(true); if (!fromPointerDown) { _emitVibration(); } } - // |_ActionSheetSlideTarget| + // |_SlideTarget| @override void didLeave() { - setState(() { isBeingPressed = false; }); widget.onPressStateChange?.call(false); } - // |_ActionSheetSlideTarget| + // |_SlideTarget| @override void didConfirm() { - setState(() { isBeingPressed = false; }); widget.onPressStateChange?.call(false); } @@ -1375,11 +1400,11 @@ class _ActionSheetButtonBackgroundState extends State<_ActionSheetButtonBackgrou late final Color backgroundColor; BorderRadius? borderRadius; if (!widget.isCancel) { - backgroundColor = isBeingPressed + backgroundColor = widget.pressed ? _kActionSheetPressedColor : _kActionSheetBackgroundColor; } else { - backgroundColor = isBeingPressed + backgroundColor = widget.pressed ? _kActionSheetCancelPressedColor : _kActionSheetCancelColor; borderRadius = const BorderRadius.all(Radius.circular(_kCornerRadius)); @@ -1566,6 +1591,7 @@ class _ActionSheetActionSection extends StatelessWidget { )); } column.add(_ActionSheetButtonBackground( + pressed: pressedIndex == actionIndex, onPressStateChange: (bool state) { onPressedUpdate(actionIndex, state); }, @@ -1586,71 +1612,52 @@ class _ActionSheetActionSection extends StatelessWidget { } // The part of an action sheet without the cancel button. -class _ActionSheetMainSheet extends StatefulWidget { +class _ActionSheetMainSheet extends StatelessWidget { const _ActionSheetMainSheet({ + required this.pressedIndex, + required this.onPressedUpdate, required this.scrollController, required this.actions, required this.contentSection, required this.dividerColor, }); + final int? pressedIndex; + final _PressedUpdateHandler onPressedUpdate; final ScrollController? scrollController; final List actions; final Widget? contentSection; final Color dividerColor; - @override - _ActionSheetMainSheetState createState() => _ActionSheetMainSheetState(); -} - -class _ActionSheetMainSheetState extends State<_ActionSheetMainSheet> { - int? _pressedIndex; - - bool get _hasContent => widget.contentSection != null; - bool get _hasActions => widget.actions.isNotEmpty; - - void _onPressedUpdate(int actionIndex, bool state) { - if (!state) { - if (_pressedIndex == actionIndex) { - setState(() { - _pressedIndex = null; - }); - } - } else { - setState(() { - _pressedIndex = actionIndex; - }); - } + Widget _scrolledActionsSection(BuildContext context) { + final Color backgroundColor = CupertinoDynamicColor.resolve(_kActionSheetBackgroundColor, context); + return _OverscrollBackground( + scrollController: scrollController, + color: backgroundColor, + child: _ActionSheetActionSection( + actions: actions, + scrollController: scrollController, + dividerColor: dividerColor, + backgroundColor: backgroundColor, + pressedIndex: pressedIndex, + onPressedUpdate: onPressedUpdate, + ), + ); } Widget _dividerAndActionsSection(BuildContext context) { - if (!_hasActions) { - return _empty; - } final Color backgroundColor = CupertinoDynamicColor.resolve(_kActionSheetBackgroundColor, context); return Column( mainAxisSize: MainAxisSize.min, crossAxisAlignment: CrossAxisAlignment.stretch, children: [ - if (_hasContent) - _Divider( - dividerColor: widget.dividerColor, - hiddenColor: backgroundColor, - hidden: false, - ), + _Divider( + dividerColor: dividerColor, + hiddenColor: backgroundColor, + hidden: false, + ), Flexible( - child: _OverscrollBackground( - scrollController: widget.scrollController, - color: backgroundColor, - child: _ActionSheetActionSection( - actions: widget.actions, - scrollController: widget.scrollController, - pressedIndex: _pressedIndex, - dividerColor: widget.dividerColor, - backgroundColor: backgroundColor, - onPressedUpdate: _onPressedUpdate, - ), - ), + child: _scrolledActionsSection(context), ), ], ); @@ -1658,14 +1665,16 @@ class _ActionSheetMainSheetState extends State<_ActionSheetMainSheet> { @override Widget build(BuildContext context) { - return LayoutBuilder( - builder: (BuildContext context, BoxConstraints constraints) { - return _PriorityColumn( - top: widget.contentSection ?? _empty, - bottom: _dividerAndActionsSection(context), - bottomMinHeight: _kActionSheetActionsSectionMinHeight + _kDividerThickness, - ); - }, + if (actions.isEmpty) { + return contentSection ?? _empty; + } + if (contentSection == null) { + return _scrolledActionsSection(context); + } + return _PriorityColumn( + top: contentSection!, + bottom: _dividerAndActionsSection(context), + bottomMinHeight: _kActionSheetActionsSectionMinHeight + _kDividerThickness, ); } diff --git a/packages/flutter/test/cupertino/action_sheet_test.dart b/packages/flutter/test/cupertino/action_sheet_test.dart index 80319a8027..274716beb9 100644 --- a/packages/flutter/test/cupertino/action_sheet_test.dart +++ b/packages/flutter/test/cupertino/action_sheet_test.dart @@ -1484,13 +1484,19 @@ void main() { expect(wasPressed, isFalse); - await tester.tap(find.text('Cancel')); + final TestGesture gesture = await tester.startGesture(tester.getCenter(find.text('Cancel'))); + await tester.pumpAndSettle(); + // Verify that the cancel button shows the pressed color. + await expectLater( + find.byType(CupertinoActionSheet), + matchesGoldenFile('cupertinoActionSheet.pressedCancel.png'), + ); + await gesture.up(); expect(wasPressed, isTrue); await tester.pump(); await tester.pump(const Duration(seconds: 1)); - expect(find.text('Cancel'), findsNothing); });