Nav bar static components respect ambient MediaQueryData #174673
Conversation
There was a problem hiding this comment.
Code Review
This pull request correctly addresses an issue where CupertinoNavigationBar components failed to respect ambient MediaQueryData, leading to incorrect rendering in dark mode. The fix, which involves using MediaQuery.of(context).copyWith() instead of creating a new MediaQueryData, is appropriate and well-implemented across all affected components. The new test case effectively validates this change. My review includes a suggestion to improve maintainability by refactoring duplicated code and a high-severity comment to fix a potential bug in the new test to ensure its reliability.
| middle: const _ExpectCustomMediaQuery(value: value, index: 0x000100), | ||
| trailing: CupertinoButton( | ||
| onPressed: () {}, | ||
| child: const _ExpectCustomMediaQuery(value: value, index: 0x010000), |
There was a problem hiding this comment.
While the current way is acceptable, I'd prefer if you can explore a better style that writes the expects in the test body instead of in the class, which is hard to find.
Can you make _ExpectCustomMediaQuery accept a void callback instead and define the expects in the callback?
There was a problem hiding this comment.
Also probably add a "regression test" comment explaining why these expectations make sense
There was a problem hiding this comment.
While the current way is acceptable, I'd prefer if you can explore a better style that writes the expects in the test body instead of in the class, which is hard to find.
Ah. I implemented it that way because it was consistent with the _ExpectStyles class in the same file. But then the _ExpectStyles class is reused across multiple tests. I can explore a solution contained entirely within the test itself.
| const double value = 10.0; | ||
|
|
||
| void expectCustomMediaQueryData(BuildContext context) { | ||
| expect(MediaQuery.platformBrightnessOf(context), Brightness.dark); |
There was a problem hiding this comment.
This looks great, but is it ok if it doesn't check the value anymore? (Which I'm ok with since the original issue wasn't about custom values)
There was a problem hiding this comment.
The problem with that is a future change might copy over only the mediaQueryData brightness leaving the other properties out. And we want to prevent against that regression. But MediaQueryData has many properties, so I'm thinking of testing with a subset of those properties numerous enough to protect against regression.
What do you think is the optimal approach here?
There was a problem hiding this comment.
Yeah I think this is sufficient. Just a few is enough to verify our approach here, and thoroughly testing copyWith should be done in MediaQueryData's tests.
flutter/flutter@973320c...a082096 2025-09-09 engine-flutter-autoroll@skia.org Roll Packages from 24588c6 to 2d651b2 (2 revisions) (flutter/flutter#175130) 2025-09-09 engine-flutter-autoroll@skia.org Roll Skia from ab13fd19dd32 to 19ba56dde579 (2 revisions) (flutter/flutter#175127) 2025-09-09 engine-flutter-autoroll@skia.org Roll Skia from 29a015f8712b to ab13fd19dd32 (2 revisions) (flutter/flutter#175108) 2025-09-09 engine-flutter-autoroll@skia.org Roll Skia from 6a4613b83365 to 29a015f8712b (5 revisions) (flutter/flutter#175106) 2025-09-09 engine-flutter-autoroll@skia.org Roll Dart SDK from 7b645442db0f to f446144fb7c9 (2 revisions) (flutter/flutter#175104) 2025-09-08 victorsanniay@gmail.com Nav bar static components respect ambient MediaQueryData (flutter/flutter#174673) 2025-09-08 mohellebiabdessalem@gmail.com fix typo in test documentation #2 (flutter/flutter#174707) 2025-09-08 jason-simmons@users.noreply.github.com Update ImageReaderSurfaceProducer.MAX_IMAGES to include the maximum number of retained dequeued images (flutter/flutter#174971) 2025-09-08 49699333+dependabot[bot]@users.noreply.github.com Bump actions/labeler from 5.0.0 to 6.0.1 in the all-github-actions group (flutter/flutter#175093) 2025-09-08 engine-flutter-autoroll@skia.org Roll Skia from 25f00cb247f2 to 6a4613b83365 (3 revisions) (flutter/flutter#175087) 2025-09-08 engine-flutter-autoroll@skia.org Roll Dart SDK from 83c6b6124380 to 7b645442db0f (1 revision) (flutter/flutter#175086) 2025-09-08 oreofesolarin@gmail.com Impeller: Convert GLProc name field and GLErrorToString to std::string_view (flutter/flutter#173771) 2025-09-08 chinmaygarde@google.com Depend on operator overload synthesis for three-way and equality comparisons. (flutter/flutter#174892) 2025-09-08 chinmaygarde@google.com Define a concept for UniqueObjectTraits. (flutter/flutter#174905) 2025-09-08 engine-flutter-autoroll@skia.org Roll Skia from 0c2b0a00b7b5 to 25f00cb247f2 (1 revision) (flutter/flutter#175077) 2025-09-08 magder@google.com Fix GitHub labeler platform-android typo (flutter/flutter#175076) 2025-09-08 jessiewong401@gmail.com [Gradle 9] Removed `minSdkVersion` and only use `minSdk` (flutter/flutter#173892) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC stuartmorgan@google.com,tarrinneal@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Fixes CupertinoSlidingSegmentedControl in CupertinoNavigationBar renders incorrectly in dark brightness (starting 3.35.0)
Fixes [Cupertino] MediaQuery.of(context) returns Size(0,0) inside CupertinoNavigationBar trailing since Flutter 3.35
Bug was introduced in CupertinoSliverNavigationBar respects accessibility text scaling
See #174642 (comment)