Update the cupertino picker visuals #65501

YeungKC · 2020-09-09T20:06:54Z

Description

The following changes were made to update to an iOS 14 style picker:
~~Update tertiarySystemFill (That's a mistake, It's been resolved.)~~
Update pickerTextStyle default letterSpacing
Adding magnifier parameters to the CupertinoPicker constructor method
Update the default magnifier default style
Updates magnifier styles for CupertinoDatePicker and CupertinoTimerPicker.
Tests Related to Modifying CupertinoPicker Styles

Compare with native visuals:

The CupertinoDatePicker and CupertinoTimerPicker tests still fail, and I still don't understand the similarity between the

matchesGoldenFile('date_picker_test.datetime.initial.png')

The operating principle of the test is modified in order to pass the test.

Related Issues

Fixes #63694

Tests

I added the following tests:

Replace this with a list of the tests that you added as part of this PR. A change in behavior with no test covering it
will likely get reverted accidentally sooner or later. PRs must include tests for all changed/updated/fixed behaviors. See Test Coverage.

Checklist

Before you create this PR, confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I signed the CLA.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I updated/added relevant documentation (doc comments with ///).
All existing and new tests are passing.
The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
I am willing to follow-up on review comments in a timely manner.

Breaking Change

Did any tests fail when you ran them? Please read Handling breaking changes.

No, no existing tests failed, so this is not a breaking change.
Yes, this is a breaking change. If not, delete the remainder of this section.
- I wrote a design doc: https://flutter.dev/go/template Replace this with a link to your design doc's short link
- I got input from the developer relations team, specifically from: Replace with the names of who gave advice
- I wrote a migration guide: https://flutter.dev/go/breaking-changes-template Replace this with a link to a pull request that adds the migration guide to https://flutter.dev/docs/release/breaking-changes

…Color and update tertiarySystemFill.

xster · 2020-09-10T00:53:33Z

packages/flutter/lib/src/cupertino/colors.dart

-    darkElevatedColor: Color.fromARGB(61, 118, 118, 128),
-    highContrastElevatedColor: Color.fromARGB(51, 118, 118, 128),
-    darkHighContrastElevatedColor: Color.fromARGB(81, 118, 118, 128),
+    color: Color.fromRGBO(118, 118, 128, 0.12),


Are these the same values as the ARGB variant?

At your reminder, I rechecked this part of the color and found that in fact the color I got from the capture view hierarchy converted and matched the original color, which was my error and is now resolved.

xster · 2020-09-10T00:55:34Z

packages/flutter/lib/src/cupertino/date_picker.dart

@@ -58,6 +58,22 @@ void _animateColumnControllerToItem(FixedExtentScrollController controller, int
  );


Can you attach a screenshot for the date picker to help review as well?

This is what the CupertinoDatePicker looks like in action, as iOS has removed this style, so it can't be compared to native components.

You can by picking wheels in xcode. But it's definitely not very obvious.

xster · 2020-09-10T00:58:33Z

packages/flutter/lib/src/cupertino/text_theme.dart

@@ -80,7 +80,7 @@ const TextStyle _kDefaultPickerTextStyle = TextStyle(
  fontFamily: '.SF Pro Display',
  fontSize: 21.0,
  fontWeight: FontWeight.w400,
-  letterSpacing: -0.41,
+  letterSpacing: -0.6,


Can you add a description on how you derived this value? This will help the next maintainer assess his confidence on whether that person is more rigorous or less than you are :)

e.g. extracted from Apple sketch specs or compared visually etc.

To do this, I've made two comparison images that overlap the native text style version and the flutter text new and old style version, respectively.

This is a comparison of native and old version:

This is a comparison of native and new version:

It doesn't look like much of a difference, but the new style version will be a little more accurate.

Sounds good, I meant could you modify the code comment above this const declaration? It'll leave some hints for the next maintainer.

Maybe I'll add a comment:

// LetterSpacing sourced from iOS 14 simulator screenshots for comparison. See also: // // * https://github.com/flutter/flutter/pull/65501#discussion_r486557093

What do you think?

For code comments, it's ok. (I wouldn't reference /// docs to ephemeral discussions like GitHub PRs).

It's updated now.

LongCatIsLooong

Looks good to me. Thank you for the PR!

Not sure why there's no untriaged digest showing up on https://flutter-gold.skia.org/. Maybe the website needs time to catch up, I'll check again tomorrow.

LongCatIsLooong · 2020-09-10T06:47:57Z

packages/flutter/lib/src/cupertino/date_picker.dart

+const BorderRadius _rightBorderRadius = BorderRadius.horizontal(right: Radius.circular(CupertinoPicker.defaultHighlighterRadius));
+
+
+Container _buildMagnifier(BuildContext context, [EdgeInsetsGeometry margin = EdgeInsets.zero, BorderRadiusGeometry borderRadius = BorderRadius.zero]) => Container(


Maybe we should expose this builder (or expose another constructor in CupertinoPicker that allows developers to specify which side of the magnifier should have rounded corners and non-zero margins), so people can build their own multi-column pickers with ease?

nit: the => syntax is usually reserved for one-liners: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#consider-using--for-short-functions-and-methods

I thought about doing this, but might it be easier to customize the style by providing the magnifier parameters directly?
If satisfied with the convenience of creating multi-column pickers, perhaps we could create a CupertinoPickerDefaulMagnifier StatelessWidget?
What do you think?

Yeah having a widget for that sounds good to me, with it we can make the margin/radius constants private.

The CupertinoPickerDefaultMagnifier has now been created.

LongCatIsLooong · 2020-09-10T07:07:17Z

packages/flutter/lib/src/cupertino/picker.dart

@@ -193,6 +190,15 @@ class CupertinoPicker extends StatefulWidget {
  /// A delegate that lazily instantiates children.
  final ListWheelChildDelegate childDelegate;

+  /// the [magnifier] widget overlaid on top of [ListWheelScrollView].


Maybe mention that the [magnifier] will be vertically centered and have the same size as the magnified centered item, and defaults to the grey rrect first introduced in iOS 14.

Good point, I'm not very good at English, thanks for the advice.

Do you think it's okay if I change it to this?

/// The [magnifier] widget overlaid on top of [ListWheelScrollView], /// it will be vertically centered and will be the same size as the magnified item, /// defaulting to the grey rrect first introduced in iOS 14.

LongCatIsLooong · 2020-09-10T07:41:58Z

packages/flutter/lib/src/cupertino/date_picker.dart

        offAxisFraction = -_kMaximumOffAxisFraction * textDirectionFactor;
-      else if (i >= 2 || columnWidths.length == 2)
+        magnifier = _buildMagnifier(context, _leftEdgeInsets, _leftBorderRadius);
+      } else


nit: else if should be on the same line?

That's my formatting omission, thank you.

xster · 2020-09-10T07:47:26Z

oh https://github.com/flutter/flutter/wiki/Writing-a-golden-file-test-for-package:flutter#first-time-contributors it looks like there's a special step for first time contributors. cc @Piinks

xster

This is a very clean fix. Thank you @YeungKC!

Just a few more comments. Generally, I'd rename magnifier to selectionOverlay everywhere (since we don't know if useMagnifier is true and what users are going to put in there).

Once address, let's wait for the golden image to come in, then we can merge.

xster · 2020-09-10T23:00:19Z

packages/flutter/lib/src/cupertino/text_theme.dart

@@ -80,7 +80,7 @@ const TextStyle _kDefaultPickerTextStyle = TextStyle(
  fontFamily: '.SF Pro Display',
  fontSize: 21.0,
  fontWeight: FontWeight.w400,
-  letterSpacing: -0.41,
+  letterSpacing: -0.6,


Sounds good, I meant could you modify the code comment above this const declaration? It'll leave some hints for the next maintainer.

xster · 2020-09-10T23:08:22Z

packages/flutter/lib/src/cupertino/picker.dart

+  /// The [magnifier] widget overlaid on top of [ListWheelScrollView],
+  /// it will be vertically centered and will be the same size as the magnified item,
+  /// default to use [CupertinoPickerDefaultMagnifier].


A widget overlaid on the picker to highlight the currently selected entry. The [selectionOverlay] widget drawn above the [CupertinoPicker]'s picker wheel. It is vertically centered in the picker and is constrained to have the same height as the center row. If unspecified, it defaults to a [CupertinoPickerDefaultMagnifier] which is a gray rounded rectangle overlay in the iOS 14 style.

Word wrap as needed. In general, documentation need a 1 line summary (which is what appears when you hover over a field in IDEs' popup hints).

I wouldn't mention ListWheelScrollView here since it's an implementation detail of this API and users probably won't know what it is.

xster · 2020-09-10T23:09:24Z

packages/flutter/lib/src/cupertino/picker.dart

+  /// The [magnifier] widget overlaid on top of [ListWheelScrollView],
+  /// it will be vertically centered and will be the same size as the magnified item,
+  /// default to use [CupertinoPickerDefaultMagnifier].
+  final Widget magnifier;


I'd rename this selectionOverlay. A 'magnifier' is what it does, not what it is. We don't know what it's going to do if the user is free to specify anything they want here.

Thank you, I've solved this now.

xster · 2020-09-10T23:10:35Z

packages/flutter/lib/src/cupertino/picker.dart

@@ -125,6 +121,7 @@ class CupertinoPicker extends StatefulWidget {
    @required this.onSelectedItemChanged,
    @required IndexedWidgetBuilder itemBuilder,
    int childCount,
+    this.magnifier,


The general pattern is to have a default value here (e.g. this.magnifier = const CupertinoPickerDefaultMagnifier()) rather than filling it in in the state. This way, you can actually specify null as an intended value (to not draw anything there if users want).

This was my mistake, thank you, and is now resolved.

It's not a mistake :) We didn't really document this pattern clearly.

But this is a best way.

xster · 2020-09-10T23:12:07Z

packages/flutter/lib/src/cupertino/picker.dart

@@ -313,6 +310,57 @@ class _CupertinoPickerState extends State<CupertinoPicker> {
  }
 }

+/// An picker default magnifier, grey rrect first introduced in iOS 14


A default selection overlay for [CupertinoPicker]s.

It draws a gray rounded rectangle to match the picker visuals introduced in iOS 14

Thank you, it's updated now.

xster · 2020-09-10T23:12:36Z

packages/flutter/lib/src/cupertino/picker.dart

+/// An picker default magnifier, grey rrect first introduced in iOS 14
+class CupertinoPickerDefaultMagnifier extends StatelessWidget {
+
+  /// Create a magnifier, it can be easily configured with the default style.


Don't editorialize or use subjective adjectives. Just state what it does.

The api doc has been updated and has a more detailed description of the arguments.

xster · 2020-09-10T23:14:18Z

packages/flutter/lib/src/cupertino/picker.dart

+  /// The color to fill in the background of the magnifier, Support for using [CupertinoDynamicColor].
+  final Color background;
+
+  /// default margin of the 'magnifier'.


Use complete sentences for all documentations (e.g. capitalize first letter).

That was my mistake and it's now resolved.

xster · 2020-09-10T23:15:02Z

packages/flutter/lib/src/cupertino/picker.dart

+  const CupertinoPickerDefaultMagnifier({
+    Key key,
+    this.background = CupertinoColors.tertiarySystemFill,
+    this.useLeftStyle = true,


Could be more self-descriptive. Maybe capLeftEdge?

It's a good name, I've been thinking about naming it for a long time.... Thank you! ~~ It's been updated now.

…htEdge

xster · 2020-09-12T07:49:58Z

packages/flutter/lib/src/cupertino/date_picker.dart

@@ -453,7 +457,7 @@ class CupertinoDatePicker extends StatefulWidget {
  }
 }

-typedef _ColumnBuilder = Widget Function(double offAxisFraction, TransitionBuilder itemPositioningBuilder);
+typedef _ColumnBuilder = Widget Function(double offAxisFraction, TransitionBuilder itemPositioningBuilder, Widget magnifier);


You missed these names here (magnifier -> selectionOverlay)

Yes, thanks for reminding me.

xster · 2020-09-12T07:50:18Z

packages/flutter/lib/src/cupertino/date_picker.dart

@@ -982,17 +990,23 @@ class _CupertinoDatePickerDateTimeState extends State<CupertinoDatePicker> {

    for (int i = 0; i < columnWidths.length; i++) {
      double offAxisFraction = 0.0;
-      if (i == 0)
+      Widget magnifier;


I'd do a comb for the rename everywhere

…ino-picker-visuals

YeungKC · 2020-09-28T20:24:28Z

Before I updated, I found out about TimerPicker golden tests missing the default fontSize value, so I got unexpected results.
This has now been updated, and I get the expected results.

btw, in the original test, the local test couldn't output the gold file, but here it does: https://flutter-gold.skia.org/detail?test=cupertino.timer_picker_test.datetime.initial&digest=ff6ad81d5189b693156e3501a20a5d7f

xster · 2020-09-29T04:57:37Z

I've approved the rest of the goldens that match expectations from the last commit. Though I'm still seeing 3 diffs that are overflowing in https://flutter-gold.skia.org/search?crs=github&fdiffmax=-1&fref=false&frgbamax=255&frgbamin=0&head=true&include=false&issue=65501&limit=50&master=false&match=name&metric=combined&neg=false&offset=0&pos=false&query=source_type%3Dflutter&sort=desc&unt=true

YeungKC · 2020-09-29T21:21:55Z

I know, but I'm not sure why it's not the same as my local test results and if I can re-run the test generation?

xster · 2020-10-02T05:46:06Z

Sorry that this process is taking longer with the tests. Let us poke at it a bit more for a day. If we can't figure it out, I'll ask you to remove the TimerPicker golden tests test from date_picker_test.dart and we'll go figure it out afterwards. The PR itself is great and the test issue here isn't caused by you as far as I can tell.

YeungKC · 2020-10-02T06:59:55Z

Thank you for your follow-up, I believe this is worth the wait.

…ino-picker-visuals

Piinks

@xster it looks like you triaged the images on https://flutter-gold.skia.org/triagelog, can you confirm that's all good? :)
I do not know why the Cirrus framework_tests-libraries-linux is showing a strange golden failure, I think there is another infra issue there, looking into it now.
The Linux web-tests look to be timing out due to #67454

xster · 2020-10-06T22:21:14Z

It's still off I think but I couldn't figure out the current state.

Looking at

no new commits appeared under the same PR. There's 45 here and 29 there. The main issues we were facing were the cupertino.timer_picker_test.datetime goldens. I marked the last known incorrect ones as negative. Though it seems like we never triggered new ones for me to be able to triage on newer commits since.

Piinks · 2020-10-06T23:06:56Z

Have you tried running it locally since the local execution was fixed?
Not every commit is tested (like when I push 3 commits, only the latest gets tested), so I think that is why the # of commits is not matching with your accounting.

xster · 2020-10-07T01:56:51Z

ya the local results are as expected.

@YeungKC could I ask you to just comment out the 'TimerPicker golden tests' in the date_picker_test.dart file? I locally validated that the layout and rendering is correct and various widths didn't overflow. I'll poke at the golden test after you merge your changes to turn it back on.

Piinks · 2020-10-07T15:46:34Z

The cirrus failure is due to #67463, fix is in #67468
Sorry for the infra issues. Our migration to luci has caused a lot of gold issues lately.

fluttergithubbot · 2020-10-07T17:37:05Z

This pull request is not suitable for automatic merging in its current state.

The status or check suite framework_tests-libraries-linux has failed. Please fix the issues identified (or deflake) before re-applying this label.

xster · 2020-10-07T17:47:55Z

@Piinks don't know if it's related to #67468. The tests are indicating a date_picker_test.datetime.initial failure though in the gold console, I already approved the new image

Maybe there's some cross-talk with Cirrus as you're pointing out.

@YeungKC could you comment out 'DatePicker golden tests' as well? Sorry for the hassle so far. I'll turn them back on after we unblock this PR.

Piinks · 2020-10-07T17:57:28Z

@Piinks don't know if it's related to #67468. The tests are indicating a date_picker_test.datetime.initial failure though in the gold console, I already approved the new image

It is related. The failure you are seeing here has nothing to do with the dashboard, that all looks to be ok.

This is a Cirrus failure. Cirrus is no longer talking to Gold, so it is failing here without a way to be resolved.

This image shows only luci builds in the parameters under CI. That looks to be as expected.

The cirrus build failure here can't be resolved until #67468 lands and can be patched in. An infra change added a Cirrus build without the config to talk to Gold, so it is stuck right now.

xster · 2020-10-07T18:09:29Z

@YeungKC sorry, I just understood what @Piinks was saying. There are 2 testing infrastructures in transition and both are running golden though only 1 is the "source of truth". Linux framework_tests have already been passing for a while. That's the one appearing in the gold console. framework_tests-libraries-linux is now the deprecated infrastructure and no longer matters. We don't have to comment out more tests :). I'll merge this PR shortly after the last test completes.

xster · 2020-10-07T20:12:02Z

Confirming both the rob test and the cirrus test are non-blockers. The rob test looped in files in the CL that aren't part of this PR due to merge issues. And the cirrus test is extraneous since the golden tests already ran on luci as mentioned above. Merging and then I'll turn the skipped golden back on. Thanks for your contribution and your patience on this one @YeungKC!

This reverts commit db25441.

YeungKC · 2020-10-08T00:46:09Z

This is my first pr, but I underestimated the scope of changes and code requirements. I really thank you all for your patience during this process, and I have grown from it.

@xster @LongCatIsLooong @Piinks

xster · 2020-10-09T18:40:07Z

Thanks for working through it too and being responsive. This is a high quality PR. Sorry for the infra issues. We're working through a migration so there's some wrinkles. Hopefully it won't happen again on your next PR :D

YeungKC added 5 commits September 10, 2020 01:09

Update the Cupertino picker visuals

ca539f4

Modify the picker_test test

40e7766

Add high-contrast colors to tertiarySystemFillColor.

a48fb9c

Add margin to the Magnifier for CupertinoTimerPicker

0c6606b

Fix tertiarySystemFill duplicate, remove duplicate tertiarySystemFill…

8da08c2

…Color and update tertiarySystemFill.

flutter-dashboard bot added f: cupertino flutter/packages/flutter/cupertino repository framework flutter/packages/flutter repository. See also f: labels. labels Sep 9, 2020

googlebot added the cla: yes label Sep 9, 2020

xster requested review from LongCatIsLooong and xster September 10, 2020 00:52

xster reviewed Sep 10, 2020

View reviewed changes

YeungKC added 2 commits September 10, 2020 14:35

Rollback tertiarySystemFill Color

f0b0288

Formatting tertiarySystemFill's code

068003b

LongCatIsLooong reviewed Sep 10, 2020

View reviewed changes

YeungKC added 3 commits September 10, 2020 16:18

fix else if line format.

cdfc306

Creating and use CupertinoPickerDefaultMagnifier

08d5eca

Update the api doc of [magnifier].

da630e9

xster approved these changes Sep 10, 2020

View reviewed changes

YeungKC added 6 commits September 12, 2020 14:24

magnifier change use default value

0b54059

The magnifier argument is change to selectionOverlay.

0aa1cdf

Update api doc on CupertinoPickerDefaultMagnifier

3259ff3

Updated api doc about of CupertinoPickerDefaultMagnifier arguments.

e59a5f4

Update api doc about of CupertinoPickerDefaultMagnifier constructor.

794064e

Update arguments useLeftStyle => capLeftEdge, useRightStyle => capRig…

4522ee4

…htEdge

xster reviewed Sep 12, 2020

View reviewed changes

xster approved these changes Sep 12, 2020

View reviewed changes

YeungKC added 2 commits September 12, 2020 16:28

magnifier rename to selectionOverlay

3b3926d

add _kDefaultPickerTextStyle.letterSpacing change code comment

681b0ec

Merge remote-tracking branch 'upstream/master' into update-the-cupert…

70fd426

…ino-picker-visuals

YeungKC mentioned this pull request Sep 30, 2020

Gold failure locally does not show images #66072

Closed

Merge remote-tracking branch 'upstream/master' into update-the-cupert…

ab5855b

…ino-picker-visuals

Piinks reviewed Oct 6, 2020

View reviewed changes

refactor: Comment out TimerPicker golden tests.

4a2ab13

xster added the waiting for tree to go green label Oct 7, 2020

xster mentioned this pull request Oct 7, 2020

iOS Catalog flutter/platform_tests#3

Merged

4 tasks

fluttergithubbot removed the waiting for tree to go green label Oct 7, 2020

xster merged commit db25441 into flutter:master Oct 7, 2020

xster mentioned this pull request Oct 7, 2020

Turn timer_picker_test goldens back on #67555

Merged

jonahwilliams added a commit that referenced this pull request Oct 7, 2020

Revert "Update the cupertino picker visuals (#65501)"

c233f3d

This reverts commit db25441.

jonahwilliams mentioned this pull request Oct 7, 2020

Revert "Update the cupertino picker visuals" #67565

Closed

pedromassangocode mentioned this pull request Jan 4, 2021

[CupertinoDatePicker] Selection background is broken for mode "time" and "dateAndTime" #73203

Closed

YeungKC mentioned this pull request Jan 4, 2021

Fix dateAndTime and time modes of CupertinoDatePicker. #73276

Merged

13 tasks

		@@ -58,6 +58,22 @@ void _animateColumnControllerToItem(FixedExtentScrollController controller, int
		);

		const BorderRadius _rightBorderRadius = BorderRadius.horizontal(right: Radius.circular(CupertinoPicker.defaultHighlighterRadius));


		Container _buildMagnifier(BuildContext context, [EdgeInsetsGeometry margin = EdgeInsets.zero, BorderRadiusGeometry borderRadius = BorderRadius.zero]) => Container(

Update the cupertino picker visuals #65501

Update the cupertino picker visuals #65501

Conversation

YeungKC commented Sep 9, 2020 • edited by xster

Description

Related Issues

Tests

Checklist

Breaking Change

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LongCatIsLooong left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xster commented Sep 10, 2020 • edited

xster left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YeungKC commented Sep 28, 2020 • edited

xster commented Sep 29, 2020

YeungKC commented Sep 29, 2020

xster commented Oct 2, 2020

YeungKC commented Oct 2, 2020

Piinks left a comment

Choose a reason for hiding this comment

xster commented Oct 6, 2020

Piinks commented Oct 6, 2020

xster commented Oct 7, 2020

Piinks commented Oct 7, 2020

fluttergithubbot commented Oct 7, 2020

xster commented Oct 7, 2020

Piinks commented Oct 7, 2020

xster commented Oct 7, 2020

xster commented Oct 7, 2020

YeungKC commented Oct 8, 2020

xster commented Oct 9, 2020

YeungKC commented Sep 9, 2020 •

edited by xster

xster commented Sep 10, 2020 •

edited

YeungKC commented Sep 28, 2020 •

edited