flutter
flutter copied to clipboard
Made Cupertino dialog more like a native dialog in dark mode
This PR addresses an issue where the CupertinoAlertDialog was not fully visible in dark mode. The dialog now adapts better to dark mode themes, ensuring proper contrast and readability across different UI elements.
Fixes: #80921
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.
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!).
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. The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group.
This PR will need images with side-by-side comparisons with native, new tests, and fixes to existing tests.
This PR will need images with side-by-side comparisons with native, new tests, and fixes to existing tests.
Sure @victorsanni! I will add images with a side-by-side comparison with native, and also fix the existing tests. However, I'm having a bit of trouble understanding the golden test documentation. Could you please help me with the following:
Do we need to create or update the golden test case? If yes, what are the steps involved in doing so?
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 #157218 at sha a1da43a44d4f2d63d8697084509d1ca0710b3797
Sure @victorsanni! I will add images with a side-by-side comparison with native, and also fix the existing tests. However, I'm having a bit of trouble understanding the golden test documentation. Could you please help me with the following:
Do we need to create or update the golden test case? If yes, what are the steps involved in doing so?
Considering its a color change, the goldens are expected to change.
Do we need to create or update the golden test case? If yes, what are the steps involved in doing so?
Nothing you need to do. One of us with access to the golden test tool will triage it and approve the changes from this PR when it's ready to merge. As such, screenshots will be helpful.
Can you explain how you got those numbers? And can you illustrate (probably with a side-by-side comparison, as mentioned by comments above) how you verify that those numbers produce native-like effects?
Thank you for you contribution!
Hi @victorsanni, @MitchellGoodwin, @dkwingsmt, here's the side-by-side comparison with native.
| Native | Flutter |
|---|---|
Also, @dkwingsmt, I derived those numbers from GitHub Issue #80921, where they were initially suggested for achieving native-like effects.
To verify the results, I created an alert dialog box in SwiftUI and manually compared the colors to ensure accuracy.
Hi @victorsanni, @MitchellGoodwin, @dkwingsmt, here's the side-by-side comparison with native.
Are the Flutter images before or after this PR? It would be easy to see the changes if the images show native, flutter before, and flutter after.
It looks like the separator between the actions and the main content needs to be lightened as well.
And the middle divider is also a little too light. Just from visually looking at it I wonder if the dividers are translucent. @thejitenpatel when trying to fix this, it might be helpful to draw the native dialog in different color backgrounds.
Hi @victorsanni, @MitchellGoodwin as requested, here's the detailed comparison of before and after PR changes with native.
Before PR
| Native | Flutter (master) |
|---|---|
After PR
| Native | Flutter (PR #157218) |
|---|---|
@thejitenpatel to add to @dkwingsmt's comment, there's no pressure to add the separator fix. But it would be nice to have that here as well, in order to close the linked issue entirely :)
Thank you Victor for the addition, although I must note that the separator issue is actually really complicated, and I would recommend doing so in a different PR.
(I don't know why the current screenshots show almost no horizontal separators, maybe it's a bug)
I can't find the issues on this problem for now, but there has been back and forth on how the thickness of the separator is defined, and my latest discovery is that iOS devices draw them with 1 physical pixel thick. Yes, this means that the separator on a low-res iPhone is thicker than a high-res one. But how should Flutter handle it? There used to be complaint on the separators looking differently on Android.
In short, fixing the separator might require further investigation and comparison. (Feel free to research on it though!)
I saw there was another open issue for the separators: https://github.com/flutter/flutter/issues/61164
Maybe this issue can be closed with this PR and target the separator fix to that issue instead?
I saw there was another open issue for the separators: #61164
Maybe this issue can be closed with this PR and target the separator fix to that issue instead?
Sounds good!
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 #157218 at sha a942ab03399d9fd3e1abc0d13094c94b71806494
the separator issue is actually really complicated, and I would recommend doing so in a different PR.
there has been back and forth on how the thickness of the separator is defined, and my latest discovery is that iOS devices draw them with 1 physical pixel thick.
The thickness of the horizontal separator might be complicated, but what about just the color? It looks darker than native. Or maybe that is part of the thickness?
Hi @MitchellGoodwin,
Thanks for the suggestion! I tried using CupertinoColors.separator.resolveFrom(context) &
final Color dividerColor = CupertinoDynamicColor.resolve(CupertinoColors.separator, context); as recommended, but it doesn't seem to be working as expected. The divider color still isn’t adapting to the theme correctly. Do you have any additional suggestions or insights on what might be going wrong? Any help would be appreciated!
Hi @MitchellGoodwin,
Thanks for the suggestion! I tried using
CupertinoColors.separator.resolveFrom(context)&final Color dividerColor = CupertinoDynamicColor.resolve(CupertinoColors.separator, context);as recommended, but it doesn't seem to be working as expected. The divider color still isn’t adapting to the theme correctly. Do you have any additional suggestions or insights on what might be going wrong? Any help would be appreciated!
Have you checked whether the value of dividerColor has changed as intended?
The divider color still isn’t adapting to the theme correctly.
Is it showing as the same color as before? And is it not picking up the brightness, the elevation, or both?
CupertinoDynamicColor.resolve may need to be called further down. Possibly in the _Divider class itself.
I haven't tested to be sure, but I believe that this PR didn't introduce that behavior, so it's not required to fix it in this PR. But my instinct is that the divider color will be an easy fix somewhere, it just may need some trial and error to find where it is. Give it a try and see what happens. If it doesn't work after a bit of trying, I'll approve the PR as-is and make a separate issue for that divider.
Hi @MitchellGoodwin, Thanks for the suggestion! I tried using
CupertinoColors.separator.resolveFrom(context)&final Color dividerColor = CupertinoDynamicColor.resolve(CupertinoColors.separator, context);as recommended, but it doesn't seem to be working as expected. The divider color still isn’t adapting to the theme correctly. Do you have any additional suggestions or insights on what might be going wrong? Any help would be appreciated!Have you checked whether the value of
dividerColorhas changed as intended?
Hi @dkwingsmt,
I've checked that the dividerColor is changing as expected, but it seems the actual Divider itself isn't reflecting this change.
Is it showing as the same color as before? And is it not picking up the brightness, the elevation, or both?
CupertinoDynamicColor.resolvemay need to be called further down. Possibly in the_Dividerclass itself.
@MitchellGoodwin, It’s showing as the same color as before—no noticeable change. It doesn’t seem to be picking up the brightness correctly, and the elevation appears to be slightly lower than expected.
Attaching screenshot of this PR for your reference:
Hi @MitchellGoodwin,
You’re correct that this PR didn’t introduce the behavior. The issue occurs specifically when a horizontal divider is drawn—it doesn’t resolve the color as expected (screenshot 1.0). It is taking elevatedColor property if I'm not wrong, whereas the vertical divider resolves it correctly via the _CupertinoAlertActionSection class (screenshot 1.1).
To address this, I added CupertinoDynamicColor.resolve in the _CupertinoAlertDialogState class, which helps resolve the color issue for the horizontal divider. However, it still doesn’t fully address the brightness, and the elevation appears slightly less than expected.
To further address the color issue, as suggested, I also applied CupertinoDynamicColor.resolve directly in the Divider class (screenshot 1.2). The Color got resolved but it's not taking brightness if I am not wrong and elevation seems little less as expected (screenshot 2.0).
Interestingly, if there are more than two buttons, all horizontal dividers render with the correct color, except for the first one (screenshot 3.0)
Screenshot 1.0:
Screentshot 1.1:
Screenshot 2.0: Dialog with single button in this PR
Screenshot 3.0: Current behavior after adding more than two buttons in this PR
Oh! You're right. I think I made a mistake at https://github.com/flutter/flutter/pull/150410/files#diff-4ea2907696de4fecf42f2ca56278a7b1d90c28921b1259d18ae28b1a2553d8e8R378
The dividerColor is never resolved here before being passed to _Divider, which doesn't resolve either. Thank you for your investigation!