[CupertinoActionSheet] Match colors to native #149568

dkwingsmt · 2024-06-03T05:41:42Z

This PR matches the various colors of CupertinoActionSheet more closely with the native one.

The following colors are changed.

Sheet background color
Pressed button color
Cancel button color
Pressed cancel button color
Divider color
Content text color

The resulting colors match with native one with deviation of at most 1 (in terms of 0~255 RGB).

The following are comparison (left to right: Native, Flutter after PR, Flutter current)

Note: The divider thickness is adjusted to 1/dpr instead of 0.3 in both Flutter version to make them look more native, as will be proposed in #149636.

Derivation

All the colors are derived through color picker and calculation. The algorithm is as followed:

Assume all colors are translucent grey colors, i.e. having the same value x for R, G, and B, with an alpha a.
Given the barrier color is x_B1=0 when the background is black, and x_B2=204 when the background is white.
Pick the target color x_t1 when the background is black, and x_t2 when the background is white
Solve the following equations for x and a

a * x + (1-a) * x_B1 = x_t1
a * x + (1-a) * x_B2 = x_t2

a = 1 - (x_t1 - x_t2) / (x_B1 - x_B2)
x = (x_t1 - (1-a) * x_B1) / a

These equations use a linear model for color composition, which might not be exact, but is close enough for an accuracy of (1/255).

The full table is as follows:

The first two columns are colors picked from XCode.
The 3~4 columns are the colors picked from the current Flutter. Notice the deviation, which is sometimes drastic.
The 5~6 columns are the colors picked from Flutter after this PR. The deviation is at most 1.
The last few columns are calculation.
- There are two rows whose calculation is based on adjusted numbers, since the original results are not accurate enough, possibly due to the linear composition.

During the calculation, I assumed these colors vary between light and dark modes, but it turns out that both modes use the same set of colors.

Screenshots

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I signed the CLA.
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making, or this PR is test-exempt.
I followed the breaking change policy and added Data Driven Fixes where supported.
All existing and new tests are passing.

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

flutter-dashboard · 2024-06-06T22:44:59Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact "@test-exemption-reviewer" in the #hackers channel in Chat (don't just cc them here, they won't see it! Use Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

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

flutter-dashboard · 2024-06-06T23:05:12Z

Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change).

If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review.

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.

Changes reported for pull request #149568 at sha df73090

flutter-dashboard · 2024-06-07T00:08:15Z

Golden file changes are available for triage from new commit, Click here to view.

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.

Changes reported for pull request #149568 at sha a4d5026

chunhtai

LGTM

flutter-dashboard · 2024-06-07T18:01:40Z

Golden file changes are available for triage from new commit, Click here to view.

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.

Changes reported for pull request #149568 at sha 7a1e3f4

flutter-dashboard · 2024-06-07T19:40:42Z

Golden file changes are available for triage from new commit, Click here to view.

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.

Changes reported for pull request #149568 at sha 9bc6f08

auto-submit · 2024-06-07T22:10:11Z

auto label is removed for flutter/flutter/149568, due to - The status or check suite Google testing has failed. Please fix the issues identified (or deflake) before re-applying this label.

This reverts commit 32081aa.

vashworth · 2024-06-10T15:07:26Z

PR to manual revert: #149998

…9998) This reverts commit 32081aa. Reason for revert: Appears to be failing tests in tree ``` 01:48 +2991 ~18: /Volumes/Work/s/w/ir/x/w/flutter/packages/flutter/test/cupertino/action_sheet_test.dart: Taps on a button can be slided to other buttons â��â��â�¡ EXCEPTION CAUGHT BY FLUTTER TEST FRAMEWORK â��â��â��â��â��â��â��â��â��â��â��â��â��â��â��â��â��â��â��â��â��â��â��â��â��â��â��â��â��â��â��â��â��â��â��â��â��â��â��â��â��â��â��â��â��â��â��â��â��â��â��â��â�� The following SkiaException was thrown while running async test code: Skia Gold received an unapproved image in post-submit testing. Golden file images in flutter/flutter are triaged in pre-submit during code review for the given PR. Visit https://flutter-gold.skia.org/ to view and approve the image(s), or revert the associated change. For more information, visit the wiki: https://github.com/flutter/flutter/wiki/Writing-a-golden-file-test-for-package:flutter Debug information for Gold -------------------------------- stdout: Given image with hash 601f421e3bb643f437cc12395de96152 for test cupertino.cupertinoActionSheet.press-drag Expectation for test: ce8ef79c146857d162663b1161fd6d5c (positive) Expectation for test: 3bb57a8782f67836c6ad3ece7f00729a (positive) Expectation for test: 3f8c592774caf3c760fbbd318a7dc5af (positive) Expectation for test: 61863f38217aa3349c239eeb49b84930 (positive) Expectation for test: 98af16e9b7eb3fb810b44c74bb18ffb9 (positive) Untriaged or negative image: https://flutter-gold.skia.org//detail?grouping=name%3Dcupertino.cupertinoActionSheet.press-drag%26source_type%3Dflutter&digest=601f421e3bb643f437cc12395de96152 stderr: Test: cupertino.cupertinoActionSheet.press-drag FAIL result-state.json: No result file found. ```

flutter/flutter@fc19ecf...32081aa 2024-06-08 [email protected] [CupertinoActionSheet] Match colors to native (flutter/flutter#149568) 2024-06-08 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Roll Flutter Engine from 1cdbebee1901 to 45cf05f7a580 (10 revisions) (#149944)" (flutter/flutter#149960) 2024-06-08 [email protected] Roll Flutter Engine from 1cdbebee1901 to 45cf05f7a580 (10 revisions) (flutter/flutter#149944) 2024-06-07 [email protected] Refactor `getRootWidgetSummaryTree` tests in `widget_inspector_test.dart` (flutter/flutter#149930) 2024-06-07 [email protected] Remove zanderso from TESTOWNERS (flutter/flutter#149935) 2024-06-07 [email protected] PinnedHeaderSliver (flutter/flutter#143196) 2024-06-07 [email protected] Fix test case in "getRootWidgetSummaryTree" test (flutter/flutter#149923) 2024-06-07 [email protected] Revert "Identify and re-throw our dependency checking errors in `flutter.groovy` (#149609)" (flutter/flutter#149918) 2024-06-07 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Fix `SegmentedButton` clipping when drawing segments (#149739)" (flutter/flutter#149927) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

…" (flutter#149998) This reverts commit 8b6be37.

… (#150015)" This reverts commit 52df034.

…) (#150015)" (#150021) Reverts: #150015 Initiated by: vashworth Reason for reverting: Still failing tree on goldens Original PR Author: dkwingsmt Reviewed By: {chunhtai} This change reverts the following previous change: Relands #149568, which was reverted in #149998 due to unverified golden tests post-commit. (Honestly I don't know what happened but I guess resubmitting should resolve it.) No code is changed.

b3nni97 · 2024-06-11T17:04:35Z

@dkwingsmt I have problems with the colors in dark mode, I suspect this PR has broken something.

I see some colors are not resolved by CupertinoDynamicColor.resolve, maybe thats the issue.

dkwingsmt · 2024-06-11T17:32:22Z

@dkwingsmt I have problems with the colors in dark mode, I suspect this PR has broken something.

I see some colors are not resolved by CupertinoDynamicColor.resolve, maybe thats the issue.

Can you provide me with screenshots and code to reproduce? I might have used the wrong method to test.

Edit: Actually this PR has been reverted and is not present in main. (Due to infra issues I can't merge it for a while.) So if you observed wrong colors in current main, it's the exact issue I'm trying to resolve. Can you test if Dark mode looks correct with this PR?

b3nni97 · 2024-06-11T18:17:13Z

Hi @dkwingsmt,

I´m using this code to reproduce:

void main() {
 runApp(TestScaffoldApp(
   theme: const CupertinoThemeData(brightness: Brightness.dark),
   actionSheet: CupertinoActionSheet(
     actions: [
       CupertinoActionSheetAction(
         onPressed: () {},
         child: Text('Button'),
       ),
       CupertinoActionSheetAction(
         onPressed: () {},
         child: Text('Button'),
       ),
     ],
     cancelButton: CupertinoActionSheetAction(
       isDefaultAction: true,
       child: Text("Cancel"),
       onPressed: () {},
     ),
   ),
 ));
}

// Shows an app that has a button with text "Go", and clicking this button
// displays the `actionSheet` and hides the button.
//
// The `theme` will be applied to the app and determines the background.
class TestScaffoldApp extends StatefulWidget {
 const TestScaffoldApp(
     {super.key, required this.theme, required this.actionSheet});
 final CupertinoThemeData theme;
 final Widget actionSheet;

 @override
 TestScaffoldAppState createState() => TestScaffoldAppState();
}

class TestScaffoldAppState extends State<TestScaffoldApp> {
 bool _pressedButton = false;

 @override
 Widget build(BuildContext context) {
   return CupertinoApp(
     theme: widget.theme,
     home: Builder(
       builder: (BuildContext context) => CupertinoPageScaffold(
         child: Center(
           child: _pressedButton
               ? Container()
               : CupertinoButton(
                   onPressed: () {
                     setState(() {
                       _pressedButton = true;
                     });
                     showCupertinoModalPopup<void>(
                       context: context,
                       builder: (BuildContext context) {
                         return widget.actionSheet;
                       },
                     );
                   },
                   child: const Text('Go'),
                 ),
         ),
       ),
     ),
   );
 }
}

This is where I use your PR using Dark Mode:

This is running the latest master (still not correct, so maybe some other PR has broken this behavior):

dkwingsmt · 2024-06-11T18:23:13Z

The first screenshot is exactly what I observed on an Xcode native app when the dark mode toggle on the status bar is on. Did I do it wrong? Can you show me what color it should be?

b3nni97 · 2024-06-11T18:42:04Z

Maybe this is a bug in Xcode itself, I can't reproduce it myself in Xcode right now, but this is what a native action sheet looks like in dark mode in the Contacts app.

If I use CupertinoDynamicColors.resolve for all colors in _ActionSheetButtonBackground (currently _kPressed, _kActionSheetCancelPressedColor and secondarySystemGroupedBackground Color are not resolved),
then I get these colors (matching native)

Sorry for the enormous screenshots.

dkwingsmt · 2024-06-11T18:44:54Z

Thanks you very much! Yeah I couldn't believe it either when I saw that the colors were the same in dark mode. I'll do more research (probably by running the native app on my phone).

flutter#149568) (flutter#150015)" (flutter#150021)" This reverts commit 5eaeaca.

Relands #149568, which was reverted in #149998 due to unverified golden tests post-commit from recent infra issues. **New changes** (all contained in 5ca5139 ): * Fixes a problem within the tests "Overall looks correctly under x theme" where the button press is not applied to the golden file. * Dark theme now uses colors different from the light theme. It was an issue reported in #149568 (comment). I made the mistake because the XCode preview doesn't correctly apply dark theme, which made me think the dark theme uses the same colors. As shown in the following table, the dark theme colors also achieved deviation of <=1. <img width="1091" alt="image" src="https://github.com/flutter/flutter/assets/1596656/f4acda2b-1857-449c-8c1b-1f48afeb9095"> Screenshot comparison: (left to right: native, Flutter after PR, Flutter before PR) <img width="1286" alt="image" src="https://github.com/flutter/flutter/assets/1596656/580eef1f-a7f9-45d9-a7c8-fab0ca9606e3">

Relands flutter#149568, which was reverted in flutter#149998 due to unverified golden tests post-commit from recent infra issues. **New changes** (all contained in flutter@5ca5139 ): * Fixes a problem within the tests "Overall looks correctly under x theme" where the button press is not applied to the golden file. * Dark theme now uses colors different from the light theme. It was an issue reported in flutter#149568 (comment). I made the mistake because the XCode preview doesn't correctly apply dark theme, which made me think the dark theme uses the same colors. As shown in the following table, the dark theme colors also achieved deviation of <=1. <img width="1091" alt="image" src="https://github.com/flutter/flutter/assets/1596656/f4acda2b-1857-449c-8c1b-1f48afeb9095"> Screenshot comparison: (left to right: native, Flutter after PR, Flutter before PR) <img width="1286" alt="image" src="https://github.com/flutter/flutter/assets/1596656/580eef1f-a7f9-45d9-a7c8-fab0ca9606e3">

Relands #149568 (first attempt) or #150129 (latest attempt), which was reverted in #150142 due to unverified golden tests post-commit from recent infra issues. No code is changed from #150129. The relevant golden files have been resubmitted in #150219. I expect these files to appear in the golden file checker in this PR (which will hint that this PR is safe.) Fingers crossed.

…0386)" (#150413) Reverts: #150386 Initiated by: andrewkolos Reason for reverting: the Mac framework_tests_impeller check has been failing (due to golden test failures) since this PR (though it occasionally passes after retry). Taking a brief look at this PR, it looks like there's been some troubles with goldens in the past (though the recent failures appear very different to past ones). Regardless, I'm taking a shot at getting the tree green again by reverting this. Apologies i Original PR Author: dkwingsmt Reviewed By: {chunhtai} This change reverts the following previous change: Relands #149568 (first attempt) or #150129 (latest attempt), which was reverted in #150142 due to unverified golden tests post-commit from recent infra issues. No code is changed from #150129. The relevant golden files have been resubmitted in #150219. I expect these files to appear in the golden file checker in this PR (which will hint that this PR is safe.) Fingers crossed.

Relands #149568 (first attempt) or #150386 (latest attempt), which was reverted in #150413 due to a one-pixel difference on the debug banner (in the tilted border of the rotated text). This attempt removed the debug banner from tests (see 31cbcb1). <img width="949" alt="image" src="https://github.com/flutter/flutter/assets/1596656/78f26f72-b69f-41ee-9134-2e2a9e8e1bdd">

Relands flutter#149568 (first attempt) or flutter#150386 (latest attempt), which was reverted in flutter#150413 due to a one-pixel difference on the debug banner (in the tilted border of the rotated text). This attempt removed the debug banner from tests (see flutter@31cbcb1). <img width="949" alt="image" src="https://github.com/flutter/flutter/assets/1596656/78f26f72-b69f-41ee-9134-2e2a9e8e1bdd">

…r#149568)

dkwingsmt added 4 commits May 31, 2024 16:45

Impl

8f935b6

Fix tests

e5ab067

Better doc

d420da8

Fix AS colors

1009339

github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: cupertino flutter/packages/flutter/cupertino repository labels Jun 3, 2024

dkwingsmt added 4 commits June 3, 2024 11:24

Fix test

62862f9

Merge remote-tracking branch 'origin/main' into as-fix-color

1d67efd

Change more colors

0a7f978

Merge remote-tracking branch 'origin/main' into as-fix-color

df73090

dkwingsmt requested a review from chunhtai June 6, 2024 22:34

dkwingsmt marked this pull request as ready for review June 6, 2024 22:44

dkwingsmt changed the title ~~[CupertinoActionSheet] Fix the color~~ [CupertinoActionSheet] Match colors to native Jun 6, 2024

flutter-dashboard bot added the will affect goldens Changes to golden files label Jun 6, 2024

dkwingsmt added 2 commits June 6, 2024 16:37

Add golden test

f591591

Fix divider color

a4d5026

chunhtai approved these changes Jun 7, 2024

View reviewed changes

Use CupertinoApp for golden

7a1e3f4

dkwingsmt added 2 commits June 7, 2024 11:46

Better testing app

ae07afb

Fix lint

9bc6f08

dkwingsmt added the autosubmit Merge PR when tree becomes green via auto submit App label Jun 7, 2024

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Jun 7, 2024

dkwingsmt added the autosubmit Merge PR when tree becomes green via auto submit App label Jun 8, 2024

auto-submit bot merged commit 32081aa into flutter:master Jun 8, 2024

vashworth added a commit to vashworth/flutter that referenced this pull request Jun 10, 2024

Revert "[CupertinoActionSheet] Match colors to native (flutter#149568)"

170453a

This reverts commit 32081aa.

dkwingsmt added a commit to dkwingsmt/flutter that referenced this pull request Jun 10, 2024

Reapply "[CupertinoActionSheet] Match colors to native (flutter#149568)…

7eb9657

…" (flutter#149998) This reverts commit 8b6be37.

dkwingsmt mentioned this pull request Jun 10, 2024

Reland: [CupertinoActionSheet] Match colors to native (#149568) #150015

Merged

9 tasks

auto-submit bot pushed a commit that referenced this pull request Jun 10, 2024

Revert "Reland: [CupertinoActionSheet] Match colors to native (#149568)…

b449fbe

… (#150015)" This reverts commit 52df034.

auto-submit bot mentioned this pull request Jun 10, 2024

Reverts "Reland: [CupertinoActionSheet] Match colors to native (#149568) (#150015)" #150021

Merged

9 tasks

dkwingsmt added a commit to dkwingsmt/flutter that referenced this pull request Jun 12, 2024

Revert "Reverts "Reland: [CupertinoActionSheet] Match colors to native (

f9659ee

flutter#149568) (flutter#150015)" (flutter#150021)" This reverts commit 5eaeaca.

dkwingsmt mentioned this pull request Jun 12, 2024

Reland 2: [CupertinoActionSheet] Match colors to native #150129

Merged

9 tasks

dkwingsmt mentioned this pull request Jun 17, 2024

Reland 3: [CupertinoActionSheet] Match colors to native #150386

Merged

9 tasks

auto-submit bot mentioned this pull request Jun 18, 2024

Reverts "Reland 3: [CupertinoActionSheet] Match colors to native (#150386)" #150413

Merged

9 tasks

dkwingsmt mentioned this pull request Jun 18, 2024

Reland 4: [CupertinoActionSheet] Match colors to native #150442

Merged

9 tasks

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 6, 2024

32081aa [CupertinoActionSheet] Match colors to native (flutter/flutte…

572939f

…r#149568)

[CupertinoActionSheet] Match colors to native #149568

[CupertinoActionSheet] Match colors to native #149568

Uh oh!

Conversation

dkwingsmt commented Jun 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Derivation

Screenshots

Pre-launch Checklist

Uh oh!

flutter-dashboard bot commented Jun 6, 2024

Uh oh!

flutter-dashboard bot commented Jun 6, 2024

Uh oh!

flutter-dashboard bot commented Jun 7, 2024

Uh oh!

chunhtai left a comment

Choose a reason for hiding this comment

Uh oh!

flutter-dashboard bot commented Jun 7, 2024

Uh oh!

flutter-dashboard bot commented Jun 7, 2024

Uh oh!

auto-submit bot commented Jun 7, 2024

Uh oh!

vashworth commented Jun 10, 2024

Uh oh!

b3nni97 commented Jun 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dkwingsmt commented Jun 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

b3nni97 commented Jun 11, 2024

Uh oh!

dkwingsmt commented Jun 11, 2024

Uh oh!

b3nni97 commented Jun 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dkwingsmt commented Jun 11, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dkwingsmt commented Jun 3, 2024 •

edited

Loading

b3nni97 commented Jun 11, 2024 •

edited

Loading

dkwingsmt commented Jun 11, 2024 •

edited

Loading

b3nni97 commented Jun 11, 2024 •

edited

Loading