-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Overflow implementation in Fabric as per Parity to Paper #15338
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
Overflow implementation in Fabric as per Parity to Paper #15338
Conversation
|
@Nitin-100 |
5603646 to
8527924
Compare
8527924 to
85b860f
Compare
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
We already use a clip for rounded corners to clip the background: ComponentView::updateClippingPath. Really we need to stop doing that for rounded corners, which means fixing our background logic for rounded corners so that that clip is not required. Then we can do this change. |
acoates-ms
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.
Need to work out how to not conflict with the existing clip.
|
Looks like the prop is not working as expected as per the screenshots attached. Please add E2ETestApp unit tests as well and more examples in playground. |
Changes are not added yet, I'm busy with release work. |
| layoutMetrics.frame.size.height * layoutMetrics.pointScaleFactor}); | ||
|
|
||
| // Apply overflow clipping | ||
| if (m_props && m_props->yogaStyle.overflow() == facebook::yoga::Overflow::Hidden) { |
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 about other overflow types?
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.
getClipsContentToBounds check this
| winrt::Microsoft::ReactNative::Composition::Experimental::MicrosoftCompositionContextHelper::InnerVisual( | ||
| Visual()); | ||
| if (compVisual) { | ||
| compVisual.Clip(nullptr); |
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 as good as doing nothing, right?
If no clip was ever applied, this line is redundant.
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.
Re-review the code.
Sure, please convert this PR to draft while you work on it. |
Hi OSG Instrumentation Team, I'm working on React Native Windows telemetry. We currently have: ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ EXISTING (working): - JavaScript CLI telemetry via 1DS SDK - Instrumentation Key: 49ff6d3ef12f4578a7b75a2573d9dba8-026332b2-2d50-452f-ad0d-50f921c97a9d-7145 - Data flows to: Aria → Kusto - Kusto cluster: [YOU NEED TO TELL ME THIS] NEW (want to add): - Native C++ telemetry via ETW/TraceLogging - Provider Name: Microsoft.ReactNativeWindows.Telemetry - Provider GUID: [WILL GENERATE] ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ QUESTION: Can you register the new ETW provider GUID and route it to the SAME Aria tenant as our existing 1DS instrumentation key? Goal: Have both JavaScript (1DS) and C++ (ETW) telemetry appear in the same Kusto database for unified querying. Is this possible? If yes, what info do you need from me? Thanks, Harini Malothu React Native Windows Team
@acoates-ms Please re review it as I have handled all scenarios now. |
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.
LGTM.
vnext/Microsoft.ReactNative/Fabric/Composition/CompositionViewComponentView.cpp
Outdated
Show resolved
Hide resolved
|
@acoates-ms |
…idden - Remove ensureContentVisual() calls from VisualToMountChildrenInto() and MountChildComponentView() - m_contentVisual is now only created in updateLayoutMetrics when getClipsContentToBounds() is true - Add null check in onThemeChanged() before accessing Visual() - Restore original ensureVisual() call in MountChildComponentView()
…ontainer
Implement proper overflow:hidden clipping for the Fabric Composition renderer,
matching iOS/Android behavior. Instead of clipping m_visual directly (which
also clips borders and background), a dedicated m_childrenContainer visual is
created on-demand as a child of m_visual, and only children are clipped.
Visual Tree (with overflow:hidden):
m_outerVisual
m_visual (background, opacity, transform, border-radius clip)
Border Visuals x N
m_childrenContainer (created on demand, clipped to bounds)
<children>
Key design decisions:
- m_childrenContainer is created lazily only when overflow:hidden is set
- Existing children are moved from m_visual into m_childrenContainer
- Once created, m_childrenContainer is never destroyed (clip is just removed
if overflow changes back to visible)
- Clipping uses outer border radii via D2D path geometry, matching iOS
- ScrollView overrides updateChildrenClippingPath as no-op since it manages
its own visual tree via m_scrollVisual and inherently clips content
Also cleans up:
- Removed unnecessary null-checks on Visual() (ensureVisual guarantees it)
- Removed shadow mask workaround code (not related to overflow)
- Removed InsetClip-based overflow handling from updateProps
- Added assert(m_visual) in Visual() accessor
- Simplified updateClippingPath to only handle border-radius clipping
vnext/Microsoft.ReactNative/Fabric/Composition/CompositionViewComponentView.cpp
Outdated
Show resolved
Hide resolved
vnext/Microsoft.ReactNative/Fabric/Composition/CompositionViewComponentView.cpp
Show resolved
Hide resolved
vnext/Microsoft.ReactNative/Fabric/Composition/CompositionViewComponentView.cpp
Outdated
Show resolved
Hide resolved
This reverts commit 5e07f75.

Desciption
On iOS/Android,
overflow: 'hidden'clips child content to the parent view's bounds. On React Native Windows (Fabric), this was not working — children always rendered beyond their parent regardless of the overflow property.Additionally, shadow (
shadowColor,shadowOffset, etc.) was being clipped by the border-radius clip onVisual(), making shadows invisible on rounded-corner views.How
m_childrenContainer— on-demand child clipping visualA dedicated
m_childrenContainerSpriteVisual is created lazily (only whengetClipsContentToBounds()is true) as a child ofm_visual. Children are mounted into it and clipped via a D2D rounded-rect path geometry.Visual tree with
overflow: hidden:Visual tree with
overflow: visible(default):Shadow on
OuterVisual()Shadow props are now applied to
OuterVisual()instead ofVisual(). This prevents the border-radius clip (onVisual()) from clipping the shadow.RelativeSizeWithOffsetfor container sizingm_childrenContainerusesRelativeSizeWithOffset({0,0}, {1,1})so it automatically tracks the parent's size without manual size updates.Files Changed
CompositionViewComponentView.cppupdateChildrenClippingPath(),VisualToMountChildrenInto(), child mount/unmount routing,applyShadowProps()→OuterVisual(),updateClippingPath()simplified to border-radius onlyCompositionViewComponentView.hm_childrenContainer,updateChildrenClippingPath()virtual methodScrollViewComponentView.cppupdateChildrenClippingPath()as no-op (scroll view clips viam_scrollVisual)ScrollViewComponentView.hKey Design Decisions
Lazy creation —
m_childrenContaineronly created whenoverflow: hidden/scroll. Views withoverflow: visible(default) have zero overhead.Child migration — When container is created, existing child visuals are moved from
m_visualintom_childrenContainer. New children route throughVisualToMountChildrenInto().Border offset skip — When children are in
m_childrenContainer,indexOffsetForBorder()is skipped (children start at index 0 in the container). Applied only when children are directly inm_visual.Once created, never destroyed — If overflow toggles from hidden→visible,
m_childrenContainerstays but its clipping path is set tonullptr. Avoids complex child migration back.Outer border radii for clip geometry — Matches iOS behavior. Clip path uses
resolveAndAlignBorderMetrics+GenerateRoundedRectPathGeometry.ScrollView no-op —
ScrollViewComponentViewoverridesupdateChildrenClippingPath()as no-op because it mounts children intom_scrollVisualwhich inherently clips.Testing
overflow: hiddenclips children at parent bounds (rect + rounded)overflow: visibleallows children to extend beyond boundsoverflow: hiddenChangelog
react-native-windows — Added
Implemented
overflow: hiddensupport for Fabric Composition renderer. Views withoverflow: 'hidden'now clip children to bounds, matching iOS/Android. Shadow rendering moved toOuterVisualto prevent border-radius clipping.