Skip to content

Conversation

@rozele
Copy link
Contributor

@rozele rozele commented Jul 28, 2021

There are a number of places where a single root view assumption exists in the app. Currently, there is no way to specifiy the correct XamlRoot for FlyoutViewManager. This is a minimal change to add rootTag to shadow node, add an API to the NativeUIManager to get a specific XamlRoot for a ReactRootView, and update FlyoutViewManager to grab the specific XamlRoot for the root view it lives under.

Fixes #8250
Fixes #6714

Microsoft Reviewers: Open in CodeFlow

There are a number of places where a single root view assumption exists
in the app. Currently, there is no way to know which root view a Flyout
will be shown on, which can be problematic when each root is on a
different window. This is the first step to supporting multiple root
views.

Towards microsoft#8250
@rozele rozele requested a review from a team as a code owner July 28, 2021 17:12
@rozele
Copy link
Contributor Author

rozele commented Jul 28, 2021

We may not need the change to CreateViewCore actually...

Copy link
Contributor Author

@rozele rozele left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My main concern with this is it's 8 additional bytes per shadow node for one very specific scenario.

Perhaps a better approach is to wire the rootTag from the UIManager.createView method (as had done previously then reverted) to avoid the memory penalty.

If no one else is opposed to the 8 additional bytes, then we can push this as is though ;)

@rozele rozele changed the title Add root tag to CreateView in ViewManagerBase Add root tag to ShadowNode for resolving correct XamlRoot in FlyoutViewManager Jul 28, 2021
@rozele
Copy link
Contributor Author

rozele commented Jul 28, 2021

Looks like the build failure is due to some flakiness with nuget.

@NickGerleman
Copy link
Contributor

NickGerleman commented Jul 29, 2021

My main concern with this is it's 8 additional bytes per shadow node for one very specific scenario.

Perhaps a better approach is to wire the rootTag from the UIManager.createView method (as had done previously then reverted) to avoid the memory penalty.

If no one else is opposed to the 8 additional bytes, then we can push this as is though ;)

I might be misinterpreting the code, but would be be possible to traverse parents to discover root tag?

@rozele
Copy link
Contributor Author

rozele commented Jul 29, 2021

I might be misinterpreting the code, but would be be possible to traverse parents to discover root tag?

I don't think so. I believe we can potentially show the Flyout at the time createView is called, at which point it's not parented in the ShadowNode hierarchy.

Specifically, we call ShowAt from this point in updateProperties:

which can be called from createView here: and unfortunately, there aren't really many good callbacks to know when a node has been parented. We've used ViewManagerBase::OnLayout in some instances, but this call is too frequent and requires extra state to know that you only want to do something the first time OnLayout is called.

@NickGerleman
Copy link
Contributor

NickGerleman commented Aug 3, 2021

Apologies for not taking a look at this sooner. Have been juggling a couple different things atm.

I think I better understand the problem. I think embedding root tag into shadow node is reasonable if we need to perform actions when not yet parented. It feels like a lot of state to duplicate though.

You had mentioned there weren't good signals of when the shadow node is parented. I think it would be reasonable to expose that. E.g. calling shadownode "onParented" callback in UIManager setChildren or manageChildren.

@NickGerleman
Copy link
Contributor

Hmm... Thinking about that more, it's possible a shadownode could be parented to a different unparanted shadow node...

Tracking root in Shadownode seems reasonable to me then.

@NickGerleman NickGerleman merged commit 8c3f1eb into microsoft:main Aug 3, 2021
@asklar
Copy link
Member

asklar commented Aug 3, 2021

IIRC, we were still discussing how modules can do this in an ABI safe way (though XamlUIService or similar). Do we have a tracking issue for that or do we need one? @rozele

@rozele
Copy link
Contributor Author

rozele commented Aug 3, 2021

IIRC, we were still discussing how modules can do this in an ABI safe way (though XamlUIService or similar). Do we have a tracking issue for that or do we need one? @rozele

Apologies, I had originally planned to include ABI changes for root tag, but decided it was probably better to solve the FlyoutViewManager first as this was our most immediate need. I re-opened #8250 which was intended to cover that scenario.

rozele added a commit to rozele/react-native-windows that referenced this pull request Oct 6, 2022
In microsoft#8327, we added a root tag field to the ShadowNode. Rather than
relying on a parent traversal to find the parent node, we can just
directly resolve the root using the root tag.
rozele added a commit to rozele/react-native-windows that referenced this pull request Oct 6, 2022
In microsoft#8327, we added a root tag field to the ShadowNode. Rather than
relying on a parent traversal to find the parent node, we can just
directly resolve the root using the root tag.
rozele added a commit to rozele/react-native-windows that referenced this pull request Oct 6, 2022
In microsoft#8327, we added a root tag field to the ShadowNode. Rather than
relying on a parent traversal to find the parent node, we can just
directly resolve the root using the root tag.
chiaramooney pushed a commit that referenced this pull request Nov 3, 2022
* Simplify root node lookups

In #8327, we added a root tag field to the ShadowNode. Rather than
relying on a parent traversal to find the parent node, we can just
directly resolve the root using the root tag.

* Change files
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add interface to pass root tag to IViewManager Flyout without target in Xaml Islands crashes app

3 participants