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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Different design between Windows and Web #54

Closed
makanddream opened this issue Jun 27, 2022 · 10 comments
Closed

fix: Different design between Windows and Web #54

makanddream opened this issue Jun 27, 2022 · 10 comments
Assignees
Labels
bug Flutter This issue can be repeated with Flutter without using this package documentation Improvements or additions to documentation
Milestone

Comments

@makanddream
Copy link

Hi Mike,

First of all, thank you for the plugin which allows me to have a unifying design! 馃槉

Description

I am developing a flutter application on windows with your plugin but I have two different designs between the web and windows version of themesplayground-v5 (5.0.1 Build-01). I checked that I had the same theme settings as well as ColorScheme. I found (for the moment) this bug only on the "Card" type widget, the bug for me is on elevation property. Do you know where it can come from?

Windows vs Web (Screenshot)

Windows Web
windows-img-1 web-img-1
windows-img-2 web-img-2
@rydmike
Copy link
Owner

rydmike commented Jun 27, 2022

Hi @makanddream,

Thank you for posting this issue, and yes this is a known issue. It happens when you set ThemeData.useMaterial3 to true, or the equivalent flag in FlexColorScheme, that just passes the toggle along to ThemeData.

It started happening in Flutter 3.0.0 and later. The shown WEB build of the app is made with release before Flutter 3.0.0, so it does not have this issue. The Windows build shown, I assume you made and with Flutter 3.0.x, so it has the issue.

I'm not sure if Flutter SDK intends to fix it or not. They seem to claim it is correct behaviour, more about this further below.

When it comes to using Material widget directly, of type card or canvas, you can fix it by explicitly giving it a shadowColor and surfaceTintColor, for example:

        final ThemeData theme = Theme.of(context);
        Material(
          elevation: 4,
          shadowColor: theme.colorScheme.shadow,
          surfaceTintColor: theme.colorScheme.surfaceTint,
          type: MaterialType.card,
          child: const SizedBox(
            height: 50,
            child: Center(child: Text('Material type card, elevation 4')),
          ),
        ),

Personally I think Material should when null default to using those colors for shadowColor and surfaceTintColor, since this would replicate past behaviour, with the M3 color tint twist for higher elevation. It is a bit odd that Material in useMaterial3 mode requires these extra properties to get nice and expected behaviour.

The underlying reason why dialogs and eg popup menu overlays do not get any elevation or overlay color is the same. This is tricker to fix, since you do not have access the needed properties in Material that those widgets use underneath. So you are left with two options until Flutter SDK fixes the issues and implements better support for the elevation on all dialogs and popup menu.

Option 1
You can keep useMaterial3 false.
When you opt-in on sub-theme in FlexColorScheme, the style is very reminiscent of Material3 style anyway.

Option 2
You can keep using it, but wrap the dialog and popup widgets in a theme where it is false.

I mentioned this Dialog/PopupMenu and Material elevation issue in this theming talk found via this blog: https://rydmike.com/blog_m3_talk, I think it is in the 2nd video.

In the talk companion app, that you can try in a DartPad here:
https://dartpad.dev/?id=515a014525c91ec55c86b489c30bfad5

You can see the same issue when turning on Material3. This demo does not use FlexColorScheme, it is just plain ThemeData. In the demo code you can see I fixed the Material widget theme elevation by giving it the extra shadowColor and surfaceTintColor properties, lines 1781 ... 1846. But there is not much I can do for the dialogs and popup menu in it either.

You can see some issues in Flutter SDK discussing the challenge of these ongoing changes with Material3 here:
flutter/flutter#102296
flutter/flutter#105101 (shows the theme wrapping work around trick for poupup menu)

As you can see the issues are being closed with ref to that it is being worked on, or that it should be as it is. I'm actually in the above devs camp, this should have been handled better, so not to cause issues with Material, Dialog and Popup elevation, regardless if using the flag useMaterial3 while full support for Material 3 is in progress and incomplete.

The current situation is not working correctly due to uncompleted transition of both flutter/flutter#91605 and flutter/flutter#91772, so this is causing issues and confusion and likely will do so until both are done.

I'm have not tested latest master channel to see what changes are pending there for next Flutter release. It might be fixed not sure. I might well be that a satisfactory fix for this cannot be done before it is fixed in Flutter SDK and we have it in next stable release. Still I'm looking into what I can do as potential workarounds too, have not found one though.

When it comes to the Material elevation itself, that is just documentation and letting users know, that this is how Material works now when using Flutter 3.0.0 and useMaterial3 flag. I will fix the sample app so it uses the shadowColor and surfaceTintColor as well and add info about this to the docs as well.

I'm keeping this issue open until we can see a proper solution in the Flutter SDK and so other devs can learn about it via this issue as well.

@rydmike rydmike added the bug Flutter This issue can be repeated with Flutter without using this package label Jun 27, 2022
@rydmike rydmike self-assigned this Jun 27, 2022
@rydmike rydmike added the documentation Improvements or additions to documentation label Jun 27, 2022
@makanddream
Copy link
Author

Thank you for your elaborate answer.
Now understand better where the problem comes from and that it is beyond your control!
For my part I will choose the first option: useMaterial3 = false

@rydmike
Copy link
Owner

rydmike commented Jul 6, 2022

I opened an issue in Flutter about this issue, to document it there too and to be able to offer a link where it is explained there as well flutter/flutter#107190.

In the issue report you can see that the issues still remains on latest Flutter master channel as well, so it has not been fixed yet, well not in any fix that has landed in master at least.

Merrit added a commit to Merrit/adventure_list that referenced this issue Jul 7, 2022
Causes incorrect colors / shadows ATM:

rydmike/flex_color_scheme#54
@rydmike
Copy link
Owner

rydmike commented Jul 13, 2022

As an update, here is an action issue from Google team that will remedy this issue concerning all the Dialogs flutter/flutter#107423.

The issue will still remain for Popups, BottomSheet, Banner and maybe a few other that need to implement the same things planned for the Dialogs.

Regarding solution for PopupMenu, this is the issue to track: flutter/flutter#101017

@rydmike
Copy link
Owner

rydmike commented Sep 4, 2022

Update:
This issue has progressed and there is fix PR here: flutter/flutter#110624

It is not yet even merged into master.

Also after merged we have no indication yet if this will be made available as a hotfix to Flutter 3.3, or if we have to wait another 3 to 4 months for it until Flutter 3.6.

If it is the latter; I have an idea that will be able to solve a part of the elevation issues as well by adding the right elevation tint color manually to widget themes that should have it as a part of their M3 design. When Flutter SDK has fixed the issues, I can remove the themp fix from FCS and use Flutter defaults.

@rydmike
Copy link
Owner

rydmike commented Sep 14, 2022

@rydmike
Copy link
Owner

rydmike commented Jan 14, 2023

Next stable Flutter release will fix this issue, and support for it be included in the next FlexColorScheme feature release, currently the WIP release version number is 6.2.0 since it contains no breaking changes so far, even if the release will be quite major feature wise.

Target is to release it shortly (few days) after the Flutter Forward event on Jan 25th, 2023.

@rydmike rydmike added this to To do in FlexColorScheme via automation Jan 14, 2023
@rydmike rydmike added this to the 6.2.0 milestone Jan 14, 2023
@rydmike rydmike moved this from To do to In progress in FlexColorScheme Jan 14, 2023
@rydmike rydmike moved this from In progress to Done in FlexColorScheme Jan 14, 2023
@rydmike rydmike modified the milestones: 6.2.0, 7.0.0 Jan 30, 2023
@rydmike
Copy link
Owner

rydmike commented Jan 30, 2023

Still same milestone target, but the release will be called 7.0.0, since I am introducing a few minor default style breaking changes. There are no breaking API changes though. There is a beta of it available on pub if you are feeling experimental, it and together with Flutter 3.7 includes the fixes for this long outstanding issue. FCS 6.1.0 also contained a partial workaround for the issue, it is removed as no longer needed in FCS 7 and its prereleases.

https://pub.dev/packages/flex_color_scheme/versions/7.0.0-dev.2

@rydmike
Copy link
Owner

rydmike commented Mar 19, 2023

Final beta of version 7 is out. There will be no additional features or API changes in the stable release. More info in this tweet thread:

https://twitter.com/RydMike/status/1637228788139302914?s=20

Closing this issue as solved in version7 final beta together with Flutter 3.7 馃槃

@rydmike rydmike closed this as completed Mar 19, 2023
@rydmike rydmike moved this from Done to Released in FlexColorScheme Mar 19, 2023
@makanddream
Copy link
Author

I've been following all the hard work you've done on the branch: m3-master-channel. It's not much but thank you for your amazing work!

Good idea to have added a peripheral frame to visualize the theme 馃槈

Cordially,
Alan.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Flutter This issue can be repeated with Flutter without using this package documentation Improvements or additions to documentation
Projects
Status: Released
Development

No branches or pull requests

2 participants