Skip to content

F
Front-End
Unverified Commit 854bbb90 authored by Kostia Sokolovskyi's avatar Kostia Sokolovskyi Committed by GitHub
Browse files
Options

Fix memory leak in CupertinoActionSheet (#134885)

parent 60fae58a
Hide whitespace changes
Inline Side-by-side
Showing with 73 additions and 51 deletions
packages/flutter/lib/src/cupertino/dialog.dart
View file @ 854bbb90
......@@ -474,7 +474,7 @@ class CupertinoPopupSurface extends StatelessWidget {
///
/// * [CupertinoActionSheetAction], which is an iOS-style action sheet button.
/// * <https://developer.apple.com/design/human-interface-guidelines/ios/views/action-sheets/>
class CupertinoActionSheet extends StatelessWidget {
class CupertinoActionSheet extends StatefulWidget {
/// Creates an iOS-style action sheet.
///
/// An action sheet must have a non-null value for at least one of the
......@@ -520,30 +520,46 @@ class CupertinoActionSheet extends StatelessWidget {
/// short.
final ScrollController? messageScrollController;
ScrollController get _effectiveMessageScrollController =>
messageScrollController ?? ScrollController();
/// A scroll controller that can be used to control the scrolling of the
/// [actions] in the action sheet.
///
/// This attribute is typically not needed.
final ScrollController? actionScrollController;
ScrollController get _effectiveActionScrollController =>
actionScrollController ?? ScrollController();
/// The optional cancel button that is grouped separately from the other
/// actions.
///
/// Typically this is an [CupertinoActionSheetAction] widget.
final Widget? cancelButton;
@override
State<CupertinoActionSheet> createState() => _CupertinoActionSheetState();
}
class _CupertinoActionSheetState extends State<CupertinoActionSheet> {
ScrollController? _backupMessageScrollController;
ScrollController? _backupActionScrollController;
ScrollController get _effectiveMessageScrollController =>
widget.messageScrollController ?? (_backupMessageScrollController ??= ScrollController());
ScrollController get _effectiveActionScrollController =>
widget.actionScrollController ?? (_backupActionScrollController ??= ScrollController());
@override
void dispose() {
_backupMessageScrollController?.dispose();
_backupActionScrollController?.dispose();
super.dispose();
}
Widget _buildContent(BuildContext context) {
final List<Widget> content = <Widget>[];
if (title != null || message != null) {
if (widget.title != null || widget.message != null) {
final Widget titleSection = _CupertinoAlertContentSection(
title: title,
message: message,
title: widget.title,
message: widget.message,
scrollController: _effectiveMessageScrollController,
titlePadding: const EdgeInsets.only(
left: _kActionSheetContentHorizontalPadding,
......@@ -554,13 +570,13 @@ class CupertinoActionSheet extends StatelessWidget {
messagePadding: EdgeInsets.only(
left: _kActionSheetContentHorizontalPadding,
right: _kActionSheetContentHorizontalPadding,
bottom: title == null ? _kActionSheetContentVerticalPadding : 22.0,
top: title == null ? _kActionSheetContentVerticalPadding : 0.0,
bottom: widget.title == null ? _kActionSheetContentVerticalPadding : 22.0,
top: widget.title == null ? _kActionSheetContentVerticalPadding : 0.0,
),
titleTextStyle: message == null
titleTextStyle: widget.message == null
? _kActionSheetContentStyle
: _kActionSheetContentStyle.copyWith(fontWeight: FontWeight.w600),
messageTextStyle: title == null
messageTextStyle: widget.title == null
? _kActionSheetContentStyle.copyWith(fontWeight: FontWeight.w600)
: _kActionSheetContentStyle,
additionalPaddingBetweenTitleAndMessage: const EdgeInsets.only(top: 8.0),
......@@ -579,26 +595,26 @@ class CupertinoActionSheet extends StatelessWidget {
}
Widget _buildActions() {
if (actions == null || actions!.isEmpty) {
if (widget.actions == null || widget.actions!.isEmpty) {
return Container(
height: 0.0,
);
}
return _CupertinoAlertActionSection(
scrollController: _effectiveActionScrollController,
hasCancelButton: cancelButton != null,
hasCancelButton: widget.cancelButton != null,
isActionSheet: true,
children: actions!,
children: widget.actions!,
);
}
Widget _buildCancelButton() {
final double cancelPadding = (actions != null || message != null || title != null)
final double cancelPadding = (widget.actions != null || widget.message != null || widget.title != null)
? _kActionSheetCancelButtonPadding : 0.0;
return Padding(
padding: EdgeInsets.only(top: cancelPadding),
child: _CupertinoActionSheetCancelButton(
child: cancelButton,
child: widget.cancelButton,
),
);
}
......@@ -621,7 +637,7 @@ class CupertinoActionSheet extends StatelessWidget {
),
),
),
if (cancelButton != null) _buildCancelButton(),
if (widget.cancelButton != null) _buildCancelButton(),
];
final Orientation orientation = MediaQuery.orientationOf(context);
......
packages/flutter/test/cupertino/action_sheet_test.dart
View file @ 854bbb90
......@@ -9,11 +9,12 @@ import 'package:flutter/material.dart';
import 'package:flutter/rendering.dart';
import 'package:flutter_test/flutter_test.dart';
import 'package:leak_tracker_flutter_testing/leak_tracker_flutter_testing.dart';
import '../widgets/semantics_tester.dart';
void main() {
testWidgets('Verify that a tap on modal barrier dismisses an action sheet', (WidgetTester tester) async {
testWidgetsWithLeakTracking('Verify that a tap on modal barrier dismisses an action sheet', (WidgetTester tester) async {
await tester.pumpWidget(
createAppWithButtonThatLaunchesActionSheet(
const CupertinoActionSheet(
......@@ -30,9 +31,9 @@ void main() {
await tester.tapAt(const Offset(20.0, 20.0));
await tester.pump();
expect(find.text('Action Sheet'), findsNothing);
});
}, leakTrackingTestConfig: LeakTrackingTestConfig.debugNotDisposed());
testWidgets('Verify that a tap on title section (not buttons) does not dismiss an action sheet', (WidgetTester tester) async {
testWidgetsWithLeakTracking('Verify that a tap on title section (not buttons) does not dismiss an action sheet', (WidgetTester tester) async {
await tester.pumpWidget(
createAppWithButtonThatLaunchesActionSheet(
const CupertinoActionSheet(
......@@ -52,7 +53,7 @@ void main() {
expect(find.text('Action Sheet'), findsOneWidget);
});
testWidgets('Action sheet destructive text style', (WidgetTester tester) async {
testWidgetsWithLeakTracking('Action sheet destructive text style', (WidgetTester tester) async {
await tester.pumpWidget(
boilerplate(
CupertinoActionSheetAction(
......@@ -73,7 +74,7 @@ void main() {
));
});
testWidgets('Action sheet dark mode', (WidgetTester tester) async {
testWidgetsWithLeakTracking('Action sheet dark mode', (WidgetTester tester) async {
final Widget action = CupertinoActionSheetAction(
child: const Text('action'),
onPressed: () {},
......@@ -130,7 +131,7 @@ void main() {
);
});
testWidgets('Action sheet default text style', (WidgetTester tester) async {
testWidgetsWithLeakTracking('Action sheet default text style', (WidgetTester tester) async {
await tester.pumpWidget(
boilerplate(
CupertinoActionSheetAction(
......@@ -146,7 +147,7 @@ void main() {
expect(widget.style.fontWeight, equals(FontWeight.w600));
});
testWidgets('Action sheet text styles are correct when both title and message are included', (WidgetTester tester) async {
testWidgetsWithLeakTracking('Action sheet text styles are correct when both title and message are included', (WidgetTester tester) async {
await tester.pumpWidget(
createAppWithButtonThatLaunchesActionSheet(
const CupertinoActionSheet(
......@@ -166,7 +167,7 @@ void main() {
expect(messageStyle.style.fontWeight, FontWeight.w400);
});
testWidgets('Action sheet text styles are correct when title but no message is included', (WidgetTester tester) async {
testWidgetsWithLeakTracking('Action sheet text styles are correct when title but no message is included', (WidgetTester tester) async {
await tester.pumpWidget(
createAppWithButtonThatLaunchesActionSheet(
const CupertinoActionSheet(
......@@ -183,7 +184,7 @@ void main() {
expect(titleStyle.style.fontWeight, FontWeight.w400);
});
testWidgets('Action sheet text styles are correct when message but no title is included', (WidgetTester tester) async {
testWidgetsWithLeakTracking('Action sheet text styles are correct when message but no title is included', (WidgetTester tester) async {
await tester.pumpWidget(
createAppWithButtonThatLaunchesActionSheet(
const CupertinoActionSheet(
......@@ -200,8 +201,9 @@ void main() {
expect(messageStyle.style.fontWeight, FontWeight.w600);
});
testWidgets('Content section but no actions', (WidgetTester tester) async {
testWidgetsWithLeakTracking('Content section but no actions', (WidgetTester tester) async {
final ScrollController scrollController = ScrollController();
addTearDown(scrollController.dispose);
await tester.pumpWidget(
createAppWithButtonThatLaunchesActionSheet(
CupertinoActionSheet(
......@@ -237,8 +239,9 @@ void main() {
);
});
testWidgets('Actions but no content section', (WidgetTester tester) async {
testWidgetsWithLeakTracking('Actions but no content section', (WidgetTester tester) async {
final ScrollController actionScrollController = ScrollController();
addTearDown(actionScrollController.dispose);
await tester.pumpWidget(
createAppWithButtonThatLaunchesActionSheet(
CupertinoActionSheet(
......@@ -285,8 +288,9 @@ void main() {
);
});
testWidgets('Action section is scrollable', (WidgetTester tester) async {
testWidgetsWithLeakTracking('Action section is scrollable', (WidgetTester tester) async {
final ScrollController actionScrollController = ScrollController();
addTearDown(actionScrollController.dispose);
await tester.pumpWidget(
createAppWithButtonThatLaunchesActionSheet(
Builder(builder: (BuildContext context) {
......@@ -350,8 +354,9 @@ void main() {
expect(tester.getSize(find.widgetWithText(CupertinoActionSheetAction, 'Five')).height, equals(92.0));
});
testWidgets('Content section is scrollable', (WidgetTester tester) async {
testWidgetsWithLeakTracking('Content section is scrollable', (WidgetTester tester) async {
final ScrollController messageScrollController = ScrollController();
addTearDown(messageScrollController.dispose);
late double screenHeight;
await tester.pumpWidget(
createAppWithButtonThatLaunchesActionSheet(
......@@ -392,7 +397,7 @@ void main() {
expect(tester.getSize(find.byType(CupertinoActionSheet)).height, screenHeight);
});
testWidgets('CupertinoActionSheet scrollbars controllers should be different', (WidgetTester tester) async {
testWidgetsWithLeakTracking('CupertinoActionSheet scrollbars controllers should be different', (WidgetTester tester) async {
// https://github.com/flutter/flutter/pull/81278
await tester.pumpWidget(
createAppWithButtonThatLaunchesActionSheet(
......@@ -422,7 +427,7 @@ void main() {
expect(scrollbars[0].controller != scrollbars[1].controller, isTrue);
});
testWidgets('Tap on button calls onPressed', (WidgetTester tester) async {
testWidgetsWithLeakTracking('Tap on button calls onPressed', (WidgetTester tester) async {
bool wasPressed = false;
await tester.pumpWidget(
createAppWithButtonThatLaunchesActionSheet(
......@@ -459,7 +464,7 @@ void main() {
expect(find.text('One'), findsNothing);
});
testWidgets('Action sheet width is correct when given infinite horizontal space', (WidgetTester tester) async {
testWidgetsWithLeakTracking('Action sheet width is correct when given infinite horizontal space', (WidgetTester tester) async {
await tester.pumpWidget(
createAppWithButtonThatLaunchesActionSheet(
Row(
......@@ -487,7 +492,7 @@ void main() {
expect(tester.getSize(find.byType(CupertinoActionSheet)).width, 600.0);
});
testWidgets('Action sheet height is correct when given infinite vertical space', (WidgetTester tester) async {
testWidgetsWithLeakTracking('Action sheet height is correct when given infinite vertical space', (WidgetTester tester) async {
await tester.pumpWidget(
createAppWithButtonThatLaunchesActionSheet(
Column(
......@@ -515,7 +520,7 @@ void main() {
expect(tester.getSize(find.byType(CupertinoActionSheet)).height, moreOrLessEquals(132.33333333333334));
});
testWidgets('1 action button with cancel button', (WidgetTester tester) async {
testWidgetsWithLeakTracking('1 action button with cancel button', (WidgetTester tester) async {
await tester.pumpWidget(
createAppWithButtonThatLaunchesActionSheet(
CupertinoActionSheet(
......@@ -542,7 +547,7 @@ void main() {
expect(findScrollableActionsSectionRenderBox(tester).size.height, 56.0);
});
testWidgets('2 action buttons with cancel button', (WidgetTester tester) async {
testWidgetsWithLeakTracking('2 action buttons with cancel button', (WidgetTester tester) async {
await tester.pumpWidget(
createAppWithButtonThatLaunchesActionSheet(
CupertinoActionSheet(
......@@ -572,7 +577,7 @@ void main() {
expect(findScrollableActionsSectionRenderBox(tester).size.height, moreOrLessEquals(112.33333333333331));
});
testWidgets('3 action buttons with cancel button', (WidgetTester tester) async {
testWidgetsWithLeakTracking('3 action buttons with cancel button', (WidgetTester tester) async {
await tester.pumpWidget(
createAppWithButtonThatLaunchesActionSheet(
CupertinoActionSheet(
......@@ -606,7 +611,7 @@ void main() {
expect(findScrollableActionsSectionRenderBox(tester).size.height, moreOrLessEquals(168.66666666666669));
});
testWidgets('4+ action buttons with cancel button', (WidgetTester tester) async {
testWidgetsWithLeakTracking('4+ action buttons with cancel button', (WidgetTester tester) async {
await tester.pumpWidget(
createAppWithButtonThatLaunchesActionSheet(
CupertinoActionSheet(
......@@ -644,7 +649,7 @@ void main() {
expect(findScrollableActionsSectionRenderBox(tester).size.height, moreOrLessEquals(84.33333333333337));
});
testWidgets('1 action button without cancel button', (WidgetTester tester) async {
testWidgetsWithLeakTracking('1 action button without cancel button', (WidgetTester tester) async {
await tester.pumpWidget(
createAppWithButtonThatLaunchesActionSheet(
CupertinoActionSheet(
......@@ -666,7 +671,7 @@ void main() {
expect(findScrollableActionsSectionRenderBox(tester).size.height, 56.0);
});
testWidgets('2+ action buttons without cancel button', (WidgetTester tester) async {
testWidgetsWithLeakTracking('2+ action buttons without cancel button', (WidgetTester tester) async {
await tester.pumpWidget(
createAppWithButtonThatLaunchesActionSheet(
CupertinoActionSheet(
......@@ -692,7 +697,7 @@ void main() {
expect(findScrollableActionsSectionRenderBox(tester).size.height, moreOrLessEquals(84.33333333333337));
});
testWidgets('Action sheet with just cancel button is correct', (WidgetTester tester) async {
testWidgetsWithLeakTracking('Action sheet with just cancel button is correct', (WidgetTester tester) async {
await tester.pumpWidget(
createAppWithButtonThatLaunchesActionSheet(
CupertinoActionSheet(
......@@ -712,7 +717,7 @@ void main() {
expect(tester.getSize(find.byType(CupertinoActionSheet)).width, 600.0);
});
testWidgets('Cancel button tap calls onPressed', (WidgetTester tester) async {
testWidgetsWithLeakTracking('Cancel button tap calls onPressed', (WidgetTester tester) async {
bool wasPressed = false;
await tester.pumpWidget(
createAppWithButtonThatLaunchesActionSheet(
......@@ -747,7 +752,7 @@ void main() {
expect(find.text('Cancel'), findsNothing);
});
testWidgets('Layout is correct when cancel button is present', (WidgetTester tester) async {
testWidgetsWithLeakTracking('Layout is correct when cancel button is present', (WidgetTester tester) async {
await tester.pumpWidget(
createAppWithButtonThatLaunchesActionSheet(
CupertinoActionSheet(
......@@ -784,7 +789,7 @@ void main() {
expect(tester.getBottomLeft(find.widgetWithText(CupertinoActionSheetAction, 'Two')).dy, 526.0);
});
testWidgets('Enter/exit animation is correct', (WidgetTester tester) async {
testWidgetsWithLeakTracking('Enter/exit animation is correct', (WidgetTester tester) async {
await tester.pumpWidget(
createAppWithButtonThatLaunchesActionSheet(
CupertinoActionSheet(
......@@ -861,7 +866,7 @@ void main() {
expect(find.byType(CupertinoActionSheet), findsNothing);
});
testWidgets('Modal barrier is pressed during transition', (WidgetTester tester) async {
testWidgetsWithLeakTracking('Modal barrier is pressed during transition', (WidgetTester tester) async {
await tester.pumpWidget(
createAppWithButtonThatLaunchesActionSheet(
CupertinoActionSheet(
......@@ -919,7 +924,7 @@ void main() {
});
testWidgets('Action sheet semantics', (WidgetTester tester) async {
testWidgetsWithLeakTracking('Action sheet semantics', (WidgetTester tester) async {
final SemanticsTester semantics = SemanticsTester(tester);
await tester.pumpWidget(
......@@ -1028,9 +1033,10 @@ void main() {
semantics.dispose();
});
testWidgets('Conflicting scrollbars are not applied by ScrollBehavior to CupertinoActionSheet', (WidgetTester tester) async {
testWidgetsWithLeakTracking('Conflicting scrollbars are not applied by ScrollBehavior to CupertinoActionSheet', (WidgetTester tester) async {
// Regression test for https://github.com/flutter/flutter/issues/83819
final ScrollController actionScrollController = ScrollController();
addTearDown(actionScrollController.dispose);
await tester.pumpWidget(
createAppWithButtonThatLaunchesActionSheet(
Builder(builder: (BuildContext context) {
......@@ -1068,7 +1074,7 @@ void main() {
expect(find.byType(CupertinoScrollbar), findsNWidgets(2));
}, variant: TargetPlatformVariant.all());
testWidgets('Hovering over Cupertino action sheet action updates cursor to clickable on Web', (WidgetTester tester) async {
testWidgetsWithLeakTracking('Hovering over Cupertino action sheet action updates cursor to clickable on Web', (WidgetTester tester) async {
await tester.pumpWidget(
createAppWithButtonThatLaunchesActionSheet(
CupertinoActionSheet(
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment

HIAST - GitLab