Make sure `LayoutBuilder` rebuild in an inactive route #154681

LongCatIsLooong · 2024-09-05T18:53:06Z

Prompted by #154060: widgets should always rebuild even when off-screen. The ancestor widget could be trying to pass down information that is not related to the UI state, or trying to pause video playback. Widgets with global keys should also always rebuild to make sure the widget tree is consistent in terms of global keys.

~~Also prevents unnecessary repaints: #106306 (comment)~~

This works by adding _RenderLayoutBuilder as a relayout boundary in the dirtly layout list so the layout callback always gets a chance to run if marked dirty.

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.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

goderbauer · 2024-09-06T20:43:29Z

packages/flutter/lib/src/rendering/object.dart

+    // This ensures that the layout callback will be run even if an ancestor
+    // chooses to not lay out this subtree (for example, OverlayEntries with
+    // `maintainState` set to true).
+    owner?._nodesNeedingLayout.add(this);


For my own understanding: Wouldn't this case child nodes to potentially also get layed out unnecessary?

Or is the idea that since the contrains haven't changed, the layout builder is pretty much guaranteed to pass the same constraints to its child and layout would short-circuit right there?

Yeah the render children may also do layout now which can be unnecessary. But I think (at least for RenderTheater) if you rebuild a widget subtree in an inactive maintainState OverlayEntry, relayout boundaries in the render subtree are still operational and call performLayout as usual?

Is the reason that we don't want to find the closest boundary because we are not sure whether between that and here may skips the layout?

Is it possible to short circuit the layout pass and directly marksNeedsBuild of the subtree if this is in active route? I am a little concern about removing the check that layout can start at non layout boundary.

Is it possible to short circuit the layout pass and directly marksNeedsBuild of the subtree if this is in active route? I am a little concern about removing the check that layout can start at non layout boundary.

I am not aware of any other ways to detect whether a LayoutBuilders layout has been skipped, other than adding its node to the dirtly layout list.

If the parent skips layout, then it doesn't care about the size of this render object so this render object is effectively a relayout boundary.

yeah, I can't think of a better way to do this. especially we only want to rebuild during layout phase .

goderbauer · 2024-09-06T20:48:08Z

packages/flutter/lib/src/rendering/object.dart

+  // The initial value of this flag must be set to true to prevent the layout
+  // callback from being called when the tree has never been laid out.


I don't understand this comment: _needsRebuild doesn't seem to be used as a guard for runLayoutCallback. And also, why do we not want to run the callback during the first time the tree is laid out?

This RenderObject will be added to the PipelineOwner's dirty list, but if its ancestors were never laid out we can't run the layout callback because the constraints are not known yet. Changed the class documentation to

/// A mixin for [RenderObject] subclasses with a layout callback. The mixin
/// guarantees the layout callback will be called even if this [RenderObject]
/// skips doing layout, unless the [RenderObject] has never been laid out and
/// does not have valid [constraints].

This new explanation sounds good to me, but you may have forgotten to push the change up to github.

goderbauer · 2024-09-06T20:54:01Z

packages/flutter/test/widgets/layout_builder_test.dart

@@ -850,6 +850,52 @@ void main() {
    expect(paintCount, 3);
    expect(mostRecentOffset, const Offset(60, 60));
  });
+
+  testWidgets('LayoutBuilder in a subtree that skips layout does not rebuild during the initial treewalk', (WidgetTester tester) async {


Can you explain why this is?

Fixes #154060 The error message doesn't make sense to me since one can call `setState` during the idle phase, and I'm not sure what this is guarding against without the right error message. In the case of #154060 the layout builder was never laid out: ``` â��â��child 1: _RenderLayoutBuilder#7c319 NEEDS-LAYOUT NEEDS-PAINT â�� creator: LayoutBuilder â�� _BodyBuilder â�� MediaQuery â�� â�� LayoutId-[<_ScaffoldSlot.body>] â�� CustomMultiChildLayout â�� â�� _ActionsScope â�� Actions â�� AnimatedBuilder â�� DefaultTextStyle â�� â�� AnimatedDefaultTextStyle â�� _InkFeatures-[GlobalKey#1f6eb ink â�� renderer] â�� NotificationListener<LayoutChangedNotification> â�� â�¯ â�� parentData: offset=Offset(0.0, 0.0); id=_ScaffoldSlot.body â�� constraints: MISSING â�� size: MISSING ``` So #154681 doesn't really fix #154060 since the layout callback cannot be run without a set of valid constraints. Before the `BuildScope` change all `_inDirtyList` flags were unset after the `BuildOwner` finishes rebuilding the widget tree, so `LayoutBuilder._inDirtyLst` is always set to false after a frame even for layout builders that were never laid out. With the `BuildScope` change, `LayoutBuilder` has its own `BuildScope` which is only flushed after LayoutBuilder gets its constraints.

Piinks · 2024-09-24T22:11:58Z

(PR Triage): FYI there are some merge conflicts here. :)

chunhtai

LGTM

chunhtai · 2024-10-03T21:06:13Z

packages/flutter/lib/src/rendering/object.dart

@@ -2446,7 +2446,6 @@ abstract class RenderObject with DiagnosticableTreeMixin implements HitTestTarge
  @pragma('vm:notify-debugger-on-exception')
  void _layoutWithoutResize() {
    assert(_needsLayout);
-    assert(_relayoutBoundary == this);


Instead of remove this, can it add a || this is RenderObjectWithLayoutCallbackMixin

chunhtai · 2024-10-03T21:10:10Z

packages/flutter/lib/src/rendering/object.dart

+    // This ensures that the layout callback will be run even if an ancestor
+    // chooses to not lay out this subtree (for example, OverlayEntries with
+    // `maintainState` set to true).
+    owner?._nodesNeedingLayout.add(this);


yeah, I can't think of a better way to do this. especially we only want to rebuild during layout phase .

Piinks · 2024-10-15T22:09:44Z

(Flyby comment from triage) This has some merge conflicts now.

chunhtai · 2024-11-13T00:10:57Z

@LongCatIsLooong said they will come back to this pr once they finished Full keyboard access pr

…ways-rebuild

LongCatIsLooong · 2025-01-08T00:07:30Z

(requested another review since I remember I spoke with @goderbauer about the issue).

…ways-rebuild

flutter-dashboard · 2025-03-11T22:43:35Z

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-11T23:17:46Z

(Triage) Hey @LongCatIsLooong, is this change still something you'd like to return to?

LongCatIsLooong · 2025-03-11T23:22:24Z

(Triage) Hey @LongCatIsLooong, is this change still something you'd like to return to?

Yeah I'm planning to get to this PR tomorrow

justinmc

LGTM other than the updates and test fixes that this PR needs.

justinmc · 2025-03-21T18:28:34Z

packages/flutter/lib/src/rendering/object.dart

@@ -4108,6 +4107,64 @@ mixin RenderObjectWithChildMixin<ChildType extends RenderObject> on RenderObject
  }
 }

+/// A mixin for [RenderObject] subclasses with a layout callback. The mixin


Nit: Break into a new paragraph after the first sentence here.

justinmc · 2025-03-21T18:33:48Z

packages/flutter/lib/src/rendering/object.dart

+  /// Informs the framework that the layout callback has been updated and must
+  /// be invoked again.


Nit: Consider explaining the difference between this and markNeedsLayout here. Also maybe add a "see also" for this method in the docs of markNeedsLayout?

LongCatIsLooong added 4 commits September 5, 2024 09:38

wip

475dce7

test

3c2b425

test

0594c59

API docs

6ebf281

github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: scrolling Viewports, list views, slivers, etc. labels Sep 5, 2024

scope down

1adbbda

LongCatIsLooong force-pushed the layout-builder-always-rebuild branch from f264ff6 to 1adbbda Compare September 5, 2024 23:02

uncomment

88eadaf

LongCatIsLooong mentioned this pull request Sep 5, 2024

Mark _LayoutBuilderElement as always clean #154694

Merged

9 tasks

This comment was marked as off-topic.

Sign in to view

LongCatIsLooong requested a review from goderbauer September 6, 2024 00:20

LongCatIsLooong marked this pull request as ready for review September 6, 2024 00:20

goderbauer reviewed Sep 6, 2024

View reviewed changes

API docs

7efd84c

chunhtai self-requested a review October 1, 2024 22:14

chunhtai approved these changes Oct 3, 2024

View reviewed changes

Merge remote-tracking branch 'upstream/master' into layout-builder-al…

5786841

…ways-rebuild

LongCatIsLooong force-pushed the layout-builder-always-rebuild branch from b3f906c to 5786841 Compare December 12, 2024 23:46

LongCatIsLooong added 3 commits December 13, 2024 11:21

fix tests

a699c6d

fix tests

966a532

Merge remote-tracking branch 'upstream/master' into layout-builder-al…

6cd8f7d

…ways-rebuild

LongCatIsLooong requested a review from goderbauer January 8, 2025 00:05

Merge remote-tracking branch 'upstream/master' into layout-builder-al…

2b894df

…ways-rebuild

justinmc reviewed Mar 21, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make sure `LayoutBuilder` rebuild in an inactive route #154681

Make sure `LayoutBuilder` rebuild in an inactive route #154681

LongCatIsLooong commented Sep 5, 2024 •

edited

Loading

This comment was marked as off-topic.

goderbauer Sep 6, 2024

goderbauer Sep 6, 2024

LongCatIsLooong Sep 7, 2024

chunhtai Oct 3, 2024

LongCatIsLooong Oct 3, 2024

chunhtai Oct 3, 2024

goderbauer Sep 6, 2024

LongCatIsLooong Sep 7, 2024

goderbauer Sep 9, 2024

goderbauer Sep 6, 2024

Piinks commented Sep 24, 2024

chunhtai left a comment

chunhtai Oct 3, 2024

chunhtai Oct 3, 2024

Piinks commented Oct 15, 2024

chunhtai commented Nov 13, 2024

LongCatIsLooong commented Jan 8, 2025

flutter-dashboard bot commented Mar 11, 2025

Piinks commented Mar 11, 2025

LongCatIsLooong commented Mar 11, 2025

justinmc left a comment

justinmc Mar 21, 2025

justinmc Mar 21, 2025

		// The initial value of this flag must be set to true to prevent the layout
		// callback from being called when the tree has never been laid out.

		/// Informs the framework that the layout callback has been updated and must
		/// be invoked again.

Make sure LayoutBuilder rebuild in an inactive route #154681

Are you sure you want to change the base?

Make sure LayoutBuilder rebuild in an inactive route #154681

Conversation

LongCatIsLooong commented Sep 5, 2024 • edited Loading

Pre-launch Checklist

This comment was marked as off-topic.

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

Piinks commented Sep 24, 2024

chunhtai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Piinks commented Oct 15, 2024

chunhtai commented Nov 13, 2024

LongCatIsLooong commented Jan 8, 2025

flutter-dashboard bot commented Mar 11, 2025

Piinks commented Mar 11, 2025

LongCatIsLooong commented Mar 11, 2025

justinmc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Make sure `LayoutBuilder` rebuild in an inactive route #154681

Make sure `LayoutBuilder` rebuild in an inactive route #154681

LongCatIsLooong commented Sep 5, 2024 •

edited

Loading