[CupertinoActionSheet] Make _ActionSheetButtonBackground stateless (#152283)

dkwingsmt · web-flow · commit 3303973d99f6 · 2024-07-31T23:39:25.000Z
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.
diff --git 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<CupertinoActionSheet> {
+  int? _pressedIndex;
+  static const int _kCancelButtonIndex = -1;
+
   ScrollController? _backupMessageScrollController;
 
   ScrollController? _backupActionScrollController;
@@ -1004,6 +1007,20 @@ class _CupertinoActionSheetState extends State<CupertinoActionSheet> {
     );
   }
 
+  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<CupertinoActionSheet> {
       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<CupertinoActionSheet> {
           child: BackdropFilter(
             filter: ImageFilter.blur(sigmaX: _kBlurAmount, sigmaY: _kBlurAmount),
             child: _ActionSheetMainSheet(
+              pressedIndex: _pressedIndex,
+              onPressedUpdate: _onPressedUpdate,
               scrollController: _effectiveActionScrollController,
               contentSection: _buildContent(context),
               actions: widget.actions ?? List<Widget>.empty(),
@@ -1208,16 +1230,16 @@ class CupertinoActionSheetAction extends StatefulWidget {
 }
 
 class _CupertinoActionSheetActionState extends State<CupertinoActionSheetAction>
-    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<CupertinoActionSheetAction>
 
 // 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,86 +1612,69 @@ 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<Widget> 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: <Widget>[
-        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),
         ),
       ],
     );
   }
 
   @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
@@ -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);
   });
 
