Skip to content

Conversation

@pranavo72bex
Copy link
Contributor

@pranavo72bex pranavo72bex commented May 7, 2025

PR Description
This PR addresses an issue in the Flutter time picker where the separator between the hour and minute selectors (:) was not centered vertically. The change ensures that the separator is now vertically centered, improving the overall UI alignment and consistency.

Previously, the separator used only the TextAlign.center property, which did not fully address the vertical alignment issue. The updated code wraps the separator text in a Center widget to ensure proper centering both horizontally and vertically.

Before:

The separator (:) was horizontally centered but not aligned vertically.
After:

The separator is now both horizontally and vertically centered within the time picker.

image-20241023-041530 Screenshot 2024-10-23 at 12 33 05

pranavo72bex and others added 12 commits May 1, 2025 09:12
This updates the Flutter minimum macOS version from 10.14 to 10.15 adds
a migrator for existing apps, and updates our own examples, tests, and
benchmark apps to 10.15. A follow-up patch will drop macOS 10.15
`@available` checks in the embedder.

This is required in order to use Swift in the embedder and not need to
bundle the Swift runtime libs in every app that uses Flutter. Swift
stable ABI was introduced in macOS 10.14.4.

As of March 2025, usage of macOS 10.14 is approximately 1.2~1.8%
depending on source of statistics, see example public usage data here:
https://gs.statcounter.com/macos-version-market-share/desktop/worldwide

This patch makes the following changes:
1. Updates mac_deployment_target from 12.0 to 13.0.
2. Changes templates to `MACOSX_DEPLOYMENT_TARGET`, `MinimumOSVersion`,
and Podfile `platform :osx` to 10.15.
3. Adds migrator for Podfile part to migrate `platform :osx, '10.14'` ->
`platform :osx, '10.15'`
4. Compiles with `-mmacosx-version-min=10.15`
5. Runs the migrator on all example apps and integration tests.
6. Updates examples, tests to macOS 10.15 deployment target

Issue: flutter#167745

## 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.
- [X] 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] I followed the [breaking change policy] and added [Data Driven
Fixes] where supported.
- [X] All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel
on [Discord].

<!-- Links -->
[Contributor Guide]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview
[Tree Hygiene]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md
[test-exempt]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests
[Flutter Style Guide]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md
[Features we expect every widget to implement]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#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/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes
[Discord]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md
[Data Driven Fixes]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
@github-actions github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. labels May 7, 2025
@MitchellGoodwin MitchellGoodwin self-requested a review May 7, 2025 17:15
@flutter-dashboard
Copy link

This pull request executed golden file tests, but it has not been updated in a while (20+ days). Test results from Gold expire after as many days, so this pull request will need to be updated with a fresh commit in order to get results from Gold.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Copy link
Contributor

@MitchellGoodwin MitchellGoodwin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies it took so long to take a look, but I think I know everything that is going on. Most of the inconsistencies where solved by other PRs, but in the tests it looks like the positioning of the separators change depending on whether it's a dial or input style time picker. It looks like two easy changes can be made to this PR to get them to look better.

The separator is positioned a little lower relative to the other widgets for the dial picker. Both widgets have the separator in the middle of a Row, but the input one adds a 10 pixel bottom padding to the other members of the Row. If you add a 10 pixel bottom padding around the _TimeSelectorSeparator for the input that should make the two types consistent with each other at least.

But also the separator sits a little too low than expected in between the two. I think that's from the font adding too much space on top, making it off-centered. It's ok if it's a little low, but I think the effect is too strong. I think this can be improved by changing the height of the effectiveTextStyle used for the separator.

final TextStyle effectiveStyle = MaterialStateProperty.resolveAs<TextStyle>(
  timePickerTheme.timeSelectorSeparatorTextStyle?.resolve(states) ??
      timePickerTheme.hourMinuteTextStyle ??
      defaultTheme.timeSelectorSeparatorTextStyle?.resolve(states) ??
      defaultTheme.hourMinuteTextStyle,
  states,
).copyWith(color: effectiveTextColor, height: 1.0);

Sorry for the delay again. This took some detective work. I think with those two changes this should be pretty smooth to merge in from there.

@pranavo72bex
Copy link
Contributor Author

_TimeSelectorSeparator

Thank you for the detailed feedback!
I have already updated the separator’s text style to use .copyWith(color: effectiveTextColor, height: 1.0) as suggested.

For the _TimeSelectorSeparator padding:
Just to confirm, you’d like me to wrap the _TimeSelectorSeparator widget with a Padding of EdgeInsets.only(bottom: 10) only when the picker is in input mode (i.e., when entryMode is TimePickerEntryMode.input or inputOnly).
So the code would look like this inside the Row:

(_TimePickerModel.entryModeOf(context) == TimePickerEntryMode.input ||
 _TimePickerModel.entryModeOf(context) == TimePickerEntryMode.inputOnly)
    ? Padding(
        padding: const EdgeInsets.only(bottom: 10),
        child: _TimeSelectorSeparator(timeOfDayFormat: timeOfDayFormat),
      )
    : _TimeSelectorSeparator(timeOfDayFormat: timeOfDayFormat),

Please let me know if this is the correct approach, or if you’d like the padding applied differently.
Thanks again for your guidance!

@MitchellGoodwin
Copy link
Contributor

Just to confirm, you’d like me to wrap the _TimeSelectorSeparator widget with a Padding of EdgeInsets.only(bottom: 10) only when the picker is in input mode (i.e., when entryMode is TimePickerEntryMode.input or inputOnly)

At the code line I linked to, it should only ever be in one of the input modes, so that conditional check is unnecessary. You can just wrap it with the padding. The expanded widgets next to it also have the same padding.

@pranavo72bex
Copy link
Contributor Author

pranavo72bex commented Jun 24, 2025

Hi @MitchellGoodwin,

The test is still failing in CI/CD, but I’m unable to reproduce the failure locally. I ran a full flutter analyze and flutter pub outdated — there are some outdated and discontinued packages, but no analysis issues locally.

However, in the CI log, I see the following repeated error:

Command: dart --enable-asserts /b/s/w/ir/x/w/flutter/dev/bots/analyze.dart  
Command exited with exit code 1 but expected zero exit code.  

There’s no explicit error message, just an empty -- This line intentionally left blank --.

Could you please help me figure out what might be causing this CI-only analysis failure? Any insights or suggestions for debugging this would be really helpful.

Thank you so much for your help!

@MitchellGoodwin
Copy link
Contributor

It's complaining about the formatting in two of the files:

Found 2 Dart files which were formatted incorrectly.
To fix, run `dart format packages/flutter/test/material/time_picker_test.dart packages/flutter/lib/src/material/time_picker.dart` or:

git apply <<DONE
diff --git a/packages/flutter/test/material/time_picker_test.dart b/packages/flutter/test/material/time_picker_test.dart
index c300eb9dcc4..00000000000 100644
--- a/packages/flutter/test/material/time_picker_test.dart
+++ b/packages/flutter/test/material/time_picker_test.dart
@@ -1485,57 +1485,59 @@ void main() {
         semantics.dispose();
       });
 
-      testWidgets('TimePicker dialog displays centered separator between hour and minute selectors', (WidgetTester tester) async {
-        tester.view.physicalSize = const Size(400, 800);
-        tester.view.devicePixelRatio = 1.0;
-        addTearDown(tester.view.reset);
+      testWidgets(
+        'TimePicker dialog displays centered separator between hour and minute selectors',
+        (WidgetTester tester) async {
+          tester.view.physicalSize = const Size(400, 800);
+          tester.view.devicePixelRatio = 1.0;
+          addTearDown(tester.view.reset);
 
-        await tester.pumpWidget(
-          MaterialApp(
-            theme: ThemeData(useMaterial3: false),
-            home: const MediaQuery(
-              data: MediaQueryData(),
-              child: Material(
-                child: TimePickerDialog(
-                  initialTime: TimeOfDay(hour: 12, minute: 0),
+          await tester.pumpWidget(
+            MaterialApp(
+              theme: ThemeData(useMaterial3: false),
+              home: const MediaQuery(
+                data: MediaQueryData(),
+                child: Material(
+                  child: TimePickerDialog(initialTime: TimeOfDay(hour: 12, minute: 0)),
                 ),
               ),
             ),
-          ),
-        );
-        await tester.pumpAndSettle();
+          );
+          await tester.pumpAndSettle();
 
-        await expectLater(
-          find.byType(Dialog),
-          matchesGoldenFile('m2_time_picker.dialog.separator.alignment.png'),
-        );
-      });
+          await expectLater(
+            find.byType(Dialog),
+            matchesGoldenFile('m2_time_picker.dialog.separator.alignment.png'),
+          );
+        },
+      );
 
-      testWidgets('TimePicker dialog displays centered separator between hour and minute selectors', (WidgetTester tester) async {
-        tester.view.physicalSize = const Size(400, 800);
-        tester.view.devicePixelRatio = 1.0;
-        addTearDown(tester.view.reset);
+      testWidgets(
+        'TimePicker dialog displays centered separator between hour and minute selectors',
+        (WidgetTester tester) async {
+          tester.view.physicalSize = const Size(400, 800);
+          tester.view.devicePixelRatio = 1.0;
+          addTearDown(tester.view.reset);
 
-        await tester.pumpWidget(
-          MaterialApp(
-            theme: ThemeData(useMaterial3: true),
-            home: const MediaQuery(
-              data: MediaQueryData(),
-              child: Material(
-                child: TimePickerDialog(
-                  initialTime: TimeOfDay(hour: 12, minute: 0),
+          await tester.pumpWidget(
+            MaterialApp(
+              theme: ThemeData(useMaterial3: true),
+              home: const MediaQuery(
+                data: MediaQueryData(),
+                child: Material(
+                  child: TimePickerDialog(initialTime: TimeOfDay(hour: 12, minute: 0)),
                 ),
               ),
             ),
-          ),
-        );
-        await tester.pumpAndSettle();
+          );
+          await tester.pumpAndSettle();
 
-        await expectLater(
-          find.byType(Dialog),
-          matchesGoldenFile('m3_time_picker.dialog.separator.alignment.png'),
-        );
-      });
+          await expectLater(
+            find.byType(Dialog),
+            matchesGoldenFile('m3_time_picker.dialog.separator.alignment.png'),
+          );
+        },
+      );
 
       testWidgets('provides semantics information for text fields', (WidgetTester tester) async {
         final SemanticsTester semantics = SemanticsTester(tester);
@@ -2720,3 +2722,4 @@ Future<void> finishPicker(WidgetTester tester) async {
   await tester.tap(find.text(materialLocalizations.okButtonLabel));
   await tester.pumpAndSettle(const Duration(seconds: 1));
 }
diff --git a/packages/flutter/lib/src/material/time_picker.dart b/packages/flutter/lib/src/material/time_picker.dart
index df565c34f83..00000000000 100644
--- a/packages/flutter/lib/src/material/time_picker.dart
+++ b/packages/flutter/lib/src/material/time_picker.dart
@@ -523,7 +523,7 @@ class _TimeSelectorSeparator extends StatelessWidget {
             style: effectiveStyle,
             textScaler: TextScaler.noScaling,
           ),
-        )
+        ),
       ),
     );
   }
@@ -3837,3 +3837,4 @@ class _TimePickerDefaultsM3 extends _TimePickerDefaults {
 // dart format on
 
 // END GENERATED TOKEN PROPERTIES - TimePicker
DONE

@MitchellGoodwin
Copy link
Contributor

Other than the formatting complaints, code LGTM.

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 5, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 5, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 6, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 6, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 6, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 7, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 7, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 7, 2025
github-merge-queue bot pushed a commit that referenced this pull request Jul 11, 2025
Reference Link
#168441 (comment)

This PR removes explicit ThemeData(useMaterial3: true) assignments from
test files. Since useMaterial3 now defaults to true in Flutter, these
lines are no longer necessary and can be safely omitted.

**Why?**  

- Keeps test code clean and up-to-date with current Flutter defaults.
- Prevents confusion for contributors who may think this setting is
still required.
justinmc pushed a commit to justinmc/flutter that referenced this pull request Jul 11, 2025
…1569)

Reference Link
flutter#168441 (comment)

This PR removes explicit ThemeData(useMaterial3: true) assignments from
test files. Since useMaterial3 now defaults to true in Flutter, these
lines are no longer necessary and can be safely omitted.

**Why?**  

- Keeps test code clean and up-to-date with current Flutter defaults.
- Prevents confusion for contributors who may think this setting is
still required.
mboetger pushed a commit to mboetger/flutter that referenced this pull request Jul 21, 2025
…flutter#168441)

PR Description
This PR addresses an issue in the Flutter time picker where the
separator between the hour and minute selectors (:) was not centered
vertically. The change ensures that the separator is now vertically
centered, improving the overall UI alignment and consistency.

Previously, the separator used only the TextAlign.center property, which
did not fully address the vertical alignment issue. The updated code
wraps the separator text in a Center widget to ensure proper centering
both horizontally and vertically.

Before:

The separator (:) was horizontally centered but not aligned vertically.
After:

The separator is now both horizontally and vertically centered within
the time picker.


![image-20241023-041530](https://github.com/user-attachments/assets/8401264a-a751-47b4-b634-5742ba4fabcd)
![Screenshot 2024-10-23 at 12 33
05](https://github.com/user-attachments/assets/ab95abd9-8de8-4f7d-9114-d96073d8f887)

Issues Fixed
This PR fixes issue flutter#157402
(Vertical alignment issue in the time picker separator).

---------

Co-authored-by: Mitchell Goodwin <[email protected]>
Co-authored-by: Chris Bracken <[email protected]>
azatech pushed a commit to azatech/flutter that referenced this pull request Jul 28, 2025
…1569)

Reference Link
flutter#168441 (comment)

This PR removes explicit ThemeData(useMaterial3: true) assignments from
test files. Since useMaterial3 now defaults to true in Flutter, these
lines are no longer necessary and can be safely omitted.

**Why?**  

- Keeps test code clean and up-to-date with current Flutter defaults.
- Prevents confusion for contributors who may think this setting is
still required.
@Gustl22
Copy link
Contributor

Gustl22 commented Aug 3, 2025

Just as ref: the low position from the separator probably is due to the wrong font size:
material-components/material-components-android#4160

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 14, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 14, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 15, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 15, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 16, 2025
ksokolovskyi pushed a commit to ksokolovskyi/flutter that referenced this pull request Aug 19, 2025
…1569)

Reference Link
flutter#168441 (comment)

This PR removes explicit ThemeData(useMaterial3: true) assignments from
test files. Since useMaterial3 now defaults to true in Flutter, these
lines are no longer necessary and can be safely omitted.

**Why?**  

- Keeps test code clean and up-to-date with current Flutter defaults.
- Prevents confusion for contributors who may think this setting is
still required.
mboetger pushed a commit to mboetger/flutter that referenced this pull request Sep 18, 2025
…1569)

Reference Link
flutter#168441 (comment)

This PR removes explicit ThemeData(useMaterial3: true) assignments from
test files. Since useMaterial3 now defaults to true in Flutter, these
lines are no longer necessary and can be safely omitted.

**Why?**  

- Keeps test code clean and up-to-date with current Flutter defaults.
- Prevents confusion for contributors who may think this setting is
still required.
lucaantonelli pushed a commit to lucaantonelli/flutter that referenced this pull request Nov 21, 2025
…flutter#168441)

PR Description
This PR addresses an issue in the Flutter time picker where the
separator between the hour and minute selectors (:) was not centered
vertically. The change ensures that the separator is now vertically
centered, improving the overall UI alignment and consistency.

Previously, the separator used only the TextAlign.center property, which
did not fully address the vertical alignment issue. The updated code
wraps the separator text in a Center widget to ensure proper centering
both horizontally and vertically.

Before:

The separator (:) was horizontally centered but not aligned vertically.
After:

The separator is now both horizontally and vertically centered within
the time picker.


![image-20241023-041530](https://github.com/user-attachments/assets/8401264a-a751-47b4-b634-5742ba4fabcd)
![Screenshot 2024-10-23 at 12 33
05](https://github.com/user-attachments/assets/ab95abd9-8de8-4f7d-9114-d96073d8f887)

Issues Fixed
This PR fixes issue flutter#157402
(Vertical alignment issue in the time picker separator).

---------

Co-authored-by: Mitchell Goodwin <[email protected]>
Co-authored-by: Chris Bracken <[email protected]>
lucaantonelli pushed a commit to lucaantonelli/flutter that referenced this pull request Nov 21, 2025
…1569)

Reference Link
flutter#168441 (comment)

This PR removes explicit ThemeData(useMaterial3: true) assignments from
test files. Since useMaterial3 now defaults to true in Flutter, these
lines are no longer necessary and can be safely omitted.

**Why?**  

- Keeps test code clean and up-to-date with current Flutter defaults.
- Prevents confusion for contributors who may think this setting is
still required.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. will affect goldens Changes to golden files

Projects

Development

Successfully merging this pull request may close these issues.

5 participants