Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Have Material widgets in a Cupertino App partially use Cupertino theme #139253

Merged
merged 9 commits into from
May 30, 2024

Conversation

MitchellGoodwin
Copy link
Contributor

@MitchellGoodwin MitchellGoodwin commented Nov 29, 2023

Fixes #138621

Fixes and issue where if a Material widget is used in a Cupertino App, then some parts of it's default Material 3 theming would show up as purplish. This could be fixed by wrapping your app with a Material theme, but that's a little awkward.

Material and Cupertino themes interact with each other a little oddly. If a theme of either is searched for but does not exist, then a default one is generated. So if a Material widget is in a Cupertino app that does not already have a Material theme, then a fallback Material theme will be created. This PR makes it so that, in this case when that Material theme is created, it's default colors will be based on the Cupertino theme.

Another oddity is that a Material theme always wraps itself with a Cupertino theme that's values are based on the Material theme. So before this change, a Material widget would theoretically add a new Material based Cupertino theme to the tree. So I added logic that would have the Material theme check to see if a Cupertino theme exists, before it overwrites it.

Before:
image

After:
Screenshot 2023-11-29 at 10 37 09 AM

Update:
I changed it to not rely on the Material 3 flag. Instead, if a Material theme is searched for and there is already a Cupertino theme in the tree, then it will use that Cupertino theme instead of generating a new. Also, if a Material theme is searched for, and it does not find one already in the tree, but does find a Cupertino theme, then it will generate one with a color palette based off of that Cupertino theme's colors.

After with this change:
Screenshot 2024-05-09 at 10 16 22 AM

We should still probably suggest for developers to include a Material theme in their Cupertino app if they wish to use widgets from both packages.

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.
  • All existing and new tests are passing.

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

@github-actions github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: cupertino flutter/packages/flutter/cupertino repository labels Nov 29, 2023
@github-actions github-actions bot added f: material design flutter/packages/flutter/material repository. d: api docs Issues with https://api.flutter.dev/ d: examples Sample code and demos labels Nov 29, 2023
@MitchellGoodwin
Copy link
Contributor Author

CC @QuncCccccc @HansMuller this is a fairly simple fix, but feels like a bandaid on a larger issue. Theoretically this should make it so that if Theme.of is used inside a MaterialApp it should use Material 3 for Material widgets, but otherwise it will not. Let me know if there's a case I missed.

But this does put a bandaid on a larger issue. I was surprised to find that a Material theme will wrap itself with a Cupertino theme. So Material widgets in a Cupertino app with no Material theme ancestor will create their own Cupertino theme, regardless of there already being one, which feels gross. I did try and wrap a basic Material theme around the Cupertino app widget to copy the solution from the Material side, but that created a dependency loop. Which was almost a relief because that was ugly too. I think it is important to fix this issue with something simple like this, but ideally the two themes would be aware of each other, if not unified in some way.

@QuncCccccc
Copy link
Contributor

I think this fix is smart! But not sure if this change will cause any confusion because we just made material 3 be default and then in this PR, we change the fallback to false?

@MitchellGoodwin
Copy link
Contributor Author

I think this fix is smart! But not sure if this change will cause any confusion because we just made material 3 be default and then in this PR, we change the fallback to false?

My intention was to have Cupertino apps default to having the material 3 flag be false. And as far as I can tell, that fallback is only used when there is no theme in the tree. So it should not be used when either a MaterialApp or Theme are provided, as MaterialApp always generates a theme. Is there a case I'm missing?

@QuncCccccc
Copy link
Contributor

Ah some unit tests might just build a widget without using MaterialApp or Theme, for example this one: https://github.com/flutter/flutter/pull/139145/files#diff-15ce4cd7a95566046eb6b57655b02d175a8297d14b033a5dc15fd5aa5aa866fbR209

But other than this, I also don't see any other use cases:)

@goderbauer
Copy link
Member

I spoke to @MitchellGoodwin and he said this is still on his radar.

@MitchellGoodwin
Copy link
Contributor Author

Converting this to a draft, as if the useMaterial3 flag gets removed in the future, then this will break. Keeping this around as a draft for the purpose of discussion.

@MitchellGoodwin MitchellGoodwin marked this pull request as draft February 27, 2024 22:29
@MitchellGoodwin MitchellGoodwin changed the title Wrap CupertinoApp with non-Material3 theme Have Material widgets in a Cupertino App partially use Cupertino theme Feb 29, 2024
@MitchellGoodwin
Copy link
Contributor Author

@HansMuller this it the draft for having the themes get along. I'll add documentation recommending that developers add a Material theme to their Cupertino apps.

@github-actions github-actions bot removed d: api docs Issues with https://api.flutter.dev/ d: examples Sample code and demos labels May 9, 2024
Copy link
Contributor

@QuncCccccc QuncCccccc left a comment

Choose a reason for hiding this comment

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

This solution makes sense to me! LGTM:) The new classes might need some more documentation.

@MitchellGoodwin MitchellGoodwin marked this pull request as ready for review May 29, 2024 19:17
Copy link
Contributor

@QuncCccccc QuncCccccc left a comment

Choose a reason for hiding this comment

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

LGTM:)

/// [ThemeData], so if further customization is needed, it is best to manually
/// add a Material [Theme] above the [CupertinoApp].
class CupertinoBasedMaterialThemeData {

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change

@MitchellGoodwin MitchellGoodwin added the autosubmit Merge PR when tree becomes green via auto submit App label May 30, 2024
@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label May 30, 2024
Copy link
Contributor

auto-submit bot commented May 30, 2024

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

@MitchellGoodwin MitchellGoodwin added the autosubmit Merge PR when tree becomes green via auto submit App label May 30, 2024
@auto-submit auto-submit bot merged commit 54573bc into flutter:master May 30, 2024
72 checks passed
victorsanni pushed a commit to victorsanni/flutter that referenced this pull request May 31, 2024
flutter#139253)

Fixes flutter#138621

Fixes and issue where if a Material widget is used in a Cupertino App, then some parts of it's default Material 3 theming would show up as purplish. This could be fixed by wrapping your app with a Material theme, but that's a little awkward.

Material and Cupertino themes interact with each other a little oddly. If a theme of either is searched for but does not exist, then a default one is generated. So if a Material widget is in a Cupertino app that does not already have a Material theme, then a fallback Material theme will be created. This PR makes it so that, in this case when that Material theme is created, it's default colors will be based on the Cupertino theme.

Another oddity is that a Material theme always wraps itself with a Cupertino theme that's values are based on the Material theme. So before this change, a Material widget would theoretically add a new Material based Cupertino theme to the tree. So I added logic that would have the Material theme check to see if a Cupertino theme exists, before it overwrites it.

Before:
![image](https://github.com/flutter/flutter/assets/58190796/95874076-e943-48fa-ba9d-1d7b0f5a2f38)

After:
<img width="386" alt="Screenshot 2023-11-29 at 10 37 09�AM" src="https://github.com/flutter/flutter/assets/58190796/959ccfd9-3439-438e-ad36-20597334837a">

Update:
I changed it to not rely on the Material 3 flag. Instead, if a Material theme is searched for and there is already a Cupertino theme in the tree, then it will use that Cupertino theme instead of generating a new. Also, if a Material theme is searched for, and it does not find one already in the tree, but does find a Cupertino theme, then it will generate one with a color palette based off of that Cupertino theme's colors.

After with this change:
<img width="390" alt="Screenshot 2024-05-09 at 10 16 22�AM" src="https://github.com/flutter/flutter/assets/58190796/79765d04-a7a3-4eb5-9477-11668ed138e5">

We should still probably suggest for developers to include a Material theme in their Cupertino app if they wish to use widgets from both packages.
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 2, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 2, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 3, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 3, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 3, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 3, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 3, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 3, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 3, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 3, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App f: cupertino flutter/packages/flutter/cupertino repository f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Flutter 3.16 CupertinoApp and Material Widgets
3 participants