-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Xaml UI Hosting #15223
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Xaml UI Hosting #15223
Conversation
|
Can we verify that we are not loading the Xaml dlls on boot when not using any Xaml controls. |
| } | ||
|
|
||
| static winrt::Microsoft::ReactNative::XamlApplication Current() { | ||
| return s_current; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if you have a XAML app, and then embed a RNW control within the Xaml App? Seems like in that case, we'd want to be using the already existing XamlApp?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's true there can only be one instance of MUX.Application in the process. But unfortunately MUX.Application doesn't let you add more metadata dynamically, so I thought it'd be good to have this type to allow multiple Xaml-based components to coordinate.
I'm not sure offhand the best way to handle the case where the host already has a MUX Application. Can we file an issue to follow up on that later, or is it urgent to solve? Thanks!
| }; | ||
|
|
||
| [webhosthidden] | ||
| interface IXamlControl |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we move this into a Microsoft.ReactNative.Xaml namespace? I'd like the try to keep any Xaml specific stuff out of core to ensure we have nice layering.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
| // Copyright (c) Microsoft Corporation. | ||
| // Licensed under the MIT License. | ||
|
|
||
| namespace Microsoft.ReactNative { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MS.RN.Xaml namespace?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
| // m_providers.push_back(winrt::make_self<XamlMetaDataProvider>().as<winrt::Microsoft::UI::Xaml::Markup::IXamlMetadataProvider>()); | ||
| s_current = *this; | ||
|
|
||
| // TODO: It's probably not a good idea to only load the controls pri file, there are other ones too. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please file a follow-up issue to investigate this one at a later time. It might be an issue when folks want to load a more complex Xaml user control, we might eventually want a way to let the app declare its own .pri file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I understand. Will create a task for follow up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
vnext/src-win/src/private/specs_DEPRECATED/components/Xaml/XamlHostNativeComponent.js
Show resolved
Hide resolved
| // This is similar to ViewComponentView::hitTest, but we don't want to hit test the children of this node, | ||
| // because the child ComponentView is kind of a dummy representation of the XamlElement and doesn't do anything. | ||
| // So, we just hit test the ContentIsland itself to make the UIA ElementProviderFromPoint call work. | ||
| // TODO: Will this cause a problem -- does this function need to do something different for non-UIA scenarios? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious of Andrew's thoughts on the best way to handle ContentIslandComponentView::hitTest
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you need this override? What kinds of components are you putting in the ContentIslandComponentView? I'd expect that the components you'd be putting in the XamlHost view would be Xaml components. And those should not inherit from ViewComponent, so they would be ignored by the default hit test, no?
|
There's an older Xaml sample in sample-custom-component called CalendarView, it'd be nice to delete that one as this goes in. There's also one in playground-composition\CustomComponent.cpp we might want to delete. This is looking fine to me, but I want to make sure we get Andrew's approval. Thanks! |
JesseCol
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will be great to get in, thanks! Please make sure you get Andrew's approval.
Yeah we are not introducing any Xaml dll's. All these come into picture when RegisterXamlHostNativeComponent is triggered from the 3P Module. |
vnext/src-win/src/private/specs_DEPRECATED/components/Xaml/XamlHostNativeComponent.js
Outdated
Show resolved
Hide resolved
| this->ResourceManagerRequested([resourceManager](auto &&, ResourceManagerRequestedEventArgs args) { | ||
| args.CustomResourceManager(resourceManager); | ||
| }); | ||
| winrt::Microsoft::UI::Xaml::Hosting::WindowsXamlManager::InitializeForCurrentThread(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will XamlApplication be instantiated if there are no XamlHost tags used in an RNW app (either directly or indirectly via modules)?
WindowsXamlManager::InitializeForCurrentThread is known to be a slow function. We need to be sure we don't penalize the mainline scenario (RNW apps with no XAML usage) for a special case scenario.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It won't be instantiated. It will only be created when XamlHostComponentView is initialized.
| assert(false); | ||
| base_type::MountChildComponentView(childComponentView, index); | ||
| } | ||
|
|
||
| void ContentIslandComponentView::UnmountChildComponentView( | ||
| const winrt::Microsoft::ReactNative::ComponentView &childComponentView, | ||
| uint32_t index) noexcept { | ||
| assert(false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Were these asserts originally put to ensure control doesn't reach here? More like unreachable code but now we've a genuine islands scenario that have them removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes sundar
| for (const auto &provider : m_providers) { | ||
| auto definitionsCurrentProvider = provider.GetXmlnsDefinitions(); | ||
| for (const auto &definition : definitionsCurrentProvider) { | ||
| allDefinitions.insert(allDefinitions.begin(), definition); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inserting to the front of a vector is better avoided generally as all following elements needs to be reseated (slow). Unless the order of the definitions is important, this will be faster:
for (const auto &provider: m_providers) {
const auto& definitions = provider.GetXmlnsDefinitions();
allDefinitions.insert(allDefinitions.cend(), definitions.cbegin(), definitions.cend());
}| auto uiaProvider = | ||
| winrt::get_self<winrt::Microsoft::ReactNative::implementation::ComponentView>(view)->EnsureUiaProvider(); | ||
|
|
||
| // TODO: Avoid exposing CompositionDynamicAutomationProvider in RootComponentView |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please file a follow-up task for this and paste its link here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#15317 - I already created a task and adding pending sub tasks to it. Will add this here.
Description
Type of Change
Why
We would like to support hosting XAML UI in Fabric.
What
Testing
Manual testing using SampleAppFabric and hardcoded XAML UI in the xamlisland
Changelog
Should this change be included in the release notes: no_
Microsoft Reviewers: Open in CodeFlow