CupertinoDatePicker & CupertinoTimerPicker dark mode #39919

LongCatIsLooong · 2019-09-05T18:43:01Z

Description

Changing the default background color of CupertinoDatePicker and CupertinoTimerPicker to null, which disables background painting entirely (i.e. the picker is going to have a completely transparent background), to match the native UIPicker and UIDatePicker.
Add dark mode support.
Disable the gradient painting when the background is null, because it looks really bad when the background is dark and backgroundColor = null

Golden Changes

https://github.com/flutter/goldens/pull/43/files?short_path=0bfad5e#diff-0bfad5e92f7d3a6380af2f64b62d3cf2

Related Issues

#35541
Mitigates #39685
TODO: #16831

Screenshots over black background, with `backgroundColor = null`.

The font color is set to blue so it's visible in both screenshots.

old	new

Tests

I added the following tests:

picker dark mode

Fixed goldens.

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 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

Does your PR require Flutter developers to manually update their apps to accommodate your change?

Yes, this is a breaking change (Please read Handling breaking changes). Replace this with a link to the e-mail where you asked for input on this proposed change.
No, this is not a breaking change.

justinmc

LGTM 👍

justinmc · 2019-09-05T19:36:31Z

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

-      (widget.backgroundColor.alpha * _kForegroundScreenOpacityFraction).toInt()
+  Widget _buildMagnifierScreen(Color resolved) {
+    final Color foreground = resolved?.withAlpha(
+      (resolved.alpha * _kForegroundScreenOpacityFraction).toInt()


Comma at the end here if you have time, I know it was already like that :) Also the identical line below.

justinmc · 2019-09-05T20:04:54Z

As we roll out these color changes, is the plan to change default background colors everywhere to transparent if they're white?

HansMuller

LGTM

HansMuller · 2019-09-05T20:00:35Z

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

  /// Background color of date picker.
  ///
-  /// Defaults to [CupertinoColors.white] when null.
+  /// Defaults to null, which disables background painting entirely.


Please include this detail in the issue's description, explain that we're making the change to improve fidelity wrt native iOS, and include before/after screenshots to help developers understand what's changed.

updated the description

HansMuller · 2019-09-05T20:05:48Z

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

  @override
  Widget build(BuildContext context) {
-    Widget result = DefaultTextStyle(
+    final Color resolvedBackgroundColor = widget.backgroundColor == null


If this idiom crops up enough, maybe we should allow CupertinoDyanmicColor.resolve(null, context)?

Anything to change now that that PR has been merged?

LongCatIsLooong · 2019-09-05T20:54:36Z

As we roll out these color changes, is the plan to change default background colors everywhere to transparent if they're white?

The main reason the picker is made transparent in this PR is that the native iOS UIPickerView has a transparent background color. I don't think it applies to widgets like CupertinoDialog despite having a white background.

LongCatIsLooong · 2019-09-09T22:21:30Z

@HansMuller @justinmc updated ListWheelScrollView, removed the gradients and replaced it with offCenterOpacity. It's a big change, could you re-review this PR?

LongCatIsLooong · 2019-09-09T22:38:07Z

It includes changes from #33070, in order for the opacity layer to work properly.

HansMuller · 2019-09-11T02:33:47Z

The new changes look good to me; just had a few small comments.

justinmc

LGTM

The only important thing to check is a potentially incorrect > commented above.

justinmc · 2019-09-11T17:26:42Z

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

-              color: foreground,
-            ),
-          ),
+          Expanded(child: Container()),


Do you still need the container now that there is no color? Or, could the Expanded be replaced with a Spacer? Also line 265 below.

I'm not sure, just throwing some ideas out there :)

Yeah makes sense. I'm going to just center the middle Container

justinmc · 2019-09-11T17:30:00Z

packages/flutter/lib/src/rendering/list_wheel_viewport.dart

+  double _offCenterOpacity = 1.0;
+  set offCenterOpacity(double value) {
+    assert(value != null);
+    assert(value > 0);


Should this be value >= 0? Looking at your comment.

ah forgot to change it. nice catch!

justinmc · 2019-09-11T17:31:35Z

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

  @override
  Widget build(BuildContext context) {
-    Widget result = DefaultTextStyle(
+    final Color resolvedBackgroundColor = widget.backgroundColor == null


Anything to change now that that PR has been merged?

Piinks · 2019-09-11T21:18:01Z

bin/internal/goldens.version

@@ -1 +1 @@
-49f8198b72f6e12c65fe1db2e46162de0204e671
+38e117bcd79860a3f239872c579acac716e70d49


This commit does not appear to exist in https://github.com/flutter/goldens/commits/master which is causing these golden file tests to fail.

Ah sorry I haven't merged the golden PR yet. I changed the golden hash to prevent flutter test from resetting the head to master so I can test with my golden fork. I'll update it once the golden PR is merged.

HansMuller · 2019-09-11T01:28:06Z

packages/flutter/lib/src/rendering/list_wheel_viewport.dart

-      _paintChildCylindrically(context, offset, child, transform, offsetToCenter);
-    else
+    final bool shouldApplyOffCenterDim = offCenterOpacity < 1;
+    if (useMagnifier || shouldApplyOffCenterDim)


This would be a good place for extra curly braces

HansMuller · 2019-09-11T01:32:06Z

packages/flutter/lib/src/rendering/list_wheel_viewport.dart

          magnifierTopLinePosition);

      // Clipping the part in the center.
      context.pushClipRect(


Formatting suggestion:

context.pushClipRect( needsCompositing, offset, centerRect, (PaintingContext context, Offset offset) { context.pushTransform( needsCompositing, offset, _magnifyTransform(), (PaintingContext context, Offset offset) { context.paintChild(child, offset + untransformedPaintingCoordinates); }, ); }, );

HansMuller · 2019-09-11T01:35:58Z

packages/flutter/lib/src/rendering/list_wheel_viewport.dart

-      },
+      offCenterOpacity == 1
+        ? innerPainter
+        : (PaintingContext context, Offset offset) => context.pushOpacity(offset, (offCenterOpacity * 255).round(), innerPainter),


Factoring innerPaint out into a local function (with a helpful comment!) seems like a good idea. Maybe do the same for the other clause. Also: prefer => for one-liners that return a value.

HansMuller · 2019-09-11T02:32:09Z

packages/flutter/lib/src/rendering/list_wheel_viewport.dart

+  ///
+  /// Must be greater than or equal to 0, and less than or equal to 1.
+  /// {@endtemplate}
+  double get offCenterOpacity => _offCenterOpacity;


I wish we had a better name for this property - "off center" sounds a bit like askew.

Maybe overAndUnderCenterOpacity?

… dark-mode-d

Piinks

Goldens look good to me! Thanks! The bot may notify you about triage on Flutter Gold when this lands. Let me know if you have any questions. :)

This reverts commit 97b27fa.

#41521)

LongCatIsLooong · 2019-09-29T22:25:44Z

9c6dd0d seems to cause the golden test to fail

…#39919)" (flutter#41521)

…#39919)" (flutter#41521)" This reverts commit 5f9c262.

dannyvalentesonos · 2020-04-04T04:55:47Z

@LongCatIsLooong
In your description, you wrote:

Disable the gradient painting when the background is null, because it looks really bad when the background is dark and backgroundColor = null

which to me means that gradient painting is only disabled when background is null, however, it seems like you removed the gradient entirely, and it's now impossible to get the same gradient effect as before. Am I right?

If so, this causes a problem for me and how I was using this widget. Is there a reason for that?

Thanks!

LongCatIsLooong · 2020-04-04T05:46:06Z

@dannyvalentesonos yes the gradient was removed entirely. The gradient has hardcoded colors that didn't work well with a dark background so it was removed.

LongCatIsLooong added 4 commits September 3, 2019 18:12

WIP

46a82c5

change default color to transparent

b97ad58

minor changes

9def5d6

fix tests

649aa3d

LongCatIsLooong requested review from HansMuller, darrenaustin and justinmc September 5, 2019 18:43

fluttergithubbot added f: cupertino flutter/packages/flutter/cupertino repository framework flutter/packages/flutter repository. See also f: labels. labels Sep 5, 2019

googlebot added the cla: yes label Sep 5, 2019

add test

93dcac2

LongCatIsLooong force-pushed the dark-mode-d branch from b53e3cf to 93dcac2 Compare September 5, 2019 18:46

LongCatIsLooong mentioned this pull request Sep 5, 2019

Goldens for https://github.com/flutter/flutter/pull/39919 flutter/goldens#43

Merged

LongCatIsLooong added c: API break Backwards-incompatible API changes will affect goldens Changes to golden files labels Sep 5, 2019

justinmc approved these changes Sep 5, 2019

View reviewed changes

HansMuller approved these changes Sep 5, 2019

View reviewed changes

review

5dd7465

LongCatIsLooong added 2 commits September 9, 2019 14:41

Opacity

e7e2a1d

update tests

9e62b97

update

2e8d74f

justinmc approved these changes Sep 11, 2019

View reviewed changes

LongCatIsLooong added 3 commits September 11, 2019 10:45

Merge branch 'master' into dark-mode-d

a693e90

comments

6c0567d

update

eb33a99

LongCatIsLooong requested a review from Piinks as a code owner September 11, 2019 18:19

Piinks reviewed Sep 11, 2019

View reviewed changes

LongCatIsLooong added a commit to flutter/goldens that referenced this pull request Sep 25, 2019

Goldens for flutter/flutter#39919 (#43)

d19eee4

HansMuller reviewed Sep 26, 2019

View reviewed changes

LongCatIsLooong added 4 commits September 26, 2019 13:32

Merge remote-tracking branch 'upstream/master' into dark-mode-d

ceed507

update golden hash

a3990c9

Merge branch 'dark-mode-d' of github.com:LongCatIsLooong/flutter into…

4189c88

… dark-mode-d

review

6b93e11

Piinks approved these changes Sep 27, 2019

View reviewed changes

LongCatIsLooong merged commit 97b27fa into flutter:master Sep 27, 2019

LongCatIsLooong deleted the dark-mode-d branch September 27, 2019 23:04

LongCatIsLooong added a commit that referenced this pull request Sep 27, 2019

Revert "CupertinoDatePicker & CupertinoTimerPicker dark mode (#39919)"

7d4dafa

This reverts commit 97b27fa.

LongCatIsLooong mentioned this pull request Sep 27, 2019

Revert "CupertinoDatePicker & CupertinoTimerPicker dark mode" #41521

Merged

LongCatIsLooong added a commit that referenced this pull request Sep 28, 2019

Revert "CupertinoDatePicker & CupertinoTimerPicker dark mode (#39919)" (

5f9c262

#41521)

Inconnu08 pushed a commit to Inconnu08/flutter that referenced this pull request Sep 30, 2019

CupertinoDatePicker & CupertinoTimerPicker dark mode (flutter#39919)

4814bec

Inconnu08 pushed a commit to Inconnu08/flutter that referenced this pull request Sep 30, 2019

Revert "CupertinoDatePicker & CupertinoTimerPicker dark mode (flutter…

8ac0a98

…#39919)" (flutter#41521)

LongCatIsLooong mentioned this pull request Sep 30, 2019

ListWheel broken since #9c6dd0de #41585

Closed

LongCatIsLooong added a commit to LongCatIsLooong/flutter that referenced this pull request Sep 30, 2019

Reland "CupertinoDatePicker & CupertinoTimerPicker dark mode (flutter…

5d553d5

…#39919)" (flutter#41521)" This reverts commit 5f9c262.

LongCatIsLooong mentioned this pull request Sep 30, 2019

Visual glitch introduced by RenderListWheelViewport needsCompositing #41654

Closed

LongCatIsLooong mentioned this pull request Oct 14, 2019

Add BoxDecoration styling to CupertinoPicker #42633

Closed

9 tasks

HansMuller mentioned this pull request Nov 25, 2019

Upcoming Cupertino Library Features #43041

Closed

18 tasks

LongCatIsLooong mentioned this pull request Dec 26, 2019

Reland 39919 CupertinoPicker #47837

Merged

13 tasks

yizhepku mentioned this pull request Feb 6, 2020

Obsolete documentation for backgroundColor of CupertinoPicker #50262

Closed

github-actions bot locked as resolved and limited conversation to collaborators Jul 31, 2021

		@@ -1 +1 @@
		49f8198b72f6e12c65fe1db2e46162de0204e671
		38e117bcd79860a3f239872c579acac716e70d49

CupertinoDatePicker & CupertinoTimerPicker dark mode #39919

CupertinoDatePicker & CupertinoTimerPicker dark mode #39919

Uh oh!

Conversation

LongCatIsLooong commented Sep 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Golden Changes

Related Issues

Screenshots over black background, with backgroundColor = null.

Tests

Checklist

Breaking Change

Uh oh!

justinmc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justinmc commented Sep 5, 2019

Uh oh!

HansMuller left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LongCatIsLooong Sep 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LongCatIsLooong commented Sep 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LongCatIsLooong commented Sep 9, 2019

Uh oh!

LongCatIsLooong commented Sep 9, 2019

Uh oh!

HansMuller commented Sep 11, 2019

Uh oh!

justinmc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Piinks left a comment

Choose a reason for hiding this comment

Uh oh!

LongCatIsLooong commented Sep 29, 2019

Uh oh!

dannyvalentesonos commented Apr 4, 2020

Uh oh!

LongCatIsLooong commented Apr 4, 2020

LongCatIsLooong commented Sep 5, 2019 •

edited

Loading

Screenshots over black background, with `backgroundColor = null`.

LongCatIsLooong Sep 5, 2019 •

edited

Loading

LongCatIsLooong commented Sep 5, 2019 •

edited

Loading