From 32f0852b62e9c7d756b3a566ba706db71ad12b1d Mon Sep 17 00:00:00 2001 From: Collin Date: Sat, 11 Jul 2026 11:54:03 +0800 Subject: [PATCH 1/4] fix: don't clobber inherited fill on stroke-only shapes extractFill emitted the black spec-default `fill` for every element without an explicit `fill`, but did not add 'fill' to `propList`. On platforms whose native renderer applies the emitted value regardless of propList (react-native-windows), this overrode fill inherited from an ancestor - so a stroke-only under a root was painted solid black instead of unfilled. Only emit `fill` when it is actually set on the element. When it is unset we now emit nothing and leave it out of propList, so the native side inherits fill from the parent. A genuinely-unset root still defaults to black via the native node's own default fill, preserving SVG spec behavior. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/lib/extract/extractFill.ts | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/src/lib/extract/extractFill.ts b/src/lib/extract/extractFill.ts index bdc00addd..0b5a8b5b0 100644 --- a/src/lib/extract/extractFill.ts +++ b/src/lib/extract/extractFill.ts @@ -20,10 +20,15 @@ export default function extractFill( inherited.push('fill'); o.fill = !fill && typeof fill !== 'number' ? defaultFill : extractBrush(fill); - } else { - // we want the default value of fill to be black to match the spec - o.fill = defaultFill; } + // When `fill` is not set on this element we deliberately emit no `fill` value + // and do not add it to `propList`, so the native side inherits fill from the + // parent (e.g. a root ). Previously we emitted the black + // spec-default here even though 'fill' was absent from propList; on platforms + // that apply the emitted value unconditionally (react-native-windows) this + // clobbered inheritance and painted a black fill on stroke-only shapes. The + // SVG black default is still honored for a genuinely unset root because the + // native node's own default fill is black. if (fillOpacity != null) { inherited.push('fillOpacity'); o.fillOpacity = extractOpacity(fillOpacity); From b2ea9df7455ed772c171dabd0761a65351c859c8 Mon Sep 17 00:00:00 2001 From: Collin Date: Sat, 11 Jul 2026 21:53:09 +0800 Subject: [PATCH 2/4] fix(windows): resolve inherited fill on Direct2D SVG DOM when unset Part 2 (native) of the fill-inheritance fix: extractFill.ts (previous commit) stops emitting a black default `fill` for elements that never declared one and leaves `fill` out of `propList`, so the intent is that native inherits fill from the parent (e.g. a root ). On react-native-windows this was not honored and a stroke-only under still painted a solid black disk. Two distinct native gaps had to be closed, both in windows/RNSVG/Fabric/: 1) SvgView.cpp (SvgView::Draw): the SvgView's own direct children were recursed into via RecurseRenderNode WITHOUT ever calling their Render()/OnRender(), so that level's D2D element was never created and its own attributes were never applied anywhere in the tree. Svg.tsx wraps every 's children in a that carries 's own presentation props (including fill="none"), so this dropped the root fill on the floor entirely - there was nothing in the D2D SVG DOM for a stroke-only descendant to inherit "none" from. Fixed by calling renderable->Render(...) for each direct child before recursing, exactly as RecurseRenderNode already does at every other level. 2) RenderableView.cpp (RenderableView::Render): even with the root fill now in the tree, D2D does not retroactively cascade an unset presentation attribute onto a child. The tree is rebuilt from scratch via CreateChild()/ SetAttributeValue() every frame (SvgView::Draw makes a new ID2D1SvgDocument on each invalidation) rather than parsed from a full document, so an element with `fill` unset (SetCommonSvgProps only sets attributes present in propList) resolves to the SVG spec's black initial value instead of the parent's computed value. Direct2D's paint model (D2D1_SVG_PAINT_TYPE) has no INHERIT member, so the literal string "inherit" via SetAttributeValue is not meaningful for paint attributes (verified: a first attempt setting "inherit" left the disk black). Instead, after OnRender() we check IsAttributeSpecified("fill") on the freshly created element; if unset, we read the PARENT element's already-resolved ID2D1SvgPaint and copy its paint type/color/id onto the child's own paint. Parents are always rendered before their children (SvgView::RecurseRenderNode), so by induction this propagates the nearest explicit ancestor's fill all the way down and bottoms out at the SVG spec's black default when no ancestor - including the document root, which never has `fill` set - declared one either. Both fixes are required: (1) alone puts the root fill in the tree but children still don't inherit it; (2) alone has nothing to inherit from. Together they make propList-driven fill inheritance work on RNW as it already does on iOS/Android. Verified end-to-end in a production RNW 0.83.2 new-arch app (Facilitron FIT, Windows 11 ARM64, Debug, 250% display scale, RNSVG built from source): a stroke-only under now renders as an unfilled ring (stroke only, no black core), matching an explicit fill="none" control, and normal filled SVGs (app logo, header/tab-bar vector icons) are unaffected. Co-Authored-By: Claude Opus 4.8 (1M context) --- windows/RNSVG/Fabric/RenderableView.cpp | 43 +++++++++++++++++++++++++ windows/RNSVG/Fabric/SvgView.cpp | 20 ++++++++++-- 2 files changed, 60 insertions(+), 3 deletions(-) diff --git a/windows/RNSVG/Fabric/RenderableView.cpp b/windows/RNSVG/Fabric/RenderableView.cpp index fb6a70ceb..6a6467364 100644 --- a/windows/RNSVG/Fabric/RenderableView.cpp +++ b/windows/RNSVG/Fabric/RenderableView.cpp @@ -158,6 +158,49 @@ void RenderableView::FinalizeUpates( ID2D1SvgElement &RenderableView::Render(const SvgView &svgView, ID2D1SvgDocument& document, ID2D1SvgElement &svgElement) noexcept { svgElement.CreateChild(GetSvgElementName(), m_spD2DSvgElement.put()); OnRender(svgView, document, *m_spD2DSvgElement); + + // extractFill.ts (JS) intentionally leaves `fill` unset and out of `propList` when an + // element never declares `fill`, so that native inherits it from the parent (e.g. a root + // ). SetCommonSvgProps only calls SetAttributeValue for attributes present + // in propList, so OnRender above leaves this freshly created D2D SVG element's `fill` + // attribute completely unset in that case. Because this tree is rebuilt from scratch via + // CreateChild()/SetAttributeValue() every frame (SvgView::Draw creates a new + // ID2D1SvgDocument on every Invalidate) rather than parsed from a full document, D2D + // does not retroactively cascade an unset presentation attribute from its parent - it + // resolves to the SVG spec's black initial paint instead, reproducing the exact bug the JS + // fix was meant to eliminate. Direct2D's paint model (D2D1_SVG_PAINT_TYPE) has no INHERIT + // value, so setting the literal string "inherit" via SetAttributeValue is not meaningful for + // `fill`/`stroke` - we resolve inheritance ourselves instead, by copying the parent + // element's already-resolved `fill` paint onto this element. Parent nodes are always + // rendered (and thus resolved by this same logic) before their children in + // SvgView::RecurseRenderNode, so by induction this correctly propagates the nearest + // explicit ancestor's fill all the way down, and bottoms out at the SVG spec's black + // default when no ancestor - including the document root, which never has `fill` set (see + // SvgView::Draw) - ever declared one either. + if (!m_spD2DSvgElement->IsAttributeSpecified(SvgStrings::fillAttributeName, nullptr)) { + winrt::com_ptr parentPaint; + winrt::com_ptr childPaint; + if (SUCCEEDED(svgElement.GetAttributeValue(SvgStrings::fillAttributeName, parentPaint.put())) && parentPaint && + SUCCEEDED(m_spD2DSvgElement->GetAttributeValue(SvgStrings::fillAttributeName, childPaint.put())) && childPaint) { + D2D1_SVG_PAINT_TYPE paintType = parentPaint->GetPaintType(); + childPaint->SetPaintType(paintType); + if (paintType == D2D1_SVG_PAINT_TYPE_COLOR) { + D2D1_COLOR_F color; + parentPaint->GetColor(&color); + childPaint->SetColor(color); + } else if ( + paintType == D2D1_SVG_PAINT_TYPE_URI || paintType == D2D1_SVG_PAINT_TYPE_URI_NONE || + paintType == D2D1_SVG_PAINT_TYPE_URI_COLOR || paintType == D2D1_SVG_PAINT_TYPE_URI_CURRENT_COLOR) { + UINT32 idLen = parentPaint->GetIdLength(); + if (idLen > 0) { + std::wstring id(idLen, L'\0'); + parentPaint->GetId(&id[0], idLen + 1); + childPaint->SetId(id.c_str()); + } + } + } + } + return *m_spD2DSvgElement; } diff --git a/windows/RNSVG/Fabric/SvgView.cpp b/windows/RNSVG/Fabric/SvgView.cpp index 1f1f4080f..6376d1361 100644 --- a/windows/RNSVG/Fabric/SvgView.cpp +++ b/windows/RNSVG/Fabric/SvgView.cpp @@ -281,10 +281,24 @@ void SvgView::Draw( spRoot->SetAttributeValue(SvgStrings::preserveAspectRatioAttributeName, preserveAspectRatio); } + // Render the SvgView's own direct children (e.g. the JS-side that Svg.tsx wraps + // around every 's children, carrying 's own presentation props such as + // fill="none") against the document root, then recurse into their children. + // + // This previously skipped straight into RecurseRenderNode(this, child, ...) *without* + // ever calling `child`'s own Render()/OnRender() - so this level's D2D element was never + // created and none of its own attributes (including `fill`) were ever applied anywhere + // in the D2D SVG DOM. Concretely: a root forwards fill="none" to that + // wrapper , but the wrapper 's own D2D element was never materialized, so + // there was nothing in the tree for a stroke-only descendant to inherit "none" from - + // it fell through to the SVG spec's black default regardless of any propList/inheritance + // fix on the descendant's own side (see RenderableView::Render). Calling Render() here, + // exactly like RecurseRenderNode does for every other level, fixes that. for (auto const &child : view.Children()) { - auto renderable = child.UserData().as(); - if (renderable->IsSupported()) { - RecurseRenderNode(this, child, *spSvgDocument, *spRoot); + auto renderable = child.UserData().try_as(); + if (renderable && renderable->IsSupported()) { + ID2D1SvgElement &newElement = renderable->Render(*this, *spSvgDocument, *spRoot); + RecurseRenderNode(this, child, *spSvgDocument, newElement); } } From 2413c909da071cce842fb4157d16537223063436 Mon Sep 17 00:00:00 2001 From: Collin Date: Sun, 12 Jul 2026 21:06:04 +0800 Subject: [PATCH 3/4] fix(windows): guard against rendering a child before its props are set The child-render added in the previous commit can run during the onMounted cascade before Fabric's prop-update pass reaches a freshly-mounted node, so RenderableView::m_props is null and get_self() yields an invalid pointer -> AV in SetCommonSvgProps' Color compare (repro: mount/remount of an SVG subtree; observed on a responsive layout switch and some cold launches). Add RenderableView::HasProps() and gate both render sites in SvgView.cpp on it (mirroring IsSupported()); a not-yet-propped node is skipped this Draw pass and redrawn by its own pending prop update's Invalidate(). Validated in a production RNW 0.83.2 app: survives repeated layout-switch remounts, ring intact. --- windows/RNSVG/Fabric/RenderableView.h | 13 +++++++++++++ windows/RNSVG/Fabric/SvgView.cpp | 4 ++-- 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/windows/RNSVG/Fabric/RenderableView.h b/windows/RNSVG/Fabric/RenderableView.h index f48326cef..b4040b533 100644 --- a/windows/RNSVG/Fabric/RenderableView.h +++ b/windows/RNSVG/Fabric/RenderableView.h @@ -143,6 +143,19 @@ struct __declspec(uuid("a03986c0-b06e-4fb8-a86e-16fcc47b2f31")) RenderableView : virtual void OnRender(const SvgView &svgView, ID2D1SvgDocument &document, ID2D1SvgElement & /*svgElement*/) noexcept; virtual bool IsSupported() const noexcept; + // True once UpdateProps has been called at least once for this node. SvgView::Draw and + // RecurseRenderNode call Render() on freshly-mounted children as soon as they appear in the + // ComponentView tree, which can race ahead of Fabric's own prop-update pass for a brand-new + // node created in the same mount (e.g. a responsive breakpoint swapping in a new SVG tree). + // Render()/OnRender() dereference m_props (via winrt::get_self) unconditionally, so + // calling them before UpdateProps has ever run reads through a null-backed props object and + // crashes. Skipping such a node for this pass is safe: the in-flight prop update triggers its + // own Invalidate() (RenderableView::UpdateProps's caller chain -> FinalizeUpates -> + // Invalidate) which redraws with props present moments later. + bool HasProps() const noexcept { + return static_cast(m_props); + } + void Invalidate(const winrt::Microsoft::ReactNative::ComponentView &view); protected: diff --git a/windows/RNSVG/Fabric/SvgView.cpp b/windows/RNSVG/Fabric/SvgView.cpp index 6376d1361..d7fa0a986 100644 --- a/windows/RNSVG/Fabric/SvgView.cpp +++ b/windows/RNSVG/Fabric/SvgView.cpp @@ -226,7 +226,7 @@ void RecurseRenderNode( { auto renderable = child.UserData().try_as(); - if (renderable && renderable->IsSupported()) { + if (renderable && renderable->IsSupported() && renderable->HasProps()) { ID2D1SvgElement &newElement = renderable->Render(*root, document, svgElement); RecurseRenderNode(root, child, document, newElement); } @@ -296,7 +296,7 @@ void SvgView::Draw( // exactly like RecurseRenderNode does for every other level, fixes that. for (auto const &child : view.Children()) { auto renderable = child.UserData().try_as(); - if (renderable && renderable->IsSupported()) { + if (renderable && renderable->IsSupported() && renderable->HasProps()) { ID2D1SvgElement &newElement = renderable->Render(*this, *spSvgDocument, *spRoot); RecurseRenderNode(this, child, *spSvgDocument, newElement); } From 0730188706cca6455a725128fead9c12719c51c4 Mon Sep 17 00:00:00 2001 From: Collin Date: Mon, 13 Jul 2026 00:47:27 +0800 Subject: [PATCH 4/4] fix(windows): narrow the null-props guard so it never drops a subtree MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous mount guard skipped Render()/CreateChild() for a node whose m_props isn't set yet in SvgView::Draw / RecurseRenderNode — but Svg.tsx wraps every 's children in an implicit outer that is structurally propless at Draw time, so that skip silently dropped the group's entire subtree (rotated groups + colored circles that DID have props) -> blank/partial SVGs. Move the guard from a subtree-wide skip to a narrow guard around only the OnRender() call in RenderableView::Render (OnRender is the sole m_props dereference). CreateChild() + recursion always run, so no subtree is ever dropped while the crash fix is preserved. Verified in a production RNW 0.83.2 app: dashboard SVG donut renders all segments; no crash across repeated mount/remount (layout switches). --- windows/RNSVG/Fabric/RenderableView.cpp | 26 ++++++++++++++++++++++++- windows/RNSVG/Fabric/RenderableView.h | 23 +++++++++++++--------- windows/RNSVG/Fabric/SvgView.cpp | 11 +++++++++-- 3 files changed, 48 insertions(+), 12 deletions(-) diff --git a/windows/RNSVG/Fabric/RenderableView.cpp b/windows/RNSVG/Fabric/RenderableView.cpp index 6a6467364..575bf7615 100644 --- a/windows/RNSVG/Fabric/RenderableView.cpp +++ b/windows/RNSVG/Fabric/RenderableView.cpp @@ -157,7 +157,31 @@ void RenderableView::FinalizeUpates( ID2D1SvgElement &RenderableView::Render(const SvgView &svgView, ID2D1SvgDocument& document, ID2D1SvgElement &svgElement) noexcept { svgElement.CreateChild(GetSvgElementName(), m_spD2DSvgElement.put()); - OnRender(svgView, document, *m_spD2DSvgElement); + + // HasProps() is false for a node whose Fabric prop-update pass (UpdateProps) hasn't run yet. + // OnRender() (and every override: GroupView, CircleView, EllipseView, RectView, LineView, + // PathView, UseView, ClipPathView, DefsView, ImageView, Linear/RadialGradientView) starts + // with winrt::get_self(m_props), an unchecked cast that's only safe once m_props is + // non-null -- calling it earlier is what used to crash (get_self(null) at + // +0x1d0, AV in the Color compare inside SetCommonSvgProps; see RenderableView.h's HasProps() + // doc comment and git history for the fix that first introduced this check). + // + // That original fix gated Render() itself (skipped in SvgView.cpp's RecurseRenderNode / Draw + // before ever reaching here), which is crash-safe but also skips CreateChild() -- so this + // node's D2D element never exists, RecurseRenderNode has nothing to recurse into, and this + // node's *entire subtree* silently drops out of the Draw() pass, including any children that + // already do have their own props set. Narrowing the guard to just this call preserves the + // exact same crash safety (OnRender(), the only thing that dereferences m_props, is still + // never reached while m_props is null) while guaranteeing this node's own D2D element is + // always created and its children are always walked -- so one still-propless node can no + // longer take an already-ready descendant down with it. A still-propless node simply renders + // as an attribute-less element for this one pass (falls through to the parent-inherited fill + // below, never black; no stroke/geometry of its own yet) and repaints correctly on the very + // next Draw(), which is guaranteed once its own UpdateProps lands (RenderableView::FinalizeUpates + // -> Invalidate()). + if (HasProps()) { + OnRender(svgView, document, *m_spD2DSvgElement); + } // extractFill.ts (JS) intentionally leaves `fill` unset and out of `propList` when an // element never declares `fill`, so that native inherits it from the parent (e.g. a root diff --git a/windows/RNSVG/Fabric/RenderableView.h b/windows/RNSVG/Fabric/RenderableView.h index b4040b533..3084ab3a7 100644 --- a/windows/RNSVG/Fabric/RenderableView.h +++ b/windows/RNSVG/Fabric/RenderableView.h @@ -143,15 +143,20 @@ struct __declspec(uuid("a03986c0-b06e-4fb8-a86e-16fcc47b2f31")) RenderableView : virtual void OnRender(const SvgView &svgView, ID2D1SvgDocument &document, ID2D1SvgElement & /*svgElement*/) noexcept; virtual bool IsSupported() const noexcept; - // True once UpdateProps has been called at least once for this node. SvgView::Draw and - // RecurseRenderNode call Render() on freshly-mounted children as soon as they appear in the - // ComponentView tree, which can race ahead of Fabric's own prop-update pass for a brand-new - // node created in the same mount (e.g. a responsive breakpoint swapping in a new SVG tree). - // Render()/OnRender() dereference m_props (via winrt::get_self) unconditionally, so - // calling them before UpdateProps has ever run reads through a null-backed props object and - // crashes. Skipping such a node for this pass is safe: the in-flight prop update triggers its - // own Invalidate() (RenderableView::UpdateProps's caller chain -> FinalizeUpates -> - // Invalidate) which redraws with props present moments later. + // True once UpdateProps has been called at least once for this node. A freshly-mounted node + // can have Render() called on it (via SvgView::Draw / RecurseRenderNode) before Fabric's own + // prop-update pass reaches it -- e.g. a node attached to an already-live parent, where + // MountChildComponentView's onMounted() cascade, or an async LayoutMetricsChanged/ + // ThemeChanged event, can race ahead of its own UpdateProps call. OnRender() (and every + // override: GroupView, CircleView, ...) dereferences m_props unconditionally via + // winrt::get_self, which is only safe once m_props is non-null. + // + // This used to gate whether Render() (and therefore CreateChild() + recursion into this + // node's children) ran at all. That skipped this node's *entire subtree* for the pass, + // including any children that already do have their own props, and depended on some later + // trigger to redraw them. HasProps() is now consulted only inside RenderableView::Render(), + // immediately around the OnRender() call -- see there for why that's both crash-safe and + // can't drop an already-ready descendant. bool HasProps() const noexcept { return static_cast(m_props); } diff --git a/windows/RNSVG/Fabric/SvgView.cpp b/windows/RNSVG/Fabric/SvgView.cpp index d7fa0a986..0b2962fbc 100644 --- a/windows/RNSVG/Fabric/SvgView.cpp +++ b/windows/RNSVG/Fabric/SvgView.cpp @@ -226,7 +226,11 @@ void RecurseRenderNode( { auto renderable = child.UserData().try_as(); - if (renderable && renderable->IsSupported() && renderable->HasProps()) { + // Do NOT also gate on HasProps() here: that would skip this node's entire subtree + // (CreateChild() never runs, so there's nothing to recurse into) instead of just the + // props-dependent attributes on this one node. See RenderableView::Render() for the + // node-local guard that replaces it. + if (renderable && renderable->IsSupported()) { ID2D1SvgElement &newElement = renderable->Render(*root, document, svgElement); RecurseRenderNode(root, child, document, newElement); } @@ -294,9 +298,12 @@ void SvgView::Draw( // it fell through to the SVG spec's black default regardless of any propList/inheritance // fix on the descendant's own side (see RenderableView::Render). Calling Render() here, // exactly like RecurseRenderNode does for every other level, fixes that. + // + // (Not gated on HasProps() here for the same reason as RecurseRenderNode above -- see + // RenderableView::Render() for the node-local guard.) for (auto const &child : view.Children()) { auto renderable = child.UserData().try_as(); - if (renderable && renderable->IsSupported() && renderable->HasProps()) { + if (renderable && renderable->IsSupported()) { ID2D1SvgElement &newElement = renderable->Render(*this, *spSvgDocument, *spRoot); RecurseRenderNode(this, child, *spSvgDocument, newElement); }