Fix Container's child state loss. by ksokolovskyi · Pull Request #163419 · flutter/flutter

ksokolovskyi · 2025-02-16T11:50:18Z

Description

Updates Container widget to be Stateful and use GlobalKey to preserve the child's state
Replaces some occurrences of Container in framework_test.dart with custom _Wrapper widget to retain tests readability. Without this change, we would have to check for KeyedSubtree-[GlobalKey#00000] in tests, instead of Container-[<1>] which decreases readability.
Replace Container with SizedBox in lookup_boundary_test.dart, because the test didn't expect the additional KeyedSubtree widget inside the Container.
Replaces Container with KeyedSubtree in packages/flutter_test/lib/src/binding.dart. This is because using the Container in TestWidgetsFlutterBinding._runTestBody introduces two additional GlobalKey which affect existing GlobalKey tests.

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.

ksokolovskyi · 2025-02-16T13:58:29Z

There is one failing test in flutter/packages/rfw package: https://github.com/flutter/packages/blob/8542af3ddbe4a3fd353b31b28f5fb04065097b3b/packages/rfw/test/runtime_test.dart#L894C15-L894C41

This test tries to get the element of the Container widget pumped by the TestWidgetsFlutterBinding._runTestBody function. This PR replaces that Container with KeyedSubtree (https://github.com/flutter/flutter/pull/163419/files#diff-d34dbaacc51359fa05706c29bf6f9defc134c57d31b2d84313b1bbcc54bd2773L1052) which breaks the test.

Could you please advise what to do in this case?

justinmc · 2025-02-18T23:07:43Z

@LongCatIsLooong FYI I think we were discussing this problem before you went on leave, see #161698.

flutter-dashboard · 2025-03-10T23:08:59Z

This pull request executed golden file tests, but it has not been updated in a while (20+ days). Test results from Gold expire after as many days, so this pull request will need to be updated with a fresh commit in order to get results from Gold.

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.

Piinks · 2025-03-11T22:27:07Z

Hey from triage, @LongCatIsLooong and @ksokolovskyi , what is the status of this PR?

ksokolovskyi · 2025-03-12T06:54:23Z

Hi @Piinks. From my side, the PR is ready for review, but I am unsure how to proceed with one failing test in the flutter/packages/rfw package. I described the issue in a comment above.

justinmc

I think this PR will require a lot of work to prove this is a valid solution that is not negatively affecting performance, but I appreciate you taking on such a difficult and fundamental problem @ksokolovskyi!

This is the failing rfw test. Can you see if it's something trivial that can be updated? It's probably just depending on Container producing a ColoredBox even when not given a color. https://github.com/flutter/packages/blob/ff7724c18a803542fc709f942c3bf1cecf7e4864/packages/rfw/test/runtime_test.dart#L894

justinmc · 2025-03-14T23:30:25Z

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

 ///  * Cookbook: [Animate the properties of a container](https://docs.flutter.dev/cookbook/animation/animated-container)
 ///  * The [catalog of layout widgets](https://docs.flutter.dev/ui/widgets/layout).
-class Container extends StatelessWidget {
+class Container extends StatefulWidget {


I wonder what the performance implications are about changing this to a stateful widget since Container is used all over the place.

@justinmc what could you suggest to prove that there is no performance decrease?
Maybe some benchmark like #148261?

I am also against turning this into stateful widget.

I am not sure what may be a good way to write a benchmark for state vs statefulwidget comparison. We can't accurately estimate how many containers are used in real world.

justinmc · 2025-03-14T23:32:00Z

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

+}
+
+class _ContainerState extends State<Container> {
+  final GlobalKey _childGlobalKey = GlobalKey();


This also might be a performance concern. Somewhat related: #161698 (comment)

justinmc · 2025-03-14T23:33:28Z

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


-    if (child == null && (constraints == null || !constraints!.isTight)) {
+    if (current != null) {
+      current = KeyedSubtree(key: _childGlobalKey, child: current);


@chunhtai I think you and I talked about solving this problem with a KeyedSubtree before and decided against it, do you remember the details at all?

@justinmc @chunhtai could you share your concerns about using KeyedSubtree?

the main concern is the use of global key. They are a lot more costly than using regular key.

ksokolovskyi · 2025-04-12T12:27:51Z

Hi @justinmc, thanks a lot for taking a look at this PR!
I opened PR with test fixes for the rfw package and am waiting for its approval: flutter/packages#9063

chunhtai

We should avoid using globalkey and statefulwidget as well in a basic building block widget.

chunhtai · 2025-04-14T17:13:41Z

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

 ///  * Cookbook: [Animate the properties of a container](https://docs.flutter.dev/cookbook/animation/animated-container)
 ///  * The [catalog of layout widgets](https://docs.flutter.dev/ui/widgets/layout).
-class Container extends StatelessWidget {
+class Container extends StatefulWidget {


I am also against turning this into stateful widget.

I am not sure what may be a good way to write a benchmark for state vs statefulwidget comparison. We can't accurately estimate how many containers are used in real world.

chunhtai · 2025-04-14T17:16:40Z

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


-    if (child == null && (constraints == null || !constraints!.isTight)) {
+    if (current != null) {
+      current = KeyedSubtree(key: _childGlobalKey, child: current);


the main concern is the use of global key. They are a lot more costly than using regular key.

…estWidgets`. (#9063) This PR updates `rfw` package tests to no longer depend on a `Container` pumped via `testWidgets` to unblock further work on flutter/flutter#163419 ## Pre-Review Checklist [^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.

ksokolovskyi · 2025-06-17T15:36:54Z

Closing this PR as the solution proposed is not an acceptable one for a basic building block widget.
The possible solutions are outlined here: #161698 (comment)

…estWidgets`. (flutter#9063) This PR updates `rfw` package tests to no longer depend on a `Container` pumped via `testWidgets` to unblock further work on flutter/flutter#163419 ## Pre-Review Checklist [^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.

github-actions bot added a: tests "flutter test", flutter_test, or one of our tests framework flutter/packages/flutter repository. See also f: labels. labels Feb 16, 2025

Fix Container's child state loss.

9fc1456

ksokolovskyi force-pushed the fix-container-child-state-loss branch from 99b8473 to 9fc1456 Compare March 12, 2025 06:52

justinmc reviewed Mar 14, 2025

View reviewed changes

ksokolovskyi mentioned this pull request Apr 12, 2025

[rfw] Update tests to no longer depend on a Container pumped via testWidgets. flutter/packages#9063

Merged

11 tasks

Merge branch 'master' into fix-container-child-state-loss

665adc1

chunhtai reviewed Apr 14, 2025

View reviewed changes

bleroux mentioned this pull request Jun 2, 2025

Revert "Fix NavigationBar indicator overlay color (#164484)" #169497

Merged

ksokolovskyi closed this Jun 17, 2025

Conversation

ksokolovskyi commented Feb 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Pre-launch Checklist

Uh oh!

ksokolovskyi commented Feb 16, 2025

Uh oh!

justinmc commented Feb 18, 2025

Uh oh!

flutter-dashboard bot commented Mar 10, 2025

Uh oh!

Piinks commented Mar 11, 2025

Uh oh!

ksokolovskyi commented Mar 12, 2025

Uh oh!

justinmc left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ksokolovskyi commented Apr 12, 2025

Uh oh!

chunhtai left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ksokolovskyi commented Jun 17, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ksokolovskyi commented Feb 16, 2025 •

edited

Loading

justinmc left a comment •

edited

Loading