From 8ec5001ac2614e5dcc3eac8242896bfc7ea4e5a5 Mon Sep 17 00:00:00 2001 From: Pierre-Louis <6655696+guidezpl@users.noreply.github.com> Date: Thu, 4 May 2023 19:27:11 +0200 Subject: [PATCH] Provide default constraints for M3 dialogs (#120082) This PR constrains M3 dialogs to 560dp max width and height. This is not a breaking change per the breaking change policy. Part of https://github.com/flutter/flutter/issues/118619 ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [ ] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. [Contributor Guide]: https://github.com/flutter/flutter/wiki/Tree-hygiene#overview [Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene [test-exempt]: https://github.com/flutter/flutter/wiki/Tree-hygiene#tests [Flutter Style Guide]: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo [Features we expect every widget to implement]: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/wiki/Chat --- packages/flutter/lib/src/material/dialog.dart | 17 +++++- .../flutter/test/material/dialog_test.dart | 54 ++++++++++++------- .../test/material/time_picker_test.dart | 23 ++++---- 3 files changed, 63 insertions(+), 31 deletions(-) diff --git a/packages/flutter/lib/src/material/dialog.dart b/packages/flutter/lib/src/material/dialog.dart index 9f3a967bfc..63d2b42740 100644 --- a/packages/flutter/lib/src/material/dialog.dart +++ b/packages/flutter/lib/src/material/dialog.dart @@ -24,6 +24,8 @@ import 'theme_data.dart'; const EdgeInsets _defaultInsetPadding = EdgeInsets.symmetric(horizontal: 40.0, vertical: 24.0); +const BoxConstraints _dialogConstraints = BoxConstraints(minWidth: 280.0, maxWidth: 560.0, maxHeight: 560.0); + /// A Material Design dialog. /// /// This dialog widget does not have any opinion about the contents of the @@ -232,7 +234,7 @@ class Dialog extends StatelessWidget { dialogChild = Align( alignment: alignment ?? dialogTheme.alignment ?? defaults.alignment!, child: ConstrainedBox( - constraints: const BoxConstraints(minWidth: 280.0), + constraints: _constraintsScaleFactor(MediaQuery.of(context).textScaleFactor, theme.useMaterial3), child: Material( color: backgroundColor ?? dialogTheme.backgroundColor ?? Theme.of(context).dialogBackgroundColor, elevation: elevation ?? dialogTheme.elevation ?? defaults.elevation!, @@ -1261,7 +1263,7 @@ class SimpleDialog extends StatelessWidget { Widget dialogChild = IntrinsicWidth( stepWidth: 56.0, child: ConstrainedBox( - constraints: const BoxConstraints(minWidth: 280.0), + constraints: _constraintsScaleFactor(MediaQuery.of(context).textScaleFactor, theme.useMaterial3), child: Column( mainAxisSize: MainAxisSize.min, crossAxisAlignment: CrossAxisAlignment.stretch, @@ -1587,6 +1589,17 @@ double _paddingScaleFactor(double textScaleFactor) { return lerpDouble(1.0, 1.0 / 3.0, clampedTextScaleFactor - 1.0)!; } +BoxConstraints _constraintsScaleFactor(double textScaleFactor, bool useMaterial3) { + if (!useMaterial3) { + return const BoxConstraints(minWidth: 280.0); + } else { + return textScaleFactor == 1.0 + ? _dialogConstraints + // Scale the max height of the dialog by the text scale factor to aid in readability. + : _dialogConstraints.copyWith(maxHeight: _dialogConstraints.maxHeight * textScaleFactor); + } +} + // Hand coded defaults based on Material Design 2. class _DialogDefaultsM2 extends DialogTheme { _DialogDefaultsM2(this.context) diff --git a/packages/flutter/test/material/dialog_test.dart b/packages/flutter/test/material/dialog_test.dart index df5c649323..f010e51410 100644 --- a/packages/flutter/test/material/dialog_test.dart +++ b/packages/flutter/test/material/dialog_test.dart @@ -5,6 +5,7 @@ import 'dart:ui'; import 'package:flutter/cupertino.dart'; +import 'package:flutter/foundation.dart'; import 'package:flutter/material.dart'; import 'package:flutter/rendering.dart'; import 'package:flutter/services.dart'; @@ -61,6 +62,16 @@ Finder _findButtonBar() { return find.ancestor(of: find.byType(OverflowBar), matching: find.byType(Padding)).first; } +// In the case of [AlertDialog], it takes up the entire screen, since it also +// contains the scrim. The first [Material] child of [AlertDialog] is the actual +// dialog itself. +Size _getDialogSize(WidgetTester tester) => tester.getSize( + find.descendant( + of: find.byType(AlertDialog), + matching: find.byType(Material), + ).first, +); + const ShapeBorder _defaultM2DialogShape = RoundedRectangleBorder(borderRadius: BorderRadius.all(Radius.circular(4.0))); final ShapeBorder _defaultM3DialogShape = RoundedRectangleBorder(borderRadius: BorderRadius.circular(28.0)); @@ -144,6 +155,8 @@ void main() { expect(material3Widget.color, material3Theme.colorScheme.surface); expect(material3Widget.shape, _defaultM3DialogShape); expect(material3Widget.elevation, 6.0); + // For some unknown reason, one pixel wider on web (HTML). + expect(_getDialogSize(tester), Size(280.0, isBrowser && !isCanvasKit ? 141.0 : 140.0)); }); testWidgets('Dialog.fullscreen Defaults', (WidgetTester tester) async { @@ -337,6 +350,26 @@ void main() { expect(bottomLeft.dy, 576.0); }); + testWidgets('Dialog respects constraints with large content on large screens', (WidgetTester tester) async { + const AlertDialog dialog = AlertDialog( + actions: [ ], + content: SizedBox( + width: 1000.0, + height: 1000.0, + ), + ); + + tester.view.physicalSize = const Size(2000, 2000); + addTearDown(tester.view.resetPhysicalSize); + + await tester.pumpWidget(_buildAppWithDialog(dialog, theme: material3Theme)); + + await tester.tap(find.text('X')); + await tester.pumpAndSettle(); + + expect(_getDialogSize(tester), const Size(560.0, 560.0)); + }); + testWidgets('Simple dialog control test', (WidgetTester tester) async { await tester.pumpWidget( const MaterialApp( @@ -593,15 +626,8 @@ void main() { await tester.tap(find.text('X')); await tester.pumpAndSettle(); - // The [AlertDialog] is the entire screen, since it also contains the scrim. - // The first [Material] child of [AlertDialog] is the actual dialog - // itself. - final Size dialogSize = tester.getSize( - find.descendant( - of: find.byType(AlertDialog), - matching: find.byType(Material), - ).first, - ); + + final Size dialogSize = _getDialogSize(tester); final Size actionsSize = tester.getSize(_findButtonBar()); expect(actionsSize.width, dialogSize.width); @@ -629,15 +655,7 @@ void main() { await tester.tap(find.text('X')); await tester.pumpAndSettle(); - // The [AlertDialog] is the entire screen, since it also contains the scrim. - // The first [Material] child of [AlertDialog] is the actual dialog - // itself. - final Size dialogSize = tester.getSize( - find.descendant( - of: find.byType(AlertDialog), - matching: find.byType(Material), - ).first, - ); + final Size dialogSize = _getDialogSize(tester); final Size actionsSize = tester.getSize(find.byType(OverflowBar)); expect(actionsSize.width, dialogSize.width - (30.0 * 2)); diff --git a/packages/flutter/test/material/time_picker_test.dart b/packages/flutter/test/material/time_picker_test.dart index 4ecec0f234..be79d70843 100644 --- a/packages/flutter/test/material/time_picker_test.dart +++ b/packages/flutter/test/material/time_picker_test.dart @@ -5,6 +5,7 @@ @TestOn('!chrome') library; +import 'dart:math'; import 'dart:ui'; import 'package:flutter/material.dart'; @@ -426,7 +427,7 @@ void main() { tester.view.devicePixelRatio = 1; await mediaQueryBoilerplate(tester, materialType: materialType); - width = timePickerPortraitSize.width + padding.horizontal; + width = min(timePickerPortraitSize.width + padding.horizontal, 560); // width is limited to 560 height = timePickerPortraitSize.height + padding.vertical; expect( tester.getSize(find.byWidget(getMaterialFromDialog(tester))), @@ -445,7 +446,7 @@ void main() { materialType: materialType, ); - width = timePickerLandscapeSize.width + padding.horizontal; + width = min(timePickerLandscapeSize.width + padding.horizontal, 560); // width is limited to 560 height = timePickerLandscapeSize.height + padding.vertical; expect( tester.getSize(find.byWidget(getMaterialFromDialog(tester))), @@ -749,10 +750,10 @@ void main() { expect(tester.getBottomLeft(find.text(okString)).dx, 616); expect(tester.getBottomRight(find.text(cancelString)).dx, 582); case MaterialType.material3: - expect(tester.getTopLeft(find.text(selectTimeString)), equals(const Offset(138, 129))); - expect(tester.getBottomRight(find.text(selectTimeString)), equals(const Offset(292.0, 143.0))); - expect(tester.getBottomLeft(find.text(okString)).dx, 616); - expect(tester.getBottomRight(find.text(cancelString)).dx, 578); + expect(tester.getTopLeft(find.text(selectTimeString)), equals(const Offset(144, 129))); + expect(tester.getBottomRight(find.text(selectTimeString)), equals(const Offset(298.0, 143.0))); + expect(tester.getBottomLeft(find.text(okString)).dx, 610); + expect(tester.getBottomRight(find.text(cancelString)).dx, 572); } await tester.tap(find.text(okString)); @@ -770,11 +771,11 @@ void main() { expect(tester.getBottomRight(find.text(okString)).dx, 184); expect(tester.getBottomLeft(find.text(cancelString)).dx, 218); case MaterialType.material3: - expect(tester.getTopLeft(find.text(selectTimeString)), equals(const Offset(508, 129))); - expect(tester.getBottomRight(find.text(selectTimeString)), equals(const Offset(662, 143))); - expect(tester.getBottomLeft(find.text(okString)).dx, 156); - expect(tester.getBottomRight(find.text(okString)).dx, 184); - expect(tester.getBottomLeft(find.text(cancelString)).dx, 222); + expect(tester.getTopLeft(find.text(selectTimeString)), equals(const Offset(502, 129))); + expect(tester.getBottomRight(find.text(selectTimeString)), equals(const Offset(656, 143))); + expect(tester.getBottomLeft(find.text(okString)).dx, 162); + expect(tester.getBottomRight(find.text(okString)).dx, 190); + expect(tester.getBottomLeft(find.text(cancelString)).dx, 228); } await tester.tap(find.text(okString));