Repository navigation
Make the Material fallback theme follow defaultTargetPlatform - #194256
auto-submit[bot] merged 3 commits into
Conversation
|
This pull request contains changes to Material or Cupertino, which are currently frozen in this repository. Changes should be made in Please refer to #188444 for instructions. |
There was a problem hiding this comment.
Code Review
This pull request introduces caching for the fallback theme in Theme so that it dynamically updates when defaultTargetPlatform changes, and adds a corresponding test. It also refactors selection handle drag tests in text_field_test.dart by targeting specific platform variants, hiding the toolbar on Fuchsia, and adding a new test for desktop platforms. The review feedback recommends using triple-slash documentation comments for the private fallback theme getter to adhere to the repository style guide, and simplifying redundant switch statements in the iOS-only drag test to direct assertions.
Theme.of falls back to a ThemeData built once per isolate when there is no Theme ancestor. ThemeData() captures defaultTargetPlatform when it is constructed, so the fallback's platform was frozen to whatever platform was in effect the first time a theme-less widget called Theme.of. Under a TargetPlatformVariant, debugDefaultTargetPlatformOverride, or the "o" key in flutter run, every later theme-less subtree kept that first platform's selection controls, toolbar, cursor and other adaptive behavior. In a shuffled test run this made theme-less tests depend on test order. Rebuild the cached fallback whenever its platform no longer matches defaultTargetPlatform, so a theme-less subtree adapts to the current platform the same way a MaterialApp does. Themes built with an explicit platform are unaffected; only the fallback is compared. The two handle-drag tests in text_field_test.dart had only ever run against Android handles regardless of their variant: desktop platforms draw no selection handles, and the Cupertino start handle sits above the line rather than below it. Restrict them to the platforms that draw handles, grab the iOS start handle where it actually is, and hide the Fuchsia toolbar that otherwise covers the handles. Add a test that desktop platforms draw no handles by default, and drop the no-shuffle tag the file no longer needs. Fixes flutter#194008 Fixes flutter#83788 See flutter#122950
bd3a531 to
86a03dd
Compare
The test only runs on iOS now, so the non-Apple branches of the two expectation switches can never execute. Assert the iOS behavior directly.
| expect(Theme.of(capturedContext).platform, defaultTargetPlatform); | ||
| }, variant: TargetPlatformVariant.all()); |
There was a problem hiding this comment.
Wow I'm surprised we never noticed that this would fail before, good catch!
There was a problem hiding this comment.
I'm not sure why all the formatting was triggered in this file but if it passes CI then it's good.
|
Going to hold off on approval of flutter/packages#13278 (flutter-packages port) before landing. |
dkwingsmt
left a comment
There was a problem hiding this comment.
LGTM.
I'm slightly concerned that ThemeData might have more implicit dependencies, but this is a good enough fix for this issue.
**Port of: flutter/flutter#194256 **Issue:** `Theme.of(context)` falls back to a `ThemeData` built once per isolate (`Theme._kFallbackTheme`) when there is no `Theme` ancestor. `ThemeData()` captures `defaultTargetPlatform` at construction, so the fallback's platform is frozen to whatever platform was in effect the first time a theme-less widget called `Theme.of(context)`. Under a `TargetPlatformVariant`, `debugDefaultTargetPlatformOverride` every later theme-less subtree keeps that first platform's selection controls, toolbar, cursor and other adaptive behavior. In a shuffled test run this makes theme-less tests depend on test order: with seed 20261008, text_field_test.dart fails 45 tests that pass in declaration order. **Fix:** Rebuild the cached fallback whenever its platform no longer matches `defaultTargetPlatform`, so a theme-less subtree adapts to the current platform the same way a `MaterialApp` does. `Theme`s built with an explicit platform are unaffected; only the fallback is compared. Adds a regression test in `theme_test.dart` that fails on master for every non-android variant. With the fallback following the variant, the two handle-drag tests in `text_field_test.dart` fail because they had only ever run against Android handles: desktop platforms draw no selection handles, and the Cupertino start handle sits above the line. These tests have been adjusted; restricted to the platforms that draw handles, the iOS start handle is grabbed where it actually is, the Fuchsia toolbar that covers the handles is hidden, and a test asserts that desktop platforms draw no handles. The orphaned no-shuffle tag is removed. **Other alternatives considered:** - `text_field_test.dart` only fix, tests explicitly install their own theme so `Theme.of(context)` does not return leaky fallback. Decided against this since it wouldn't fix this leak for all users as reported in #83788 Fixes [#83788](flutter/flutter#83788) Fixes [#194008](flutter/flutter#194008) ## Pre-Review Checklist **Note**: The Flutter team is currently trialing the use of [Gemini Code Assist for GitHub](https://developers.google.com/gemini-code-assist/docs/review-github-code). Comments from the `gemini-code-assist` bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed. [^1]: Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling.
…utter#194256) **Issue:** `Theme.of(context)` falls back to a `ThemeData` built once per isolate (`Theme._kFallbackTheme`) when there is no `Theme` ancestor. `ThemeData()` captures `defaultTargetPlatform` at construction, so the fallback's platform is frozen to whatever platform was in effect the first time a theme-less widget called `Theme.of(context)`. Under a `TargetPlatformVariant`, `debugDefaultTargetPlatformOverride` every later theme-less subtree keeps that first platform's selection controls, toolbar, cursor and other adaptive behavior. In a shuffled test run this makes theme-less tests depend on test order: with seed 20261008, text_field_test.dart fails 45 tests that pass in declaration order. **Fix:** Rebuild the cached fallback whenever its platform no longer matches `defaultTargetPlatform`, so a theme-less subtree adapts to the current platform the same way a `MaterialApp` does. `Theme`s built with an explicit platform are unaffected; only the fallback is compared. Adds a regression test in `theme_test.dart` that fails on master for every non-android variant. With the fallback following the variant, the two handle-drag tests in `text_field_test.dart` fail because they had only ever run against Android handles: desktop platforms draw no selection handles, and the Cupertino start handle sits above the line. These tests have been adjusted; restricted to the platforms that draw handles, the iOS start handle is grabbed where it actually is, the Fuchsia toolbar that covers the handles is hidden, and a test asserts that desktop platforms draw no handles. The orphaned no-shuffle tag is removed. **Other alternatives considered:** - `text_field_test.dart` only fix, tests explicitly install their own theme so `Theme.of(context)` does not return leaky fallback. Decided against this since it wouldn't fix this leak for all users as reported in flutter#83788 **This will still need a follow up PR that does the same change in `material_ui`** Fixes flutter#83788 Fixes flutter#194008 ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [AI contribution guidelines] and understand my responsibilities, or I am not using AI tools. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant in-code documentation (doc comments with `///`). - [x] If this PR introduces a new feature or capability, I created and linked a website documentation issue or PR in [flutter/website] (or verified none is needed). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing.
…lutter#194008) Reverts flutter#193550 and restores the no-shuffle tag definition in packages/flutter/dart_test.yaml on flutter-3.50-candidate.0, where Theme._kFallbackTheme platform caching (flutter#194008, fixed after branch cut in flutter#194256) causes 56 tests in text_field_test.dart to fail under shuffled test seeds such as 20261103.
This pull request is created by [automatic cherry pick workflow](https://github.com/flutter/flutter/blob/main/docs/releases/Flutter-Cherrypick-Process.md#automatically-creates-a-cherry-pick-request) Cherry-pick of #193528 (already cherry-picked to `3.47.9` stable in #195015, ensuring `3.50` does not regress public API shipped in `3.47.9`). Also includes `e58c176fe4` / `7d0aca2908` (`+12/-1`, test-only) restoring the `no-shuffle` tag on `packages/flutter/test/material/text_field_test.dart` (reverting #193550 and restoring `no-shuffle` in `packages/flutter/dart_test.yaml`) because `3.50` branched before #194256 fixed #194008 (`Theme._kFallbackTheme` platform caching across shuffled `TargetPlatformVariant` tests). ### Issue Link: #193527 #194008 ### Impact Description: `ContextMenuButtonKind` and `ContextMenuButtonItem.kind` shipped in Flutter `3.47.9` stable (`#195015`) after `flutter-3.50-candidate.0` branched. Without cherry-picking `#193528` into `flutter-3.50-candidate.0`, packages and apps adopting `ContextMenuButtonKind` on `3.47.9` stable (including `cupertino_ui` and `material_ui` for iOS Translate context menu support in `#180021`) would fail to compile on `3.50.0`. The change itself is purely additive and does not deprecate `ContextMenuButtonType` or alter existing behavior. Restoring `no-shuffle` on `text_field_test.dart` unblocks `framework_tests_libraries` and `Mac framework_tests_impeller` on `flutter-3.50-candidate.0` under daily shuffled test seeds. ### Changelog Description: - [flutter/193527](#193527) When building custom text selection menus, packages can't add new actions such as Translate without breaking exhaustive switches on `ContextMenuButtonType`; adds the extensible `ContextMenuButtonKind`. ### Workaround: No. ### Risk: What is the risk level of this cherry-pick? ### Test Coverage: Are you confident that your fix is well-tested by automated tests? ### Validation Steps: Run `flutter test test/widgets/context_menu_button_item_test.dart test/widgets/editable_text_test.dart test/widgets/selectable_region_test.dart test/widgets/system_context_menu_test.dart` and `flutter test --test-randomize-ordering-seed=20261103 --fatal-warnings test/material/text_field_test.dart` to verify `ContextMenuButtonKind` backwards compatibility and `text_field_test.dart` execution under shuffled seeds.
Issue:
Theme.of(context)falls back to aThemeDatabuilt once per isolate (Theme._kFallbackTheme) when there is noThemeancestor.ThemeData()capturesdefaultTargetPlatformat construction, so the fallback's platform is frozen to whatever platform was in effect the first time a theme-less widget calledTheme.of(context). Under aTargetPlatformVariant,debugDefaultTargetPlatformOverrideevery later theme-less subtree keeps that first platform's selection controls, toolbar, cursor and other adaptive behavior. In a shuffled test run this makes theme-less tests depend on test order: with seed 20261008, text_field_test.dart fails 45 tests that pass in declaration order.Fix: Rebuild the cached fallback whenever its platform no longer matches
defaultTargetPlatform, so a theme-less subtree adapts to the current platform the same way aMaterialAppdoes.Themes built with an explicit platform are unaffected; only the fallback is compared. Adds a regression test intheme_test.dartthat fails on master for every non-android variant.With the fallback following the variant, the two handle-drag tests in
text_field_test.dartfail because they had only ever run against Android handles: desktop platforms draw no selection handles, and the Cupertino start handle sits above the line. These tests have been adjusted; restricted to the platforms that draw handles, the iOS start handle is grabbed where it actually is, the Fuchsia toolbar that covers the handles is hidden, and a test asserts that desktop platforms draw no handles. The orphaned no-shuffle tag is removed.Other alternatives considered:
text_field_test.dartonly fix, tests explicitly install their own theme soTheme.of(context)does not return leaky fallback. Decided against this since it wouldn't fix this leak for all users as reported in [Widget Tests]variantanddebugDefaultTargetPlatformOverridedo not work as expected leading to failed tests #83788This will still need a follow up PR that does the same change in
material_uiFixes #83788
Fixes #194008
Pre-launch Checklist
///).