Fix text color not resolving when CupertinoThemeData.brightness is null

[ivirtex]

In the build method of the CupertinoApp, CupertinoTheme.of(context) gets called before MediaQuery exists in the tree.

By default, every value of the CupertinoTheme.data gets resolved according to the CupertinoThemeData.brightness, but if CupertinoThemeData.brightness is null, MediaQuery.maybeOf(context)?.platformBrightness is used.
Due to the lack of MediaQuery in the tree, CupertinoTheme.data gets its values resolved against Brightness.light.
This causes text color to be constantly equal to black - 0xff000000.

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

Lines 990 to 994 in 8b32ac7

	CupertinoDynamicColor resolveFrom(BuildContext context) {
	Brightness brightness = Brightness.light;
	if (_isPlatformBrightnessDependent) {
	brightness = CupertinoTheme.maybeBrightnessOf(context) ?? Brightness.light;
	}

Fixes #48438

Before / After

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.

[flutter-dashboard]

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 #115026 at sha 7987b1a9c923b2ad62c2506b45e27c3eeef12092

[ivirtex]

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

Seems like this is related to the #114450.
After merging the revert, goldens are passing.

[ivirtex]

Hey @MitchellGoodwin, I've seen that you self-requested a review of this PR almost 1.5 months ago, and I just wanted to remind you about that in case you lost my PR in your review queue.

[justinmc]

I posted a workaround and some details on the issue: #48438 (comment)

Ideally a solution would make Material and Cupertino behave identically here, where the brightness is set automatically on the theme data, and they would both do so in the same way.

[justinmc]

Isn't this sort of thing already being done in WidgetsApp?

flutter/packages/flutter/lib/src/widgets/app.dart

Lines 1754 to 1758 in 9a347fb

	if (!widget.useInheritedMediaQuery || data == null) {
	child = MediaQuery.fromWindow(
	child: child,
	);
	}

[MitchellGoodwin]

It looks like MediaQuery.fromWindow is planed for deprecation as well. #117480

[MitchellGoodwin]

Sorry I haven't responded to this sooner. I've been keeping an eye on this PR, but because it's dealing with code at the app level, I wanted to get a second opinion before reviewing.

I think the reason why Material works correctly but Cupertino doesn't is that the check for the platform brightness happens in a builder passed to MaterialApp.

flutter/packages/flutter/lib/src/material/app.dart

Line 917 in 9a347fb

final Brightness platformBrightness = MediaQuery.platformBrightnessOf(context);

But in Cupertino it's happening in a static method in CupertinoTheme, so it's not automatically updating when the MediaQuery enters the tree. I'm guessing your solution is indirectly solving the issue by causing a rebuild in CupertinoApp that catches the change in the static method. Ideally the way the theme gets the default brightness would be passed in similar to Material in the builder.

[ivirtex]

I've removed MediaQuery workaround and created _cupertinoBuilder similar to the _materialBuilder in the MaterialApp. In order for text to resolve, DefaultTextStyle must be declared in the builder rather than passing a textStyle to the WidgetsApp.

It works great, but I also found something else, color passed to the WidgetsApp is not being resolved according to the platform brightness, but only to the brightness property of the CupertinoThemeData (the same situation as with the text).
I think there are 2 options here:

1. Leave it as is, let it resolve only to the manually set brightness and end up with 2 CupertinoTheme widgets in the tree (one working correctly from the _cupertinoBuilder and one from the build method of CupertinoApp).
2. Pass color unresolved, similarly to the MaterialApp (no real use in iOS either?):
   

   flutter/packages/flutter/lib/src/material/app.dart

   Lines 966 to 974 in 7ddf42e

   	Widget _buildWidgetApp(BuildContext context) {
   	// The color property is always pulled from the light theme, even if dark
   	// mode is activated. This was done to simplify the technical details
   	// of switching themes and it was deemed acceptable because this color
   	// property is only used on old Android OSes to color the app bar in
   	// Android's switcher UI.
   	//
   	// blue is the primary color of the default theme.
   	final Color materialColor = widget.color ?? widget.theme?.primaryColor ?? Colors.blue;

[MitchellGoodwin]

Thanks for making this update. Is there no way in either builder to get the platform brightness in order to get the right colors? Either through MediaQuery, or the workaround Justin mentioned?

[ivirtex]

Well, it would be technically possible using the WidgetsBindingObserver Justin mentioned, but it would look like reimplementing CupertinoDynamicColor(only for resolving one color!), colors need to be resolved not only by platform brightness, but also by accessibility features e.g. high contrast.
Is color property of WidgetsApp even used on iOS? From what I tested, there is no visual difference when this color is resolved or not.

[ivirtex]

It can also be done by inserting a MediaQuery above the _buildWidgetApp builder, but we would have to wait for the #118004 to land.

[MitchellGoodwin]

It can also be done by inserting a MediaQuery above the _buildWidgetApp builder, but we would have to wait for the #118004 to land.

I think this makes the most sense to me, and we can fix the original issue, and move this one to a new one, if that is taking a while.

[ivirtex]

Alright, it seems that #116924 and #118004 accidentally fixed the original bug with text color and it is now correctly resolved (View, and therefore MediaQuery, is higher in the tree than CupertinoTheme so it can depend on it and grab correct platform brightness).

I've also noticed that DefaultSelectionStyle has its colors unresolved, so I fixed that and added relevant tests.

[MitchellGoodwin]

My apologies @ivirtex, this somewhat slipped my attention. Can you rebase on the latest? It looks like I can't with the new option due to conflicts. Otherwise this looks gtg to me.

[ivirtex]

@MitchellGoodwin Conflicts are now resolved.

Btw, would you mind taking a look at flutter/platform_tests#18?