Canvas: fix 21 correctness and robustness bugs in the shared Canvas2D polyfill

[bkaradzic-microsoft]

Fixes #1782.

21 Canvas2D bugs in the shared canvas polyfill. All are backend-agnostic (nanovg / polyfill level), so they affect the existing bgfx path exactly as found — I hit them while bringing up a WebGPU canvas backend, but nothing here is WebGPU-specific.

Most are "the method threw not implemented but the caller was asking for nothing", or "the paint was built from the wrong geometry". Several have to land together: 1-4 are all required before the GUI ColorPicker renders at all, and 5-8 before a gradient-stroked GUI control renders at all.

Correctness

#	Bug
1	ImageData.data returned a fresh copy on every access, so getImageData → mutate → putImageData silently no-op'd. It is also now the specified Uint8ClampedArray; a plain Uint8Array wrapped data[i] = v + 40 modulo 256, turning bright pixels dark
2	clip() suppressed beginPath() for every later draw, so each fill repainted the union of every rect since the clip
3	3-arg drawImage(img, dx, dy) anchored the pattern at (0,0) while drawing the rect at (dx,dy), so it drew nothing at any non-zero offset
4	Gradient paint was built from the shape being filled, not the gradient — every gradient came out horizontal, anchored at the canvas origin, with (x0,y0)→(x1,y1) discarded
5	strokeStyle accepted only a color string; a gradient threw TypeError: A string was expected and took the whole scene down (GUI Line, Button border)
6	Radial gradients discarded the inner circle, so every one came out concentric with r0 == 0. Now solves the real two-circle equation. The tangent case (leading coefficient vanishes → linear solve) needs its own branch and is not exotic — the repro's createRadialGradient(100,100,150, 250,100,300) lands there
7	Color stops were interpolated in straight alpha, so #ff0000ff → #00000000 faded through maroon to black. Now premultiplied as specified
8	fill() never bound the fill style, using whatever color nanovg happened to hold — never a gradient. Every path-filled control (Ellipse, Button background, Slider) filled solid white
9	createImageData was missing entirely
10	Path2D was registered only on _native, not as a global, so AbstractEngine.createCanvasPath2D threw ReferenceError on every engine except ThinNativeEngine
11	loadTTF failures surfaced as a bare Error: Invalid argument, naming neither method nor argument. Now descriptive, and any ArrayBuffer view is accepted — so the usual real cause, Tools.LoadFileAsync(url) without useArrayBuffer, is named directly
12	putImageData was an unconditional throw. Now writes through a transient nanovg image under nvgSave/nvgReset with NVG_COPY, so it correctly ignores transform, clip, globalAlpha, compositing, shadows and filters and replaces rather than blends
13	setLineDash threw — and GUI Line/MultiLine call it every render with _dash defaulting to [], so any scene with a GUI line died on an empty dash list asking for nothing. Empty now means "solid", which is what we already draw
14	The color regex matched components as 1-3 digit integers, so the very common rgba(0, 0, 0, 0.5) failed to parse. Now generic CSS numbers with optional %, plus the whitespace-separated and / alpha forms and hsl()/hsla() — see the behaviour change below
15	All four shadow attributes threw from both getter and setter — and GUI resets shadowBlur/shadowOffset* to 0 right after drawing, so the throw hit the reset path too and any shadowed control failed outright. nanovg has no shadow primitive, but these are ordinary state attributes, so they are stored and reported
16	save()/restore() snapshotted only fillStyle/strokeStyle, but the wrapper mirrors everything it reprojects or that nanovg cannot report back — so lineCap, lineJoin, lineWidth, miterLimit, the dash list, filter, direction, letterSpacing, globalAlpha and the shadow attributes were never restored. The getters reported the post-save() value forever, and the shadow attributes, re-applied from the mirror on every draw, actually rendered wrong. All fifteen fields are now snapshotted
17	Coincident color stops were dropped — std::map::insert() discarded the second stop at an already-present offset, which is exactly how the spec encodes a hard transition, so a stripe pattern came out as a smooth fade. Now a std::multimap
18	setLineDash() cleared the list before validating, so setLineDash([-1]) after setLineDash([5,10]) left getLineDash() returning []

Two smaller gradient bugs were fixed alongside 4-8: the ramp sample buffer was uninitialized (samples outside the stop range read stack garbage), and gradientSpan divided by zero when two stops shared an offset.

Robustness

Three crash classes found by the Copilot review, all reachable from ordinary script rather than only from a misbehaving embedder:

#	Bug
19	fillStyle = "rgb(999…999, 0, 0)", font = "18e999px Arial" and letterSpacing = "999…999px" made std::stof/std::stoi throw std::out_of_range. That is not a Napi::Error, so node-addon-api's wrapper did not catch it and it unwound out of the N-API boundary and terminated the process. Now strtof/strtol, with infinities folded to finite extremes so the existing clamps stay well defined (nvgHSLA takes fmodf of the hue, and fmodf(inf, 1) is NaN)
20	A gradient outliving its context: UpdateCache() dereferenced context.lock() three times unchecked, so the guard in Paint() ran after the crash
21	putImageData(img, 0, 0, 0, 0, -2147483648, 1) normalized negative dirty extents in int32_t, making dirtyX += dirtyWidth and -dirtyWidth signed overflow on caller-controlled input. Now int64_t, saturating back after clipping

19 is confirmed by experiment, not inspection: built without the fix, the unit test binary stops dead inside the Canvas2D suite with [Uncaught Error] Unknown failure and exit 1, taking the remaining ~20 tests with it. The pre-existing font guard covered only std::invalid_argument (the "normal" keyword), so out_of_range still went through.

One intentional behaviour change — please look at this closely

The parseColor unit test asserted rgba(16,32,48,64) → alpha 0x40, i.e. that alpha is a 0-255 channel like r/g/b. CSS Color defines alpha as 0-1 (or a percentage), so a browser clamps 64 to 1 and paints that colour opaque.

That reading was not a deliberate extension — the old regex matched all four components as 1-3 digit integers and ran std::stoi over them, so alpha simply inherited the channel treatment, and the test was written to describe what the implementation happened to do (it arrived with the impl in #1051). It is also the same root cause as bug 14: under a 0-255 alpha, rgba(0, 0, 0, 0.5) truncates to 0 and the colour vanishes. Alpha cannot be both, so the test now follows CSS, extended to cover fractional and percentage alpha, the / alpha form, whitespace-separated and percentage components, and hsl()/hsla(). All ten malformed-input cases still throw.

Deliberate limitations

• Non-rectangular clip() is approximated. nanovg's scissor is rectangle-only and no stencil path is available, so the choice is between approximating and throwing; this keeps the rectangular case exact and degrades the other rather than failing the scene.
• putImageData() clobbers the current path — as do fillRect, clearRect, drawImage and Path2D playback, none of which may touch it per spec. nanovg offers no local fix, since nvgSave/nvgRestore only push and pop NVGstate while nvgBeginPath clears ctx->ncommands and the path cache. A real fix means journaling the path wrapper-side and replaying it; self-contained, and better as a follow-up.

Validation

Full Playground validation sweep 304/304 PASS, no regressions; JavaScript.All unit tests 36/36.

Three previously-excluded tests are re-enabled and now pass inside the default 2.5% budget:

Test	Before	After	Needs
GUI Gradient Linear (#XCPP9Y#17227)	TypeError — never rendered	1.14%	5, then 8
GUI Gradient Radial (#4Z7EK3)	23.6%	0.62%	6 (tangent case)
GUI Gradient Linear with transparency (#PFK1Z5)	33.8%	pixel-exact	7

The GUI test went 13.5% → 3.9% (allowed 4%) across bugs 1-4 — 13.5% → 8.2% (clip) → 6.4% (drawImage origin) → 3.9% (gradients) — because its ColorPicker builds its wheel via getImageData + mutate + putImageData (1), composites it with a 3-arg drawImage at a non-zero offset (3), draws it and its saturation square under one clip() (2), and paints that square with two overlaid linear gradients (4). Nine further tests newly pass across the same four fixes.

Bugs 10 and 11 are test-neutral and were verified directly, and new Canvas2D unit tests cover the contract that used to throw: both style round-trips, a CanvasGradient as fillStyle and as strokeStyle, a radial gradient from two independent circles, save/restore of a gradient strokeStyle and of all fifteen state fields, dash retention on a rejected argument, coincident stops, and three regressions for the out-of-range parse crashes.

One implementation note: DataView is read through its buffer/byteOffset/byteLength properties rather than Napi::DataView, because Chakra's Node-API backs napi_get_dataview_info with JsGetExternalData, which only succeeds for natively created DataViews — a JS-constructed one fails with napi_invalid_arg even though napi_is_dataview returns true. That shim inconsistency is a separate JsRuntimeHost issue; this change just avoids depending on it.

[Copilot]

Pull request overview

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Fixes multiple Canvas2D correctness issues in the shared canvas polyfill (ImageData semantics, clipping/path behavior, drawImage anchoring, gradient geometry, and missing createImageData).

Changes:

• Make ImageData.data a stable, spec-correct clamped typed array instead of returning a fresh copy each access.
• Fix path handling under clip() and correct 3-arg drawImage() pattern anchoring.
• Implement gradient paints based on gradient geometry (plus ramp baking fixes) and add createImageData() overloads.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
Polyfills/Canvas/Source/ImageData.h	Store and reuse a persistent JS typed array for ImageData.data.
Polyfills/Canvas/Source/ImageData.cpp	Allocate clamped backing array once, read pixels into it, and return it unchanged.
Polyfills/Canvas/Source/Gradient.h	Add CanvasGradient::Paint() API to produce geometry-correct NanoVG paint.
Polyfills/Canvas/Source/Gradient.cpp	Fix ramp baking edge cases and implement paint generation for linear/radial gradients.
Polyfills/Canvas/Source/Context.h	Add createImageData() and simplify BindFillStyle signature.
Polyfills/Canvas/Source/Context.cpp	Register/implement createImageData(), fix clip/path behavior, fix drawImage anchoring, use gradient->Paint().

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Pull request overview

Copilot reviewed 12 out of 13 changed files in this pull request and generated 3 comments.

Suppressed comments (12)

Polyfills/Canvas/Source/Context.cpp:922

• putImageData() must not modify the current path, but the implementation calls nvgBeginPath()/nvgRect() and then explicitly records that the path is now the upload rectangle. A path built before putImageData() is consequently lost, and a following stroke()/fill() targets the wrong geometry. Render the upload without replacing the user path, or preserve/replay the path around this internal draw.

        // The path now holds just this rect, so clear the non-rect flag: a stale `true` left over
        // from an earlier arc/curve would make a subsequent clip() take the emulated path branch.
        m_pathHasNonRect = false;

Polyfills/Canvas/Source/Context.h:122

• These shadow attributes are also Canvas2D drawing state, but they are not included in SavedStyle; restore() only rewinds fill/stroke. A shadow value assigned after save() therefore survives restore(), contradicting the round-trip/state behavior described in the PR. Store and restore all four shadow fields.

        // Shadow attributes from shadowColor/shadowBlur/shadowOffsetX/shadowOffsetY.
        // Retained only so the getters round-trip; nanovg has no shadow primitive,
        // so nothing is ever drawn from them. Defaults are the spec's.
        std::string m_shadowColor{"rgba(0, 0, 0, 0)"};
        double m_shadowBlur{0.0};

Polyfills/Canvas/Source/Gradient.cpp:388

• This still maps the old concentric texture onto (x1, y1, r1). RadialGradientStops() fixes its focal point at the texture center (fxp/fyp = 0) and never uses x0, y0, or r0, so non-concentric and nonzero-inner-radius gradients—including the tangent case named in the PR—remain incorrect. The baked field must evaluate the two-circle equation before this paint is mapped.

        // The radial ramp is baked into a square image whose inscribed circle is the r1
        // isoline, so map that image onto the bounding box of the outer circle.
        const float radius = std::max(r1, 1e-4f);
        return nvgImagePattern(*nvg, x1 - radius, y1 - radius, 2.f * radius, 2.f * radius, 0.f, cachedImage, 1.f);

Polyfills/Canvas/Source/Gradient.cpp:167

• Zero-initializing the ramp does not implement the claimed premultiplied-alpha interpolation. gradientSpan() still interpolates raw r/g/b/a independently at lines 53-60, and lerpColor() does the same at lines 194-200, so transparent linear and radial stops still fade through dark straight-alpha colors. Premultiply RGB before interpolation and unpremultiply the baked sample afterward in both paths.

        // Zero-initialize: the spans below only cover the range the stops span, so any
        // sample left untouched would otherwise be read from uninitialized stack memory.
        uint32_t data[GRADIENT_SAMPLES_L]{};

Polyfills/Canvas/Source/Gradient.cpp:51

• This guard cannot handle coincident color stops because colors is a std::map<float, NVGcolor> and AddColorStop() uses insert; the second stop at an identical offset is discarded before gradientSpan() runs. Hard transitions at duplicate offsets therefore remain wrong. Preserve duplicate stops with ordered sequence/multimap storage and process them in insertion order.

        // Coincident stops produce an empty span; the per-sample deltas below would divide
        // by zero and write NaNs into the ramp.
        if (e <= s)
        {
            return;

Polyfills/Canvas/Source/Context.cpp:159

• The new binder is still not called by Context::Fill() (lines 266-281), which proceeds directly to nvgFill(). Assigning a gradient and then calling fill() therefore continues to use stale NanoVG paint, leaving the path-fill correctness fix described by the PR unimplemented. Bind the current fill style after any PlayPath2D() call and before nvgFill().

    void Context::BindFillStyle(const Napi::CallbackInfo& info)

Polyfills/Canvas/Source/Context.h:158

• Only a fill-style binder is introduced. m_strokeStyle/SavedStyle::strokeStyle remain strings, SetStrokeStyle() still unconditionally casts to Napi::String, and stroke, strokeRect, and strokeText never bind a gradient paint. Thus the PR's core CanvasGradient stroke support still throws on assignment. Add the matching stroke variant/binder and invoke it for every stroke operation.

        void BindFillStyle(const Napi::CallbackInfo& info);

Polyfills/Canvas/Source/Context.cpp:448

• Leaving the clip path current is not clipping: fillRect() appends its rectangle and fills the union of that rectangle and the clip path, while clearRect() and drawImage() call nvgBeginPath() and ignore this flag entirely. Non-rectangular clips therefore either paint the clip shape or are not applied. Use an actual clip mask/stencil implementation (or explicitly keep this unsupported) rather than treating the clip path as draw geometry.

This issue also appears on line 920 of the same file.

        // A non-rectangular clip path cannot be expressed as a scissor rectangle.
        // Emulate it by leaving the path current so the next fill draws it, and
        // leave any enclosing scissor untouched rather than clipping to a
        // rectangle this path never described.
        if (m_pathHasNonRect)
        {
            m_isClipped = true;
            return;

Polyfills/Canvas/Source/Context.cpp:1388

• The comment says an invalid dash list retains the previous value, but m_lineDash is cleared before validation and cleared again here. For example, after [5, 10], setLineDash([-1]) incorrectly changes getLineDash() to []. Parse into a temporary vector and assign it only after every segment validates.

                    // Per spec, a list containing a non-finite or negative value
                    // is ignored entirely and the previous list is retained; a
                    // non-numeric entry cannot be interpreted, so ignore it too.
                    m_lineDash.clear();
                    return;

Polyfills/Canvas/Source/Context.cpp:1400

• Canvas2D duplicates an odd-length dash sequence, so setLineDash([5]) must make getLineDash() return [5, 5]. The current code stores and returns only [5]. Duplicate the parsed sequence when its length is odd.

                m_lineDash.push_back(value);
            }
        }

Polyfills/Canvas/Source/Context.h:116

• m_lineDash is new wrapper-side canvas state, but SavedStyle still contains only fill/stroke styles. Consequently save(); setLineDash(...); restore(); getLineDash() does not restore the saved pattern as Canvas2D requires. Include the dash vector in the state pushed and restored alongside these fields.

This issue also appears on line 118 of the same file.

        // Dash pattern from setLineDash. Retained only so getLineDash() round-trips;
        // strokes are always drawn solid (nanovg has no dashed stroke).
        std::vector<double> m_lineDash{};

Polyfills/Canvas/Source/Canvas.cpp:77

• Buffer validation is still skipped when the font name already exists, because GetFontDataArgument() is only called inside this conditional. After one successful load, Canvas.loadTTF(name) or Canvas.loadTTF(name, "text") silently succeeds despite the PR's stated arity/type validation. Validate and extract argument 2 before the duplicate-name early-out, then discard the extracted bytes when retaining the existing font is intentional.

        if (fontsInfos.find(fontName) == fontsInfos.end())
        {
            fontsInfos[fontName] = GetFontDataArgument(info, 1, "Canvas.loadTTF");

[bkaradzic-microsoft]

You're right, and thanks for catching it — that was a genuine miss on my part, not a stale view on your end.

canvas-shared-fixes was sitting two commits ahead of the remote locally, so 99ff0b8b (gradient semantics) and 7bd4ae14 (the putImageData / std::stof / null-context hardening) never made it to the PR. The branch is now pushed and the head is 7bd4ae14; CI is re-running.

The part worth flagging: as you say, the review threads were already resolved, so the PR read as clean and green while the code being described wasn't actually on it. That's the bad failure mode here — the green checkmark was real, it just wasn't covering the fixes under discussion. I've re-verified against the pushed tree that each resolved thread now corresponds to code that's actually there:

• Colors.h — std::stof replaced with strtof, with the non-finite results folded back to finite extremes so the downstream clamps (and nvgHSLA's fmodf on hue) stay well defined.
• Context.cpp putImageData — negative-extent normalization moved into int64_t, so negating INT32_MIN and summing two INT32_MINs are no longer signed overflow.
• Gradient.cpp — context.lock() is locked once and checked for both a null shared_ptr and a null underlying context before any dereference, including on the UpdateCache() path that previously dereferenced it internally before the check.

I also confirmed the branch still merges cleanly onto current master — config.json is touched by both this branch and #1830, but it auto-merges without conflict.

Sorry for the round trip. Will wait for CI to come back green before asking for another look.

[bkaradzic-microsoft]

CI is back green on 7bd4ae14 — 34/34, no failures. Ready for another look whenever you have time, @bghgary.

[matthargett]

Thanks for closing these conformance gaps! If we are consistently beating the 2.5% threshold of incorrect pixels, I'd recommend lowering it to tighten things up a bit now that so much of the screenshot testing stability and accuracy has made such huge strides.

[bkaradzic-microsoft]

Thanks! I agree with the direction, so I pulled the actual numbers rather than guess at the headroom.

Data is from this PR's green CI run (7bd4ae14), scraped from the Pixel difference: lines the harness logs per test. Validation runs on the Win32_x64_*_D3D11 jobs, and 254 of the comparisons use the default 2.5% (the other 12 tests in config.json already carry explicit higher errorRatio overrides, 4%–35%; nothing currently overrides downward).

Distribution on the 2.5% default

p50	p75	p90	p95	p99	max
0.352%	0.709%	1.134%	1.428%	1.846%	2.249%

So the median has a ~7x margin, but the worst test sits at 2.249% — only 0.25pp under the limit. Lowering the global default is therefore not quite free:

new default	tests that would start failing
2.0%	2
1.5%	8
1.0%	33
0.5%	94

The good news is that these numbers are extremely stable, which is really the property that matters for tightening a threshold. Across two independent CI runs on different commits (bd3e3e27 → 7bd4ae14): p50 0.349% → 0.352%, p90 1.116% → 1.134%, p99 identical at 1.846%, max identical at 2.249%. And within a single run the three JS-engine jobs (Chakra / QuickJS / V8) produce byte-identical distributions — same n, same p50/p90/max, same counts over every candidate threshold. The rendering is deterministic; the 2.5% isn't absorbing run-to-run noise, it's absorbing a small number of genuinely-large stable diffs.

That means a tightening is low-risk and won't introduce flakiness — it just has to be paired with per-test overrides for the tail, exactly the mechanism config.json already uses in the other direction. Concretely I'd suggest default 1.5% plus explicit overrides on the 8 tests above it, which tightens 246 of 254 tests by 40% while leaving the known-bad ones documented and visible instead of hidden under a blanket allowance.

Two caveats worth stating: this is Win32 D3D11 only (D3D12 CI doesn't run the pixel comparison today — relevant to #1671), and the per-channel threshold of 25 is arguably the blunter knob of the two, since it's what lets a pixel be off by up to 25/255 per channel before it's even counted.

I'd rather not fold this into this PR though — it's repo-wide test policy and would churn config.json for 8 unrelated tests, whereas this PR is scoped to the Canvas polyfill. Happy to do it as a follow-up PR right after this lands, with the same measurements attached. Sound good?

[bkaradzic-microsoft]

Pushed two more commits (8bbf33bc, bd40ea28) fixing three further issues, and triaged the twelve comments in the collapsed Suppressed comments block of the Copilot review — those don't create review threads, so they weren't visible in the resolved/unresolved list and I'd missed them until now.

Most of that block was generated against bd3e3e27, before the last two pushes, so a good part of it describes code that is no longer there. Breakdown:

Fixed in this push

SavedStyle didn't include the shadow attributes. Correct, and worse than reported. nvgRestore() rewinds nanovg's copy of the state, but the wrapper keeps its own C++ mirrors of everything it has to reproject or that nanovg can't report back, and SavedStyle only snapshotted fillStyle/strokeStyle. So lineCap, lineJoin, lineWidth, miterLimit, the dash list, filter, direction, letterSpacing and globalAlpha had the same bug — the getters reported the post-save() value forever after, and the shadow attributes, which are re-applied from the mirror on every draw, actually rendered wrong. SavedStyle now snapshots all fifteen fields.

Coincident color stops were dropped. Correct — std::map + insert() silently discarded the second stop at an offset already present, which is exactly how the spec encodes a hard transition, so a stripe pattern came out as a smooth fade. Now a std::multimap. Both consumers only iterate in sorted order into a vector, so it's a drop-in, and the existing zero-width-span guard at Gradient.cpp:51 turns the duplicate offset into the required hard edge.

setLineDash() didn't retain the previous list on a rejected argument. Correct, and the code contradicted its own comment: m_lineDash was cleared before validation, so after setLineDash([5, 10]), setLineDash([-1]) left getLineDash() returning [] instead of [5, 10]. Now parsed into a temporary and committed only once every segment validates.

Three unit tests cover the state round-trip and the dash retention; a fourth covers the duplicate stops. 36/36 pass locally.

Already fixed in 99ff0b8b/7bd4ae14 — these were generated against the older head

• "Fill() never calls the new binder" / "only a fill-style binder is introduced; stroke still throws" — both BindFillStyle and BindStrokeStyle exist and are called from all six draw sites (Context.cpp:231, 329, 535, 626, 727, 1521), and m_fillStyle/m_strokeStyle are both std::variant<std::string, CanvasGradient*>, not strings.
• "premultiplied-alpha interpolation isn't actually implemented" — it is, on both paths: gradientSpan() premultiplies at Gradient.cpp:75-76 and unpremultiplies the baked sample at :88, and lerpColor() does the same at :226-227 and :232.
• "the radial paint still ignores x0, y0, r0" — it doesn't; RadialGradientStops() calls solveRadialOffset() (:362), which solves the two-circle equation over all six parameters. The image is still mapped onto the outer circle's bounding box, but that's the correct place to put it now that the ramp is baked per-pixel rather than being a concentric texture.

Known limitations, unchanged and deliberate

Non-rectangular clip(). Flagged as "not clipping" — that's fair, and it's called out as such in the PR description. nanovg's scissor is rectangle-only and there's no stencil path available here, so the choice is between an approximation and throwing. This keeps the rectangular case exact and degrades the non-rectangular one instead of failing the scene.

putImageData() clobbers the current path. Correct, and I'd rather not fix it in this PR. It isn't specific to putImageData — fillRect(), clearRect(), drawImage() and Path2D playback all call nvgBeginPath(), and per spec none of them may touch the current path. nanovg offers no way to fix it locally: nvgSave/nvgRestore only push and pop NVGstate, while nvgBeginPath clears ctx->ncommands and the path cache, which aren't part of that state. A real fix means journaling the current path wrapper-side and replaying it after each of those operations — the Path2DCommand machinery in Path2D.h is the obvious basis for it, but it means routing every path method on Context through the journal. That's a self-contained change and I'd prefer it as a follow-up rather than growing this PR further; happy to open an issue to track it.

CI was 34/34 green on the previous head; I'll confirm it stays green on bd40ea28.

[bghgary]

[Reviewed by Copilot on behalf of @bghgary]

Twelve inline.

[bghgary]

roundRect() + clip() misses this branch and falls through to the scissor below, clipping to m_rectangleClipping — which RoundRect sets to the plain bounding box (:502) without ever setting m_pathHasNonRect. The radii are dropped and the corners come out square. A rounded rect is exactly the kind of shape nvgScissor cannot express, which is what this flag's own comment says it tracks.

Pre-existing rather than a regression, but the flag is new here and this is the case that slips through it.

[bkaradzic-microsoft]

You're right that roundRect never sets the flag, and I set it -- then the visual tests said no.

Routing roundRect into the emulation branch makes the output substantially worse. The emulation leaves the path current so the next fill draws it, and nanovg fills the union of the subpaths rather than their intersection, so roundRect(); clip(); fillRect() paints the whole fillRect instead of the rounded region. Measured on the Native Canvas visual test:

roundRect sets m_pathHasNonRect	pixel difference
no (current)	1.850%
yes	20.980%

I also gated it on nanovg's own 0.1 radius threshold first, in case near-zero radii were the problem. Same result.

So dropping the radii is wrong, but it is the far smaller of the two errors, and the emulation cannot be made right by feeding more shapes into it -- a correct clip() needs real path clipping (a stencil pass in nanovg). I've left roundRect on the scissor and put the measurement and the reasoning in a comment at the end of RoundRect so the next reader doesn't repeat the experiment. Happy to open a follow-up issue for the stencil-based clip if you'd like it tracked.

d690deb

[bghgary]

font is the one exposed attribute left out. SetFont writes m_font and m_currentFontId (Context.cpp:1691-1698) and GetFont reads m_font, so it is the same wrapper-side mirror as the fields here, and per spec restore() rewinds it. It is also worse than the getter-only cases: m_currentFontId is what the text draw path binds, so the wrong font actually renders after a restore.

Holding this state as a single State struct on the class — with save/restore a push/pop of it — would make a forgotten field impossible rather than unlikely.

[bkaradzic-microsoft]

Fixed in d690deb -- SavedStyle now carries font and currentFontId, and Restore rewinds both. Confirmed it was the real-rendering case you describe and not just a getter: with only Save updated the test still failed on the bound face.

New test restores font, which fails without the fix:

AssertionError: expected '40px "Times"' to equal '18px "Arial"'

(It compares against the round-tripped value rather than the assigned literal, since the getter reports a normalized serialization.)

On folding the whole thing into a single State struct that save/restore push and pop -- I agree, and it's the right shape; a forgotten field is exactly how this one got missed. I'd rather not do it in this PR: it touches every getter and setter on Context, which would bury the behavior fixes here in a rename diff. Happy to do it as the immediate follow-up, or here if you'd prefer it not wait.

[bghgary]

These warnings go to stderr, which on Android and iOS reaches nobody by default — the platforms where a silently-degraded canvas is hardest to diagnose.

JsConsoleLogger::LogWarn(info.Env(), …) calls console.warn, needs nothing but the Napi::Env already in hand, and lands where an app developer will see it. It lives in Plugins/NativeEngine/Source/ today, so this means relocating it; JsRuntimeHost's Core/JsRuntime fits — Canvas already links it through JsRuntimeInternal, it exposes napi publicly, and JsRuntimeHost owns the Console polyfill that provides the console being called. That makes it a JsRuntimeHost change plus a GIT_TAG bump here.

The shadow warning in WarnShadowUnsupported needs the same treatment, plus an Napi::Env parameter — it takes none today, and its three callers all have info.

[bkaradzic-microsoft]

Agreed on all of it -- stderr is the wrong sink here, and JsConsoleLogger::LogWarn with the Napi::Env already in hand is the right one.

I'd like to do it as a separate PR rather than in this one. As you laid out it isn't a local change: JsConsoleLogger has to move from Plugins/NativeEngine/Source/ into JsRuntimeHost's Core/JsRuntime, which is a JsRuntimeHost PR, and then this repo needs a GIT_TAG bump before either call site can use it. That sequencing means it can't land atomically with the fixes here, and I'd rather not hold them behind it.

WarnShadowUnsupported gaining an Napi::Env parameter belongs in that same change -- all three callers have info, so it's mechanical once the logger is reachable.

I'll open the JsRuntimeHost PR next and link it here. Say the word if you'd rather this PR wait for it instead.

[bghgary]

When a global Path2D already exists this keeps the foreign one, and Context::Fill (315-317) unwraps whatever object it is handed as a NativeCanvasPath2D. So the branch that preserves someone else's constructor is the branch that guarantees ctx.fill(new Path2D(d)) unwraps an object that was never wrapped.

Canvas::Initialize calls this at Canvas.cpp:311 while the polyfill is still installing, so a global found here came from outside it.

Claiming the global unconditionally is the fix: a foreign Path2D cannot be drawn by this Context under any circumstances, so deferring to it only guarantees the failure. Verifying the object in Fill/Stroke is worth doing as well, but that converts undefined behaviour into a clean error rather than making the call work.

[bkaradzic-microsoft]

Agreed and fixed in d690deb -- the global is now claimed unconditionally, with your reasoning recorded in the comment: a foreign Path2D cannot be drawn by this Context under any circumstances, so the branch that preserved it was the branch that guaranteed the bad unwrap.

Verifying the object in Fill/Stroke is worth doing too, and I'd like to take it with the CanvasGradient::IsInstance pattern from the other thread rather than bolt on a second one-off check -- happy to add it here if you'd rather not split it.

[bkaradzic-microsoft]

Pushed d690deb + 21ac522, rebased onto master (8bf91bc5).

All twelve threads answered. Ten are fixed; two are answered without a code change and say why:

• roundRect + clip -- the fix makes it worse. Setting m_pathHasNonRect there takes the Native Canvas visual test from 1.850% to 20.980%, because the emulation fills the union of the subpaths, not their intersection. Kept on the scissor, measurement recorded in a comment.
• console.warn for the degraded-canvas warnings -- agreed, but it needs JsConsoleLogger moved into JsRuntimeHost plus a GIT_TAG bump, so it can't land atomically here. Proposing a follow-up PR.

Four new unit tests, each verified to fail with the fix reverted and pass with it:

test	failure without the fix
keeps the previous dash list when the argument is not a list	expected [] to deeply equal [ 5, 10 ]
restores font	expected '40px "Times"' to equal '18px "Arial"'
ignores a fillStyle or strokeStyle that is neither a color string nor a gradient	'Error: Invalid argument' was thrown
accepts two color stops at the same offset	retitled -- see the thread; it does not guard the multimap change and now says so

Validation on Win32 D3D11:

• UnitTests: 21/21 suites, 39/39 JavaScript assertions.
• Full Playground visual sweep: ran=305 passed=305 failed=0 missingRef=0.

One thing worth flagging that no thread raised: this PR moves Native Canvas from 0.391% to 1.850% against a 2.5% allowance. It arrives with the first commit in the branch and is the expected consequence of changing gradient and colour semantics against a reference image captured under the old behaviour -- but the remaining margin is thin, so it's worth knowing about.

[Copilot]

Pull request overview

Copilot reviewed 14 out of 15 changed files in this pull request and generated 1 comment.

Suppressed comments (4)

Polyfills/Canvas/Source/Context.cpp:425

• ResetPathState() clears the new flags but leaves m_rectangleClipping describing the previous path. Consequently, after rect(A); beginPath(); clip(), Clip() reuses A even though beginPath() discarded it. Clear or explicitly invalidate the rectangle candidate here, and have Clip() distinguish an empty current path instead of falling back to stale geometry/full-canvas clipping.

        m_isClipped = false;
        m_pathHasNonRect = false;

Polyfills/Canvas/Source/Context.cpp:1499

• Canvas 2D requires an odd-length dash list to be repeated so the stored list has even length. As written, setLineDash([7, 8, 9]) makes getLineDash() return [7, 8, 9] rather than [7, 8, 9, 7, 8, 9]. Normalize parsed before committing it.

        m_lineDash = std::move(parsed);

Polyfills/Canvas/Source/Colors.h:229

• Using independently interchangeable separators accepts invalid CSS colors, such as rgb(1, 2 3) and rgb(1, 2, 3 / .5). Canvas style setters must ignore these malformed values, but this parser now treats them as valid and changes the paint. Parse the legacy comma grammar and modern space/slash grammar as separate alternatives so they cannot be mixed.

            static const std::string separator{R"((?:\s*,\s*|\s+))"};
            static const std::string alphaSeparator{R"((?:\s*,\s*|\s*/\s*))"};
            static const std::string components{
                R"(\(\s*)" + number + separator + number + separator + number +
                "(?:" + alphaSeparator + number + R"()?\s*\))"};

Polyfills/Canvas/Source/Gradient.cpp:433

• This maps the gradient from its raw coordinates and lets nvgFillPaint apply the transform active at draw time. Canvas gradients instead capture the current transform when createLinearGradient/createRadialGradient is called. If the transform changes between creation and painting, both endpoints (and the radial box below) are therefore rendered in the wrong coordinate space. Capture the creation-time transform in CanvasGradient and use it when building the paint.

            return nvgImagePattern(*nvg, x0, y0, length, length, std::atan2(dy, dx), cachedImage, 1.f);

[bkaradzic-microsoft]

Good catch, and it reproduces. I instrumented ~CanvasGradient and the bind path and ran a test that creates the gradient in a scope holding no reference to it, then forces a collection:

~CanvasGradient      this=0000021D4B9DA240
BindFillStyle deref  this=0000021D4B9DA240

Same address, destroyed and then dereferenced. It did not crash, which is why it went unnoticed -- the freed memory simply had not been reused yet. CanvasGradient is an ObjectWrap, so the native instance is owned by the JS wrapper and deleted from its finalizer, and ctx.fillStyle = ctx.createLinearGradient(...) leaves nothing rooting that wrapper.

Fixed in 4201348 by storing the assigned object in a persistent reference rather than a raw pointer. SavedStyle copies the styles on save()/restore() and Napi::ObjectReference is move-only, so it is held through a shared_ptr and the copies share one strong reference.

I also took your point about the getter. It now returns the gradient object instead of a Napi::External wrapping the raw pointer, which is what the spec asks for and makes otherCtx.fillStyle = ctx.fillStyle round-trip -- previously the getter handed back an External that the setter would then reject.

Two tests cover it, one for fillStyle and one for strokeStyle. Each tags the gradient with an expando, drops every reference, collects, and then checks the tag survives and the native gradient still accepts addColorStop. Both fail with the change reverted and pass with it. Full suite is 21/21 and the visual sweep is 305/305.

[Copilot]

Pull request overview

Copilot reviewed 14 out of 15 changed files in this pull request and generated 1 comment.

Suppressed comments (4)

Polyfills/Canvas/Source/Gradient.cpp:433

• The pattern is transformed when nvgFillPaint/nvgStrokePaint is called, so these raw endpoints use the transform active at draw time. Canvas gradients instead capture the current coordinate space when they are created; creating a gradient under scale(2, 2), resetting the transform, and then filling should retain the scaled endpoints, but this implementation does not. Capture the creation transform and bake/apply it to both linear and radial geometry before binding the paint.

            return nvgImagePattern(*nvg, x0, y0, length, length, std::atan2(dy, dx), cachedImage, 1.f);

Polyfills/Canvas/Source/Context.cpp:1501

• Canvas normalizes an odd-length dash list by concatenating it with itself. Committing parsed directly makes setLineDash([7, 8, 9]); getLineDash() return [7, 8, 9] rather than [7, 8, 9, 7, 8, 9], so the newly added round-trip API is observably incorrect.

        m_lineDash = std::move(parsed);

Polyfills/Canvas/Source/Context.cpp:247

• This getter enables the documented otherCtx.fillStyle = ctx.fillStyle round trip, but the gradient image is cached in the creator context. The destination context then tries to bind that image handle through its own NanoVG texture table, where it is not registered, so cross-context gradient assignment renders no gradient (and also stops working once the creator is disposed). Cache/bake the ramp for the context that is drawing it rather than permanently binding a gradient to its creator.

            // Return the gradient object that was assigned, as the spec requires, so that
            // `otherCtx.fillStyle = ctx.fillStyle` round-trips.
            return std::get<GradientStyle>(m_fillStyle)->Value();

Polyfills/Canvas/Source/Context.cpp:884

• putImageData has 3-argument and 7-argument overloads. Calls with 4–6 arguments match neither overload and must throw, but this validation accepts them and the dirty-rectangle branch silently ignores the supplied extra arguments, writing the full image instead. Reject the incomplete dirty-rectangle form explicitly.

        if (info.Length() < 3 || !info[0].IsObject())
        {
            throw Napi::TypeError::New(env, "Context2D.putImageData requires at least 3 arguments (imageData, dx, dy).");
        }