Feat: Add brightnessOf method for theme #163733

rkishan516 · 2025-02-20T15:40:06Z

Feat: Add brightnessOf method for theme
fixes: #163393

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.

rkishan516 · 2025-02-20T15:41:25Z

This PR doesn't include implementation of InheritedModel for Theme.

navaronbracke · 2025-02-21T12:07:18Z

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

+    return inheritedTheme?.theme.data.brightness ?? MediaQuery.platformBrightnessOf(context);
+  }
+
+  /// Retrieves the [Brightness] to use for descendant Cupertino widgets, based


Same here

Suggested change

/// Retrieves the [Brightness] to use for descendant Cupertino widgets, based

/// Retrieves the [Brightness] to use for descendant Material widgets, based

/// on the value of [ThemeData.brightness] in the given [context].

///

/// If no [InheritedTheme] can be found in the given [context], it will fall

/// back to [MediaQueryData.platformBrightness].

///

/// Returns null if no valid [InheritedTheme] or [MediaQuery] widgets exist in

/// the ancestry tree.

///

/// See also:

///

/// * [ThemeData.brightness], the property that takes precedence over

/// [MediaQueryData.platformBrightness] for descendant Material widgets.

/// * [brightnessOf], which throws if no valid [InheritedTheme] or

/// [MediaQuery] exists, instead of returning null.

navaronbracke · 2025-02-21T12:13:24Z

packages/flutter/test/material/theme_test.dart

@@ -44,7 +44,7 @@ void main() {
    await tester.tap(find.byKey(popupMenuButtonKey));
    await tester.pump(const Duration(seconds: 1));

-    expect(Theme.of(tester.element(find.text('menuItem'))).brightness, equals(Brightness.dark));
+    expect(Theme.brightnessOf(tester.element(find.text('menuItem'))), equals(Brightness.dark));


I would propose to not do this change in this PR, we can probably do all the ThemeData.brightness changes in a follow up PR, after this one lands. (which probably makes it a bit easier to review)

Instead, I would test the behavior of brightnessOf / maybeBrightnessOf in separate tests:

brightnessOf returns the correct brightness if only a MediaQuery is provided

brightnessOf returns the correct brightness if both a MediaQuery & ThemeData are provided

brightnessOf throws if neither a MediaQuery or ThemeData is provided

maybeBrightnessOf returns the correct brightness if only a MediaQuery is provided

maybeBrightnessOf returns the correct brightness if both a MediaQuery & ThemeData are provided

maybeBrightnessOf returns null if neither a MediaQuery or ThemeData is provided

I will file a follow up issue to do the former sparately.

Sure, let me create another test which just verify that Theme.brightnessOf works in all scenario

@navaronbracke I don't think scenario 3 and 6 can be produced, because in testing media query always return platform brightness.

For these tests I would just pump a widget with the WidgetTester:

Only pumps a MediaQuery widget with a Builder as child (to get the context for the brightness test)

Pumps both a MediaQuery & a Theme widget, with a Builder as child

Pumps only a Builder widget and never pumps a MediaQuery or Theme widget

Cases 4-6 are variations, but use the other method.

You should be able to give the MediaQuery a custom MediaQueryData, to force the brightness?

Yes, MediaQuery takes data as input.

navaronbracke · 2025-02-21T13:08:00Z

packages/flutter/test/material/theme_test.dart

+      await tester.pumpWidget(
+        MediaQuery(
+          data: const MediaQueryData(platformBrightness: Brightness.dark),
+          child: Container(),


I would use a SizedBox, instead of a Container to make the tests a little lighter.

navaronbracke · 2025-02-21T13:10:03Z

packages/flutter/test/material/theme_test.dart

+      await tester.pumpWidget(
+        Theme(
+          data: ThemeData(brightness: Brightness.light),
+          child: MediaQuery(


Should the MediaQuery be above the Theme, for consistency with the real widget tree in MaterialApp?

navaronbracke · 2025-02-21T13:13:04Z

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

+  ///
+  /// See also:
+  ///
+  /// * [ThemeData.brightness], the property takes precedence over


Suggested change

/// * [ThemeData.brightness], the property takes precedence over

/// * [ThemeData.brightness], the property that takes precedence over

navaronbracke · 2025-02-21T13:13:57Z

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

+  ///
+  /// * [maybeBrightnessOf], which returns null if no valid [InheritedTheme] or
+  ///   [MediaQuery] exists, instead of throwing.
+  /// * [ThemeData.brightness], the property takes precedence over


Suggested change

/// * [ThemeData.brightness], the property takes precedence over

/// * [ThemeData.brightness], the property that takes precedence over

navaronbracke · 2025-02-21T13:15:29Z

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

+  /// * [maybeBrightnessOf], which returns null if no valid [InheritedTheme] or
+  ///   [MediaQuery] exists, instead of throwing.
+  /// * [ThemeData.brightness], the property takes precedence over
+  ///   [MediaQueryData.platformBrightness] for descendant widgets.


I would keep the mention of Material, since Theme is a Material thing and not under the widgets library.

Suggested change

/// [MediaQueryData.platformBrightness] for descendant widgets.

/// [MediaQueryData.platformBrightness] for descendant Material widgets.

navaronbracke · 2025-02-21T13:16:22Z

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

@@ -161,6 +161,48 @@ class Theme extends StatelessWidget {
        .resolveFrom(context);
  }

+  /// Retrieves the [Brightness] to use for descendant widgets, based


I would keep the mention of Material, since Theme is a Material thing and not under the widgets library.

Suggested change

/// Retrieves the [Brightness] to use for descendant widgets, based

/// Retrieves the [Brightness] to use for descendant Material widgets, based

navaronbracke · 2025-02-21T13:17:09Z

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

+    return inheritedTheme?.theme.data.brightness ?? MediaQuery.platformBrightnessOf(context);
+  }
+
+  /// Retrieves the [Brightness] to use for descendant widgets, based


I would keep the mention of Material, since Theme is a Material thing and not under the widgets library.

Suggested change

/// Retrieves the [Brightness] to use for descendant widgets, based

/// Retrieves the [Brightness] to use for descendant Material widgets, based

navaronbracke · 2025-02-21T13:17:35Z

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

+  /// See also:
+  ///
+  /// * [ThemeData.brightness], the property takes precedence over
+  ///   [MediaQueryData.platformBrightness] for descendant widgets.


I would keep the mention of Material, since Theme is a Material thing and not under the widgets library.

Suggested change

/// [MediaQueryData.platformBrightness] for descendant widgets.

/// [MediaQueryData.platformBrightness] for descendant Material widgets.

navaronbracke

Minor docs correction and a suggestion for some missing tests.

navaronbracke · 2025-02-22T10:53:36Z

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

+  /// If no [InheritedTheme] can be found in the given [context], or its `brightness`
+  /// is null, it will fall back to [MediaQueryData.platformBrightness].
+  ///
+  /// Throws an exception if no valid [InheritedTheme] or [MediaQuery] widgets


Suggested change

/// Throws an exception if no valid [InheritedTheme] or [MediaQuery] widgets

I had a look at the implementation of https://api.flutter.dev/flutter/widgets/MediaQuery/platformBrightnessOf.html

and it returns Brightness.light instead of throwing if there is no MediaQuery.
So you can remove the phrase that says it throws.

packages/flutter/test/material/theme_test.dart

navaronbracke

Some minor docs adjustments, to match the actual behavior of the method that is used, along with a clarification for one test.

navaronbracke · 2025-02-22T12:44:50Z

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

+  /// See also:
+  ///
+  /// * [maybeBrightnessOf], which returns null if no valid [InheritedTheme] or
+  ///   [MediaQuery] exists, instead of throwing.


Since platformBrightnessOf does not throw, but returns a default, I think we can tweak the wording here.

Suggested change

/// [MediaQuery] exists, instead of throwing.

/// [MediaQuery] exists.

navaronbracke · 2025-02-22T12:46:32Z

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

+  /// If no [InheritedTheme] or [MediaQuery] can be found in the given [context], it will
+  /// return null.
+  ///
+  /// Returns null if no valid [InheritedTheme] or [MediaQuery] widgets exist in


Can we remove this phrase? Currently it says returns null twice

navaronbracke · 2025-02-22T12:47:30Z

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

+  ///
+  /// * [ThemeData.brightness], the property that takes precedence over
+  ///   [MediaQueryData.platformBrightness] for descendant Material widgets.
+  /// * [brightnessOf], which throws if no valid [InheritedTheme] or


Same tweak here, since that method does not throw.

Suggested change

/// * [brightnessOf], which throws if no valid [InheritedTheme] or

/// * [brightnessOf], which returns a default value if no valid [InheritedTheme] or

navaronbracke · 2025-02-22T12:50:19Z

packages/flutter/test/material/theme_test.dart

+    testWidgets('returns Brightness.light when no theme or media query is present', (
+      WidgetTester tester,
+    ) async {
+      await tester.pumpWidget(const SizedBox());


We need to prevent the View from adding the MediaQuery here, otherwise there is an implicit MediaQuery

Suggested change

await tester.pumpWidget(const SizedBox());

// Prevent the implicitly added View from adding a MediaQuery

await tester.pumpWidget(

RawView(view: FakeFlutterView(tester.view, viewId: 77), child: const SizedBox()),

wrapWithView: false,

);

navaronbracke · 2025-02-22T15:18:28Z

@rkishan516 While to me this PR looks good now, I will have to defer to people on the framework team for a review / explicit approval. So you will have to be patient a little :)

See https://github.com/flutter/flutter/blob/master/docs/contributing/Tree-hygiene.md#when for more info on the review process.

rkishan516 · 2025-02-22T15:55:07Z

@rkishan516 While to me this PR looks good now, I will have to defer to people on the framework team for a review / explicit approval. So you will have to be patient a little :)

See https://github.com/flutter/flutter/blob/master/docs/contributing/Tree-hygiene.md#when for more info on the review process.

Sure and Thanks for helping @navaronbracke.

QuncCccccc

Can we use Theme.of(context).brightness to detect the brightness?

navaronbracke · 2025-03-14T19:46:35Z

That is how you would do this today. However, this PR aims to make it explicit, using a named method, and is a step towards eventually making Theme an InheritedModel. The latter is the main reasoning behind this change.

dkwingsmt · 2025-03-14T20:05:26Z

@navaronbracke I'm not familiar with this work, but is there an issue tracking our progress (and discussions, if any) towards making Theme an InheritedModel?

navaronbracke · 2025-03-14T20:10:32Z

@nate-thegrate made a design doc that talks about this a little in https://docs.google.com/document/d/1MfU6gkgIgXCxa5E25sPYuAP5Bmgehyl3Hk7yDH24zl0/edit?tab=t.0

dkwingsmt · 2025-03-14T20:25:54Z

I've asked @MitchellGoodwin and it seems the conversation didn't reached a point were a decision was made. Although I agree with the motivation of the work, I think it'd be better not to move forward until we have agreed on the exact design and plan.

If you would like to drive this project, you can start by writing a more detailed design doc with thorough planning and introduction, and we can seek comments or even host a Dash forum for discussion.

MitchellGoodwin · 2025-03-14T20:58:13Z

Outside of InheritedModel, there is value in adding a brightnessOf method to Material for the sake of API parity since Cupertino has a brightnessOf method. Material and Cupertino handle their brightness differently though, in such a way were I don't think this method would decrease rebuilds the way it does for Cupertino theme.

navaronbracke · 2025-03-14T21:30:17Z

Outside of InheritedModel, there is value in adding a brightnessOf method to Material for the sake of API parity since Cupertino has a brightnessOf method. Material and Cupertino handle their brightness differently though, in such a way were I don't think this method would decrease rebuilds the way it does for Cupertino theme.

Yes, feature parity between the two was also something I mentioned in the OP of #163393

rkishan516 · 2025-03-14T22:37:06Z

Outside of InheritedModel, there is value in adding a brightnessOf method to Material for the sake of API parity since Cupertino has a brightnessOf method. Material and Cupertino handle their brightness differently though, in such a way were I don't think this method would decrease rebuilds the way it does for Cupertino theme.

If we are able to push Theme as InheritedModel, then later part will not be an issue. If we decide to go with migrating Theme to InheritedModel, I won't mind doing that change.

github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. labels Feb 20, 2025

navaronbracke reviewed Feb 21, 2025

View reviewed changes

navaronbracke mentioned this pull request Feb 21, 2025

[Proposal] Migrate framework Material widgets to Theme.brightnessOf(context) #163837

Open

rkishan516 force-pushed the brightness-of-theme branch from 4226de7 to c2e7da0 Compare February 21, 2025 12:44

rkishan516 requested a review from navaronbracke February 21, 2025 12:51

navaronbracke reviewed Feb 21, 2025

View reviewed changes

rkishan516 force-pushed the brightness-of-theme branch from c2e7da0 to f1770bd Compare February 21, 2025 14:52

rkishan516 requested a review from navaronbracke February 21, 2025 16:06

navaronbracke reviewed Feb 22, 2025

View reviewed changes

rkishan516 force-pushed the brightness-of-theme branch 3 times, most recently from 3cfa06b to b6c93e8 Compare February 22, 2025 11:54

rkishan516 requested a review from navaronbracke February 22, 2025 12:40

navaronbracke reviewed Feb 22, 2025

View reviewed changes

rkishan516 force-pushed the brightness-of-theme branch from b6c93e8 to 26f49a7 Compare February 22, 2025 13:25

rkishan516 requested a review from navaronbracke February 22, 2025 15:10

rkishan516 force-pushed the brightness-of-theme branch 2 times, most recently from 95dd5a5 to 227f414 Compare March 1, 2025 01:20

dkwingsmt requested review from dkwingsmt and QuncCccccc and removed request for navaronbracke March 12, 2025 18:50

QuncCccccc reviewed Mar 14, 2025

View reviewed changes

Feat: Add brightnessOf method for theme

1f075f8

rkishan516 force-pushed the brightness-of-theme branch from bf5e57e to 1f075f8 Compare March 15, 2025 02:18

rkishan516 mentioned this pull request Mar 17, 2025

Feat: Migrate Theme to InheritedModel #165289

Draft

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat: Add brightnessOf method for theme #163733

Feat: Add brightnessOf method for theme #163733

rkishan516 commented Feb 20, 2025

rkishan516 commented Feb 20, 2025

navaronbracke Feb 21, 2025

navaronbracke Feb 21, 2025

rkishan516 Feb 21, 2025 •

edited

Loading

rkishan516 Feb 21, 2025

navaronbracke Feb 21, 2025 •

edited

Loading

rkishan516 Feb 21, 2025

navaronbracke Feb 21, 2025

navaronbracke Feb 21, 2025

navaronbracke Feb 21, 2025

navaronbracke Feb 21, 2025

navaronbracke Feb 21, 2025

navaronbracke Feb 21, 2025

navaronbracke Feb 21, 2025

navaronbracke Feb 21, 2025

navaronbracke left a comment

navaronbracke Feb 22, 2025

navaronbracke left a comment

navaronbracke Feb 22, 2025

navaronbracke Feb 22, 2025

navaronbracke Feb 22, 2025

navaronbracke Feb 22, 2025

navaronbracke commented Feb 22, 2025

rkishan516 commented Feb 22, 2025

QuncCccccc left a comment

navaronbracke commented Mar 14, 2025

dkwingsmt commented Mar 14, 2025

navaronbracke commented Mar 14, 2025

dkwingsmt commented Mar 14, 2025 •

edited

Loading

MitchellGoodwin commented Mar 14, 2025

navaronbracke commented Mar 14, 2025

rkishan516 commented Mar 14, 2025

	/// * [ThemeData.brightness], the property takes precedence over
	/// * [ThemeData.brightness], the property that takes precedence over

	/// [MediaQueryData.platformBrightness] for descendant widgets.
	/// [MediaQueryData.platformBrightness] for descendant Material widgets.

	/// Retrieves the [Brightness] to use for descendant widgets, based
	/// Retrieves the [Brightness] to use for descendant Material widgets, based

	/// [MediaQuery] exists, instead of throwing.
	/// [MediaQuery] exists.

	/// * [brightnessOf], which throws if no valid [InheritedTheme] or
	/// * [brightnessOf], which returns a default value if no valid [InheritedTheme] or

-      await tester.pumpWidget(const SizedBox());
+      // Prevent the implicitly added View from adding a MediaQuery
+      await tester.pumpWidget(
+        RawView(view: FakeFlutterView(tester.view, viewId: 77), child: const SizedBox()),
+        wrapWithView: false,
+      );

Feat: Add brightnessOf method for theme #163733

Are you sure you want to change the base?

Feat: Add brightnessOf method for theme #163733

Conversation

rkishan516 commented Feb 20, 2025

Pre-launch Checklist

rkishan516 commented Feb 20, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rkishan516 Feb 21, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

navaronbracke Feb 21, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

navaronbracke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

navaronbracke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

navaronbracke commented Feb 22, 2025

rkishan516 commented Feb 22, 2025

QuncCccccc left a comment

Choose a reason for hiding this comment

navaronbracke commented Mar 14, 2025

dkwingsmt commented Mar 14, 2025

navaronbracke commented Mar 14, 2025

dkwingsmt commented Mar 14, 2025 • edited Loading

MitchellGoodwin commented Mar 14, 2025

navaronbracke commented Mar 14, 2025

rkishan516 commented Mar 14, 2025

rkishan516 Feb 21, 2025 •

edited

Loading

navaronbracke Feb 21, 2025 •

edited

Loading

dkwingsmt commented Mar 14, 2025 •

edited

Loading