-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Fix XAML popup positioning and light dismiss in ScrollView (#15557) #15564
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?
Fix XAML popup positioning and light dismiss in ScrollView (#15557) #15564
Conversation
…#15557) - Add SetXamlRoot() API on ContentIslandComponentView for 3rd party XAML components - Add DismissPopups() using VisualTreeHelper.GetOpenPopupsForXamlRoot() - Fire LayoutMetricsChanged on scroll to update popup positions - Dismiss child ContentIsland popups when scroll begins - Add ComboBox sample component demonstrating the pattern - Add xamlPopupBug test sample for playground-composition
sundaramramaswamy
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.
Please address comments.
…#15557) - Set LocalToParentTransformMatrix synchronously before Connect() to fix initial position - Fire LayoutMetricsChanged on scroll to update position after scrolling - Add DismissPopupsRequest event for 3P components to implement light dismiss - Update ComboBox sample to use event-based approach (core remains XAML-agnostic)
Issue microsoft#15557: Fix popup positioning for XAML controls in ScrollView Bug 1 (Position Fix): - Convert getClientRect() physical pixels to DIPs by dividing by pointScaleFactor - LocalToParentTransformMatrix expects logical pixels (DIPs), not physical pixels - Update transform synchronously instead of async to avoid race conditions - Set initial transform in OnMounted() before Connect() Bug 2 (Light Dismiss Fix): - Add DismissPopupsRequest event for 3P components to handle popup dismissal - ScrollView fires event when scroll begins via DismissChildContentIslandPopups() - Core remains XAML-agnostic - 3P components handle their own popups
sundaramramaswamy
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.
Please revert formatting changes.
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 still see a huge red blob of formatting changes. It's hard to review with formatting changes.
f0d7eb5 to
e8cfe99
Compare
| runtimeclass ContentIslandComponentView : ViewComponentView { | ||
| void Connect(Microsoft.UI.Content.ContentIsland contentIsland); | ||
|
|
||
| // Issue #15557: Event fired when a parent ScrollView starts scrolling. |
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.
We should bring this up with the ContentIsland folks. This should be handled by a shared popup manager of some kind. ContentIslands should not need to talk to any RNW specific APIs to properly implement controls.
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.
Possibly InputLightDismissAction should be extended to provide this functionality.
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 sure, we can connect for long term strategy, let me know with whom I can connect to follow up.
| float x = static_cast<float>(clientRect.left) / scaleFactor; | ||
| float y = static_cast<float>(clientRect.top) / scaleFactor; | ||
|
|
||
| m_childSiteLink.LocalToParentTransformMatrix( |
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.
The reason for the post before was to batch notifications. During a layout pass it's likely that a bunch of the parent layouts will change. So, this will end up getting called a bunch of times. Is it pretty cheap call?
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, LocalToParentTransformMatrix is a cheap call - it just sets a 4x4 matrix value.
The async approach caused race conditions where the popup would open with stale transform values (before the async update executed). Synchronous updates ensure the transform is always correct when the popup opens.
Additionally:
- During initial mount,
ParentLayoutChanged()is only called once at the end - During scroll, events are already throttled by the scroll visual
- Added a comment in the code explaining why sync is required
| // This notifies 3P components to dismiss their own popups - implementing light dismiss behavior. | ||
| void ScrollViewComponentView::DismissChildContentIslandPopups() noexcept { | ||
| // Helper lambda to recursively find ContentIslandComponentView children and fire the event | ||
| std::function<void(const winrt::Microsoft::ReactNative::ComponentView &)> fireEventRecursively = |
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 looks like potentially quite expensive work to be doing continuously on scroll. It would probably be better to have the content island register for the event on all its parent scrollviews during its onMount. So, you only have to do the tree walk once.
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, Good point, I re-implemented it:
ContentIslandComponentViewnow registers forScrollBeginDragon all parentScrollViewComponentViewinstances duringOnMounted()(one-time tree walk)ScrollViewComponentViewexposes aScrollBeginDragevent and just fires it when scroll begins- No tree walking on every scroll - registration happens once during mount
- Cleanup happens in
OnUnmounted()
This is the same pattern used for LayoutMetricsChanged registration.
|
Assuming LocalToParentTransformMatrix is pretty cheap, the LocalToParentTransformMatrix changes look good. The light dismiss logic seems more wrong. This looks like we need additional hosting APIs from ContentIslands. ContentIslands should provide APIs to handle this kind of communication, and Xaml / RNW should talk to the new API. Xaml content islands should not have to know about RNW APIs - it breaks the ContentIsland encapsulation. |
|
|
||
| // Issue #15557: Fire DismissPopupsRequest event on child ContentIslandComponentView instances when scroll begins. | ||
| // This notifies 3P components to dismiss their own popups - implementing light dismiss behavior. | ||
| void ScrollViewComponentView::DismissChildContentIslandPopups() noexcept { |
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.
If you have nested ScrollViews, both will fire DismissChildContentIslandPopups() on scroll begin. The inner ScrollView will walk all its children, and the outer ScrollView will also walk all children (including the inner ScrollView's children). Maybve you can consider tracking whether a ContentIsland has already received the dismiss event in this frame to avoid redundant processing.
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 is already addressed with the optimization I just pushed!
With the new implementation:
- ContentIslandComponentView registers with ALL parent ScrollViews during OnMounted() (one-time tree walk up)
- Each ScrollView just fires its ScrollBeginDrag event to registered listeners
- No tree walking on every scroll
- No nested iteration issue - each ContentIsland gets notified once by its direct parent ScrollView subscription
This is the same pattern used for LayoutMetricsChanged.
| // Issue #15557: Fire LayoutMetricsChanged to notify ContentIslandComponentView instances | ||
| // that scroll position has changed, so they can update their LocalToParentTransformMatrix | ||
| // for correct XAML popup positioning. | ||
| void ScrollViewComponentView::FireLayoutMetricsChangedForScrollPositionChange() noexcept { |
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.
The method fires a LayoutMetricsChanged event with identical old and new metrics. This is semantically misleading - receivers of this event might check if oldMetrics != newMetrics and ignore the event.
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.
Valid observation. The LayoutMetricsChanged event with identical old/new metrics is intentionally used to notify descendants that the scroll position changed (not the layout metrics themselves).
ContentIslandComponentView doesn't compare old vs new - it just recalculates its position via getClientRect() which includes the scroll offset. So the identical metrics don't cause any issue.
Alternative would be to add a new ScrollPositionChanged event, but that would be more invasive. The current approach reuses existing infrastructure and works correctly.
Fix XAML popup positioning and light dismiss in ScrollView (#15557)
Description
This PR fixes Issue #15557 - "Pop-ups of Xaml controls need positioning and dismissal"
When XAML controls with popups (like ComboBox, DatePicker, TimePicker) are hosted inside a React Native ScrollView via
ContentIslandComponentView, two bugs occur:Root Cause Analysis
Bug 1: Popup Position
ContentIslandComponentViewusesChildSiteLink.LocalToParentTransformMatrixfor popup positioninggetClientRect()returns values in physical pixels (scaled bypointScaleFactor), butLocalToParentTransformMatrixexpects logical pixels (DIPs)UIDispatcher().Post(), causing race conditions where popup opened with stale transform valuesConnect(), causing wrong position even without scrollingBug 2: Light Dismiss on Scroll
Solution
Bug 1 Fix: Popup Position After Scroll
getClientRect()values bypointScaleFactorbefore settingLocalToParentTransformMatrixUIDispatcher().Post()wrapper - update transform immediatelyLocalToParentTransformMatrixinOnMounted()beforeConnect()ScrollViewComponentViewfiresLayoutMetricsChangedevent when scroll position changesBug 2 Fix: Light Dismiss on Scroll (Optimized)
DismissPopupsRequestevent toContentIslandComponentViewScrollBeginDragevent toScrollViewComponentViewOnMounted(),ContentIslandComponentViewwalks up the tree once and registers forScrollBeginDragon all parentScrollViewComponentViewinstancesScrollViewComponentViewfires itsScrollBeginDragevent, which notifies only the registeredContentIslandComponentViewinstancesDismissPopupsRequestand dismiss their own popupsFiles Changed
Core Fix (vnext/Microsoft.ReactNative/)
Fabric/Composition/ContentIslandComponentView.cpp- DIP conversion, sync updates, register for parent ScrollView events during mountFabric/Composition/ContentIslandComponentView.h- Store scroll subscriptionsFabric/Composition/ScrollViewComponentView.cpp- Fire LayoutMetricsChanged on scroll, expose ScrollBeginDrag eventFabric/Composition/ScrollViewComponentView.h- ScrollBeginDrag eventCompositionComponentView.idl- Added DismissPopupsRequest event to ContentIslandComponentView, ScrollBeginDrag event to ScrollViewComponentViewTesting
XamlPopupReprosample in Playground##Screenshot
testingFix.mp4