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); diff --git a/windows/RNSVG/Fabric/RenderableView.cpp b/windows/RNSVG/Fabric/RenderableView.cpp index fb6a70ceb..575bf7615 100644 --- a/windows/RNSVG/Fabric/RenderableView.cpp +++ b/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(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 + // ). 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/RenderableView.h b/windows/RNSVG/Fabric/RenderableView.h index f48326cef..3084ab3a7 100644 --- a/windows/RNSVG/Fabric/RenderableView.h +++ b/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, 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); + } + void Invalidate(const winrt::Microsoft::ReactNative::ComponentView &view); protected: diff --git a/windows/RNSVG/Fabric/SvgView.cpp b/windows/RNSVG/Fabric/SvgView.cpp index 1f1f4080f..0b2962fbc 100644 --- a/windows/RNSVG/Fabric/SvgView.cpp +++ b/windows/RNSVG/Fabric/SvgView.cpp @@ -226,6 +226,10 @@ void RecurseRenderNode( { auto renderable = child.UserData().try_as(); + // 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 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. + // + // (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(); - 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); } }