fix: don't clobber inherited fill on stroke-only shapes (emit fill only when set)

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 <Svg fill="none">). 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);

windows/RNSVG/Fabric/RenderableView.cpp

@@ -157,7 +157,74 @@ 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<TProps>(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<SvgGroupProps>(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
+  // <Svg fill="none">). 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 <svg> 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<ID2D1SvgPaint> parentPaint;
+    winrt::com_ptr<ID2D1SvgPaint> 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;
 }
 

windows/RNSVG/Fabric/RenderableView.h

@@ -143,6 +143,24 @@ 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. 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<TProps>, 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<bool>(m_props);
+  }
+
   void Invalidate(const winrt::Microsoft::ReactNative::ComponentView &view);
 
  protected:

windows/RNSVG/Fabric/SvgView.cpp

@@ -226,6 +226,10 @@ void RecurseRenderNode(
     {
       auto renderable = child.UserData().try_as<RenderableView>();
 
+      // 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);
@@ -281,10 +285,27 @@ void SvgView::Draw(
     spRoot->SetAttributeValue(SvgStrings::preserveAspectRatioAttributeName, preserveAspectRatio);
   }
 
+  // Render the SvgView's own direct children (e.g. the JS-side <G> that Svg.tsx wraps
+  // around every <Svg>'s children, carrying <Svg>'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 <Svg fill="none"> forwards fill="none" to that
+  // wrapper <G>, but the wrapper <G>'s own D2D <g> 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.
+  //
+  // (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().as<RenderableView>();
-    if (renderable->IsSupported()) {
-      RecurseRenderNode(this, child, *spSvgDocument, *spRoot);
+    auto renderable = child.UserData().try_as<RenderableView>();
+    if (renderable && renderable->IsSupported()) {
+      ID2D1SvgElement &newElement = renderable->Render(*this, *spSvgDocument, *spRoot);
+      RecurseRenderNode(this, child, *spSvgDocument, newElement);
     }
   }