Attach an Element's render object to a designated OverlayEntry #39104

LongCatIsLooong · 2019-08-23T07:19:17Z

Description

This PR adds a new widget OverlayConduit that manages a regular child and an overlayItem child. When mounted, the overlayItem child has the same behavior as the regular child, except that it attaches its render object to the designated overlayEntry. Thus the overlayItem gets its inherited widgets from the current BuildContext, as the regular child does. This also guarantees that when the entire OverlayConduit subtree unmounts, the removal of listeners and invocation of State.dispose will proceed in an orderly fashion: the deepest elements get removed/disposed of first, preventing #35555 from happening.

Classes in this PR, in a typical setting

Related Issues

#36220

Tests

I added the following tests:

packages/flutter/test/widgets/overlay_conduit_test.dart

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I signed the CLA.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I updated/added relevant documentation (doc comments with ///).
All existing and new tests are passing.
The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require Flutter developers to manually update their apps to accommodate your change?

Yes, this is a breaking change (Please read Handling breaking changes). Replace this with a link to the e-mail where you asked for input on this proposed change.
No, this is not a breaking change.

goderbauer

Some comments I noticed during the initial read-through.

goderbauer · 2019-09-12T16:48:19Z

packages/flutter/lib/src/foundation/node.dart

@@ -54,8 +54,8 @@ class AbstractNode {
  @protected
  void redepthChild(AbstractNode child) {
    assert(child.owner == owner);
-    if (child._depth <= _depth) {
-      child._depth = _depth + 1;
+    if (child.depth <= depth) {


Why this change?

the default implementation of redepthChild should respect AbstractNode.depth overrides so the minDepth trick in _RenderFollowerProxyBox works

That will probably have painful performance implications (moves this from a simple jmp to an indirect jump).

goderbauer · 2019-09-12T16:51:01Z

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

@@ -563,6 +564,9 @@ class _TheatreElement extends RenderObjectElement {
  void visitChildren(ElementVisitor visitor) {
    if (_onstage != null)
      visitor(_onstage);
+    if (_offstage == null)


When would you call visitChildren while _offstage is still null? Should this be an assert?

I ran into this issue #37161 when I was poking around. Probably was trying to print the overlay before it was mounted and toString() threw.

let's resolve this in a separate PR

goderbauer · 2019-09-12T16:53:07Z

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

+  final _RenderLeaderProxyBox leaderRenderBox;
+}
+
+/// An [OverlayEntry] that allows an [OverlayConduit] to attach its


This probably needs a better doc comment explaining when and for what this should and shouldn't be used.

goderbauer · 2019-09-12T16:58:14Z

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

+  final GlobalKey _mountPointKey;
+  _PendingRenderObjectManifest _pendingRenderObject;
+
+  @visibleForTesting


Move this annotation to after the comment.

goderbauer · 2019-09-12T17:00:28Z

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

+        && element == other.element;
+  }
+
+  // We're not hashing this anyways.


This comment doesn't seem useful.

goderbauer · 2019-09-12T17:37:08Z

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

+  /// to the designated [ProxyOverlayEntry] [overlayEntry].
+  const OverlayConduit({
+    Key key,
+    this.overlayItem,


Maybe: overlayChild to make it clear that this is the child that goes into the overlay?

goderbauer · 2019-09-12T17:39:38Z

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

+       assert(overlayItem == null || overlayEntry != null),
+       super(key: key);
+
+  /// A widget whose render object will be attached onto [overlayEntry].


A widget that is build like a regular child, but visually appears in the [overlayEntry].

The widget is build in the build context of this widget, but its render object is attached to the overlayEntry.

goderbauer · 2019-09-12T17:42:32Z

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

+}
+
+@immutable
+class _OverlaySlot {


If the slot's identity is only defined by the element - can you just use the element as slot?

_OverlayConduitElement used to have no render object (similar to a ComponentElement) so I added this class to differentiate the overlayChild slot and the child slot. I guess it's safe to get rid of it now.

goderbauer · 2019-09-12T17:45:35Z

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

+  void forgetChild(Element child) {
+    if (child == _child)
+      child = null;
+    assert(child == _overlayItem);


This is strange: The only child we can forget is the overlayItem? What about the child child? Can those not be different?

Or is the if missing a return maybe? (If so, we should add a test that fails without it)

oh it's missing a return. Thanks for catching that!

child = null; didn't know it was possible 👀

goderbauer · 2019-09-12T17:50:01Z

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

+        // If mountPoint is already built.
+        assert(mountPoint.renderObject != null);
+        mountPoint.renderObject.leader = renderObject;
+        mountPoint.insertChildRenderObject(child, slot);


Does this one have multiple slots? Can slot just be null?

Hixie · 2019-10-21T18:05:51Z

@LongCatIsLooong Any chance you can merge this to tip of tree? I tried checking out the branch but I got a lot of merge conflicts.

Hixie · 2019-10-22T20:12:16Z

So I tried this out yesterday and by and large it seems to work as expected (which is impressive! Wow!). However, I think we need to separate out the conduit API from Overlay. Having them be tied too closely leads to all kinds of problems, for example you can't use Positioned in the overlayChild.

The other weird thing about this API is that if you have multiple children you want to move to elsewhere in the tree you need one widget for each one. That took me a while to understand. Once I got the hang of it I was able to work with that easily enough.

Here's the code I wrote to experiment with this: http://junkyard.damowmow.com/606

I think I need to do more experiments, but I'd like to wait to have a version that isn't tied to the Overlay before I do that.

Hixie · 2019-12-09T19:34:32Z

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

+
+import 'framework.dart';
+
+// When an OverlayConduit tries to mount its overlayItem onto its overlayEntry's


looks like the comments need updating

Ahmadre · 2019-12-30T18:12:29Z

any updates? Can we merge this?

Ahmadre · 2019-12-30T22:37:22Z

Oh ok, I see it's just in progress :) gtk

Hixie · 2020-01-07T18:12:54Z

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

+  _RenderLeaderProxyBox([RenderBox child]) : super(child);
+
+  _RenderFollowerProxyBox __follower;
+  _RenderFollowerProxyBox get _follower => __follower;


nit: since the class is private, the member can be public. this lets you avoid the double-underscore thing.

Hixie · 2020-01-07T18:19:29Z

I like this new approach. I need to try playing with it.

Hixie · 2020-01-08T19:16:56Z

@LongCatIsLooong points out for (3) that when the Leader is pushed down, its nodes are (temporarily) removed from the tree which also removes the children from the Follower, so it actually reduces to case (1).

So really all we need to do is make sizedByParent prevent the layout() call from laying out the children immediately.

goderbauer · 2020-02-05T22:53:27Z

Looking at the test failures, this probably requires a rebase to make cirrus happy.

LongCatIsLooong · 2020-02-06T23:07:56Z

@Hixie @goderbauer I tried out the "throw sizedByParent children back to the dirty list after performResize" trick in ec0680107f76148e9e6f7eebb437c19f12e91f86. The new tests are passing but some existing tests started to fail.

It seems some render objects need not only the size of their child but also the child's layout, e.g. render objects that call their child's getDistanceToActualBaseline. In this case we can't defer the child's performLayout even if it's sized by its parent.
Does it make sense to add a flag to RenderObject that indicates whether the parent is allowed to use the child's layout, or add it to RenderObject.layout (so it becomes something like void layout(Constraints constraints, { bool parentUsesSize = false, bool parentUsesLayout = false }))?
A quick search for "implements Render" in the registered tests yielded nothing.

LongCatIsLooong · 2020-02-07T00:07:03Z

If both subtrees are dirty and the Conduit branch lays out first, does making the anchor sizedByParent causes the layout of the remote branch to happen twice (triggered in both subtrees)? I guess we want to make the Conduit node deeper instead?
The current road blocker is when a ConduitAnchor is deferred to the next layout iteration, its descendant render objects that are also in the dirty list will be laid out more than once. I'll see if we can defer these nodes to the next iteration too.

goderbauer · 2020-03-04T23:06:02Z

@LongCatIsLooong Do you think there still is a path forward for this PR? Are you still working on it?

LongCatIsLooong · 2020-03-04T23:32:18Z

Yes I'm still (not very actively) working on this. I think it may still be doable, just need to find an approach that requires less crazy framework changes.

…ject

LongCatIsLooong · 2020-04-20T03:05:30Z

@Hixie @goderbauer all the tests seem to be passing now. Could you take a look? I added a shouldDeferLayout getter and a markDepedentNeedsLayout(RenderObject) method to RenderObject.

chunhtai

For the depthbarrier, if i understand correctly, it is trying to prevent the attached children under conduit anchor to be laid out before the anchor. If that is the intention, have you checked out this https://github.com/rrousselGit/flutter_portal/blob/rework/lib/src/portal.dart?

This is @rrousselGit attempt to deal with this problem and i think it worth considering. The goal is the conduit widget is not responsible for laying out or paint its remote child. It merely attach its remote child to the anchor key. When the conduit anchor layout, it pick up the anchor key and see which children it has and proceed to layout and paint them. The entire discussion can be found in #50961

Besides the comment, I have a general question, It seems like the attempt here is to wrapped the overlay with conduit and conduitanchor to provide declarative overlay api. The portal is trying to achieve the same thing except it replace the overlay with an entire different system Portal. Why do we prefer one over the other?

chunhtai · 2020-05-04T20:43:06Z

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

+  ///
+  /// The [dependent] must be [attached], has the same [PipelineOwner] as this
+  /// [RenderObject], and its [shouldDeferLayout] and [sizedByParent] must be true.
+  /// Additionally To prevent introducing circular dependencies, [dependent]'s


Suggested change

/// Additionally To prevent introducing circular dependencies, [dependent]'s

/// Additionally to prevent introducing circular dependencies, [dependent]'s

chunhtai · 2020-05-04T21:01:00Z

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

+class _RenderConduit extends RenderProxyBox {
+  _RenderConduit([RenderBox child]) : super(child);
+
+  _RenderConduitAnchor _follower;


I was confused by the name. If i read it correctly, shouldn't the Conduit be follower and ConduitAnchor be the leader because conduit attach its child the conduitanchor?

The anchor is named follower in the sense that it depends on its conduit. The anchor changes if its conduit changes, but not the other way around. I guess dependent/dependency sounds good here too.

The problem with naming the anchor follower is that you then have one follower with multiple leaders which is counterintuitive. Usually there should only be one leader with multiple followers. The dependent/dependency sounds good

rrousselGit · 2020-05-04T21:49:19Z

The portal is trying to achieve the same thing except it replace the overlay with an entire different system Portal. Why do we prefer one over the other?

I can answer that question:
Because portal was implemented outside of Flutter. So I was unable to modify Overlay, and instead made a fork.

Although if it were to be merged into Flutter, it could still be valuable to separate them, to help the migration.
Kind of like Router vs Navigator

rrousselGit · 2020-05-04T22:03:23Z

The goal is the conduit widget is not responsible for laying out or paint its remote child.

In the rework branch, the mechanism is slightly different.

The conduit does the layout of the overlay entry (where the constraints are obtained with an object equivalent to LayerLink).
And the Overlay does the painting of the entries
This way it does not rely on having a specific layout order to work (so no situation where the widget may build twice like @Hixie mentioned).

Although I'm not sure if this has undesired side-effects

LongCatIsLooong · 2020-05-06T08:59:11Z

@rrousselGit Thanks for the explanation! I think the current implementation in the rework branch will probably break RenderObject.getTransformTo (and maybe some other things that rely on the hierarchy of the render object tree), nonetheless it looks really promising! It makes more sense to me to paint somewhere else than to move render objects around. 👍

rrousselGit · 2020-05-06T09:12:58Z

@rrousselGit Thanks for the explanation! I think the current implementation in the rework branch will probably break RenderObject.getTransformTo (and maybe some other things that rely on the hierarchy of the render object tree), nonetheless it looks really promising! It makes more sense to me to paint somewhere else than to move render objects around.

It did, but I managed to fix it (it's in the branch)

My biggest worry was about RepaintBoundary & co, wondering if there isn't an edge case where the UI doesn't refresh or something

Haven't found anything so far though.

rrousselGit · 2020-06-19T21:38:31Z

Are you giving up on this?

…

On Fri, Jun 19, 2020, 22:32 LongCatIsLooong ***@***.***> wrote: Closed #39104 <#39104>. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#39104 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AEZ3I3ITSE5M6G6WT4TFIB3RXPKNHANCNFSM4IO4OPUA> .

LongCatIsLooong · 2020-06-19T21:45:00Z

@rrousselGit no I'm just trying to reduce the number of open PRs since I'm currently not working on this.

rrousselGit · 2020-06-24T16:03:38Z

What remains to be done?

Between Hero/package:animation issues, state restoration, and provider having difficulties, I think it's quite important and may continue it myself.

fluttergithubbot added the framework flutter/packages/flutter repository. See also f: labels. label Aug 23, 2019

googlebot added the cla: yes label Aug 23, 2019

LongCatIsLooong mentioned this pull request Aug 29, 2019

Hero inserts widgets After Dispose #35555

Closed

Piinks added the work in progress; do not review label Sep 3, 2019

LongCatIsLooong changed the title ~~WIP Attach an Element's render object to a different tree~~ Attach an Element's render object to a designated OverlayEntry Sep 8, 2019

LongCatIsLooong marked this pull request as ready for review September 8, 2019 08:57

LongCatIsLooong removed the work in progress; do not review label Sep 8, 2019

LongCatIsLooong force-pushed the overlay-render-object branch from 8029459 to 8ca93a6 Compare September 8, 2019 09:53

LongCatIsLooong requested a review from HansMuller September 8, 2019 20:12

goderbauer reviewed Sep 12, 2019

View reviewed changes

LongCatIsLooong requested a review from Hixie September 17, 2019 17:14

LongCatIsLooong mentioned this pull request Oct 11, 2019

Copy-paste popup menu of TextField uses app theme, not the theme of closest surrounding Theme or ButtonTheme widget #42405

Closed

LongCatIsLooong force-pushed the overlay-render-object branch 4 times, most recently from f36d8b6 to 6f2d077 Compare October 27, 2019 07:36

Piinks added a: animation Animation APIs f: routes Navigator, Router, and related APIs. a: text input Entering text in a text field or keyboard related problems a: typography Text rendering, possibly libtxt labels Nov 11, 2019

Hixie reviewed Dec 9, 2019

View reviewed changes

LongCatIsLooong force-pushed the overlay-render-object branch 2 times, most recently from 2cef766 to 0db2ab9 Compare December 17, 2019 06:27

Piinks mentioned this pull request Dec 30, 2019

Hero Widget misbehavior with Text #47984

Closed

Hixie reviewed Jan 7, 2020

View reviewed changes

LongCatIsLooong force-pushed the overlay-render-object branch from 1ebdede to 1c61cb4 Compare February 6, 2020 17:31

LongCatIsLooong changed the title ~~Attach an Element's render object to a designated OverlayEntry~~ [WIP] Attach an Element's render object to a designated OverlayEntry Feb 8, 2020

Piinks added the work in progress; do not review label Feb 19, 2020

Hixie mentioned this pull request Feb 22, 2020

Overlay 2.0 / Declarative OverlayEntry #50961

Closed

LongCatIsLooong force-pushed the overlay-render-object branch from 3e1c937 to 4b6adf1 Compare April 19, 2020 07:39

re-apply diff

af596b1

LongCatIsLooong force-pushed the overlay-render-object branch from 4b6adf1 to af596b1 Compare April 19, 2020 07:42

LongCatIsLooong added 2 commits April 19, 2020 14:57

Merge remote-tracking branch 'upstream/master' into overlay-render-ob…

c264c18

…ject

add missing _needsLayout = true

b12dc75

LongCatIsLooong changed the title ~~[WIP] Attach an Element's render object to a designated OverlayEntry~~ Attach an Element's render object to a designated OverlayEntry Apr 28, 2020

LongCatIsLooong removed the work in progress; do not review label Apr 28, 2020

chunhtai reviewed May 4, 2020

View reviewed changes

review

5ca0af6

LongCatIsLooong closed this Jun 19, 2020

rrousselGit mentioned this pull request Jun 24, 2020

ProviderScope override "forgets" the StreamProvider value during navigation rrousselGit/riverpod#10

Closed

github-actions bot locked as resolved and limited conversation to collaborators Jul 30, 2021


		import 'framework.dart';

		// When an OverlayConduit tries to mount its overlayItem onto its overlayEntry's

	/// Additionally To prevent introducing circular dependencies, [dependent]'s
	/// Additionally to prevent introducing circular dependencies, [dependent]'s

Attach an Element's render object to a designated OverlayEntry #39104

Attach an Element's render object to a designated OverlayEntry #39104

Conversation

LongCatIsLooong commented Aug 23, 2019 • edited

Description

Classes in this PR, in a typical setting

Related Issues

Tests

Checklist

Breaking Change

goderbauer 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

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

LongCatIsLooong Sep 12, 2019 • edited

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

Hixie commented Oct 21, 2019

Hixie commented Oct 22, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ahmadre commented Dec 30, 2019

Ahmadre commented Dec 30, 2019

Choose a reason for hiding this comment

Hixie commented Jan 7, 2020

Hixie commented Jan 8, 2020

goderbauer commented Feb 5, 2020

LongCatIsLooong commented Feb 6, 2020

LongCatIsLooong commented Feb 7, 2020 • edited

goderbauer commented Mar 4, 2020

LongCatIsLooong commented Mar 4, 2020

LongCatIsLooong commented Apr 20, 2020 • edited

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

LongCatIsLooong May 6, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rrousselGit commented May 4, 2020

rrousselGit commented May 4, 2020 • edited

LongCatIsLooong commented May 6, 2020 • edited

rrousselGit commented May 6, 2020

rrousselGit commented Jun 19, 2020 via email

LongCatIsLooong commented Jun 19, 2020

rrousselGit commented Jun 24, 2020

LongCatIsLooong commented Aug 23, 2019 •

edited

LongCatIsLooong Sep 12, 2019 •

edited

LongCatIsLooong commented Feb 7, 2020 •

edited

LongCatIsLooong commented Apr 20, 2020 •

edited

LongCatIsLooong May 6, 2020 •

edited

rrousselGit commented May 4, 2020 •

edited

LongCatIsLooong commented May 6, 2020 •

edited