Add/remove overlay child RenderObject from the tree in attach/detach

[LongCatIsLooong]

Fixes #180569
Fixes #177693

• Moves _RenderDeferredLayoutBox from Element.activate/Element.deactivate to RenderObject.attach/RenderObject.detach.
• Makes _addDeferredChild/_removeDeferredChild reentrant.

Before this OverlayPortalElement.deactivate removes the overlay child render object from Overlay. This is fine but in OverlayPortalElement.activate the OverlayPortal's render object has not reattach yet so the RO mutation check fails because it couldn't find the ancestor actively doing layout for the overlay child render object, when it tries to re-attach.

Pre-launch Checklist

• I read the Contributor Guide and followed the process outlined there for submitting PRs.
• I read the AI contribution guidelines and understand my responsibilities, or I am not using AI tools.
• 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.

If this change needs to override an active code freeze, provide a comment explaining why. The code freeze workflow can be overridden by code reviewers. See pinned issues for any active code freezes with guidance.

Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. 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.

[gemini-code-assist]

Code Review

This pull request refactors RenderObject mutation checks and OverlayPortal lifecycle management. It introduces _debugClosestMutationRoot to streamline mutation permission logic and replaces boolean flags with a counter in _RenderTheater to handle deferred child updates more robustly. Additionally, _RenderLayoutSurrogateProxyBox now manages its own attachment lifecycle. Feedback includes a potential regression where a field assignment was replaced by an assertion in _OverlayPortalElement.insertRenderObjectChild, along with minor typo corrections in documentation comments.

[gemini-code-assist]

The assignment to renderObject._deferredLayoutChild was removed and replaced with an assert. However, insertRenderObjectChild is the primary point where the parent element receives the child's RenderObject. If this field is not set before this call, the assert will fail. Furthermore, the logic in attach and detach in _RenderLayoutSurrogateProxyBox depends on this field being correctly populated. Unless the assignment was moved to a location not shown in this diff (like a new setter or a different part of the element lifecycle), this appears to be a regression.

      renderObject._deferredLayoutChild = child as _RenderDeferredLayoutBox;
      slot._addChild(child as _RenderDeferredLayoutBox);

[LongCatIsLooong]

Actually the suggested change is more robust if we change the lifecycle of this in the future. But I think doing it during the construction of the render object makes the lifecycle of the variable easier to understand.

[LongCatIsLooong]

added a comment.

[victorsanni]

maybe the comment could be a little more explicit for findability? e.g

// _deferredLayoutChild is assigned in _DeferredLayout.createRenderObject.

[LongCatIsLooong]

This is the same exact logic as before just restructured it a little bit.

[chunhtai]

in OverlayPortalElement.activate the OverlayPortal's render object has not reattach yet so the RO mutation check fails

This seems like will be a problem even if this is first build, how does this not crash every time?

[LongCatIsLooong]

in OverlayPortalElement.activate the OverlayPortal's render object has not reattach yet so the RO mutation check fails

This seems like will be a problem even if this is first build, how does this not crash every time?

activate is not called on mount, it's only called during global key reparenting (if that's what you meant)?

[chunhtai]

This breaks abstracting of attached. If we later update the attached getter to take more thing into account, this may break.

I think we should revert this

[LongCatIsLooong]

Ah you mean someone overrides attached so it becomes something like super.attached && someOtherCondition? That is most likely not legal because the framework assumes that every render object in the tree must be attached to the same owner and every node must be attached when they're in the tree. So basically attached is synonymous with owner != null.

[LongCatIsLooong]

I guess we should mark attached @nonVirtual?

[LongCatIsLooong]

Or do you mean at some point we may change the framework so we can mark the entire tree detached despite the nodes have an owner, or mark the entire tree attached when the nodes don't have an owner?

[chunhtai]

both, but mainly for us.

we can mark the entire tree detached despite the nodes have an owner, or mark the entire tree attached when the nodes don't have an owner?

right

[LongCatIsLooong]

Undid the change. I think still we should add @nonVirtual to attached to make sure it's interchangeable with owner != null, since currently it's illegal to change the definition of attached (the framework assumes attached means owner != null or _owner != null and checks _owner instead of attached most of the time).

[chunhtai]

This is not safe though.

Part of a reason why semantics compiling is hard to refactor is that they made a bunch of assumption based on mix match different property state, and I have to try my best to create a bunch of getter to make it more manageable.

I can totally imagine we later introduce addition logic to the getter that can mess up the things if the code assume _ower != null

[LongCatIsLooong]

attached literally means wether this RenderObject is attached to an owner. Today it has to be synonymous with owner != null or you'll likely get an assert when calling addChild / dropChild (I think it is an intended invariant that all RenderObjects in the tree must be attached). If someone really wants to override the behavior of attached, they should override owner instead (IOW owner is going to be the single source of truth and attached is a convenience getter).

I don't see how we would want to change attached to account for some other flags, but I think we should consider changing the owner getter instead if we ever want to so attached and owner != null still mean the same thing (or introduce new flags to represent the new possible states) to avoid breaking existing code.

[chunhtai]

The case I imagine will be something like

bool get attached {
  if (inSomeWeirdLimboState) {
     return false;
  }
  return owner != null;
}

It is not the first time we have cases where we have to keep render object dangling in certain lifecycle phase without moving it to another lifecycle phase

I think we should consider changing the owner getter

you meant even through renderobject has _owner, but we still return null base on condition? I know we do that in some places, but it always feels a bit weird to me

[LongCatIsLooong]

Ideally we don't have to touch neither attached or owner if we have to introduce a different lifecycle state (like offscreen or something if that's what you meant). The semantics of these two are already broadly accepted and changing them will probably break existing code. We can introduce a new source of truth (e.g., lifecycleState) and have owner and attached both reflect that. I find it confusing if attached needs to account for something that's not owner since the documentation literally says this means if this RO is attached to an owner (unless there is an example of inSomeWeirdLimboState?).

[chunhtai]

_deactivate and _active should probably be named to _attach and _detach

[LongCatIsLooong]

I couldn't come up with better names so I didn't change them. The method calls dropChild so its not regular detach. Added documentation to _deactivate to explain why it is so named.

[LongCatIsLooong]

hmm _detachFromLayoutParent sounds more descriptive than _detach or _deactivate, WDYT?

[LongCatIsLooong]

went with _detachFromLayoutSurrogate to match the class name.

[chunhtai]

this also need a better name

[chunhtai]

activate is not called on mount, it's only called during global key reparenting (if that's what you meant)?

you are right, I forgot this part, so basically global key reparent a opened overlayportal will just crash 100% of the time?

[LongCatIsLooong]

activate is not called on mount, it's only called during global key reparenting (if that's what you meant)?

you are right, I forgot this part, so basically global key reparent a opened overlayportal will just crash 100% of the time?

Not necessarily. The OverlayPortal needs to be in a LayoutBuilder because the mutation checks are only performed during layout. Also the overlay child needs to be marked as dirty after it is detached from the tree (See the test).

[victorsanni]

maybe the comment could be a little more explicit for findability? e.g

// _deferredLayoutChild is assigned in _DeferredLayout.createRenderObject.

[chunhtai]

LGTM, just some question about the code comments

[chunhtai]

should the is activated => is attached?

[LongCatIsLooong]

The only case this can happen is when the element reactivates I think (and this method is not always called on attach, unless _didDetachDeferredChild is true meaning the OverlayPortalElement has been deactivated but the Overlay is not).

[LongCatIsLooong]

Same for _detachFromLayoutSurrogate: it's only called if OverlayPortalElement is deactivating but the Overlay is still in the tree.

[LongCatIsLooong]

I have to admit that I did not consider the case where the Overlay and OverlayPortal are both reparenting (but separately), in which case the detaching/reattaching won't happen. But that should work too because the detach/attach shenanigans are just for maintaining the "every RO in the tree must be attached" invariant and does not have to guarantee the target Overlay is still the overlay we're targeting. When OverlayPortal rebuilds after activation it will move the overlay child to the correct overlay.

[chunhtai]

same deactivates => right before detached

[LongCatIsLooong]

I think deactivate is more accurate (for reasons stated in the comment above).

[victorsanni]

Should #177693 also be added to this PR description so it is tracked here? Not sure if merging this PR will close that issue.

[auto-submit]

autosubmit label was removed for flutter/flutter/186564, because The base commit of the PR is older than 7 days and can not be merged. Please merge the latest changes from the main into this branch and resubmit the PR.

[auto-submit]

auto label is removed for flutter/flutter/186564, Failed to enqueue flutter/flutter/186564 with HTTP 400: Pull request Required status check "Merge Queue Guard" is expected..

[davidhicks980]

Should #187162 also be added to the PR description?