Canvas: consolidate the drawing state into a State struct - #1843
Conversation
save()/restore() had to rewind seventeen separate wrapper-side members, and did it by hand: Save() built a SavedStyle from a positional initializer list and Restore() copied each field back out. The field list was therefore written out three times -- the members, the SavedStyle mirror, and the two copy loops -- so adding an attribute meant editing four places, and missing one of them silently produced an attribute that never rewound. Group them into a State struct instead and push/pop it whole. Save() and Restore() no longer name any individual attribute, so a new attribute is rewound correctly by construction. No behavior change: the set of saved fields is exactly what SavedStyle held. The current path is deliberately still outside the stack, since per spec save() does not save it.
globalAlpha was registered with a nullptr getter, so `ctx.globalAlpha` read back undefined. The setter passed the value straight to nvgGlobalAlpha and never mirrored it, which left the wrapper with a globalAlpha field that save()/restore() dutifully pushed and popped but that nothing could ever read -- consolidating the state into State is what made the dead field obvious. Register the getter and mirror the value. Rendering was already correct, since nvgSave/nvgRestore rewinds nanovg's own copy, so this only affects what the attribute reports. Also apply the spec's range rule while the value is being validated: a value that is not finite, or outside [0, 1], is ignored and leaves the previous value in place, rather than being clamped or passed through. The old code forwarded NaN and out-of-range values to nanovg unchecked.
There was a problem hiding this comment.
Pull request overview
This PR refactors the Canvas2D Context wrapper-side drawing state into a single State struct that is saved/restored as a whole, and fixes globalAlpha to be readable (with spec-compliant range/finite-value handling).
Changes:
- Consolidate wrapper-side drawing attributes into
Context::Stateand push/pop that insave()/restore(). - Register
globalAlphagetter, mirror its value in wrapper state, and ignore invalid values per spec. - Extend Canvas2D unit tests to cover
globalAlphareadback, save/restore, and invalid-value behavior.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| Polyfills/Canvas/Source/Context.h | Introduces State struct and saved-state stack; adds GetGlobalAlpha declaration. |
| Polyfills/Canvas/Source/Context.cpp | Switches attribute storage to m_state, updates save/restore to push/pop State, and implements globalAlpha getter + validation. |
| Apps/UnitTests/JavaScript/src/tests.javaScript.all.ts | Adds assertions/tests covering globalAlpha readback and invalid values. |
| Apps/UnitTests/JavaScript/dist/tests.javaScript.all.js | Updates built test output to match the TypeScript test changes. |
Suppressed comments (1)
Polyfills/Canvas/Source/Context.h:150
- State defaults for miterLimit and lineWidth are 0, but Canvas2D defaults are miterLimit=10 and lineWidth=1. With the current defaults, new contexts will report incorrect values until explicitly set, even though nanovg likely renders with its own defaults.
float miterLimit{0.f};
float lineWidth{0.f};
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
lineCap, lineJoin, lineWidth and miterLimit are mirrors of state nanovg also holds, and their getters return the mirror. They were value- initialized to ""/""/0/0, but nvgReset installs NVG_BUTT, NVG_MITER, strokeWidth 1 and miterLimit 10, which is also what the spec requires, so a fresh context reported four defaults it was not drawing with. Only the getters are affected: the mirror is never pushed to nanovg, and restore() pops it while nvgRestore rewinds nanovg's own copy, so nothing rendered changes.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
Polyfills/Canvas/Source/Context.h:155
- Now that
globalAlphais readable, storing it asfloatmakes valid JavaScript values fail to round-trip: assigning0.1reports approximately0.10000000149. The attribute is anunrestricted double; keep the wrapper-side value asdoubleand cast only when passing it to NanoVG. The new tests currently use binary-exact values (0.5,0.25), so they do not expose this loss of precision.
float globalAlpha{1.f};
Co-authored-by: Gary Hsu <bghgary@users.noreply.github.com>
|
[Commented by Copilot on behalf of @bghgary] This is still on master at c8a8071 — m_state.globalAlpha = alpha;
nvgGlobalAlpha(*m_nvg, static_cast<float>(m_state.globalAlpha));The three assertions use 1, 0.5 and 0, all exact in binary float, so they pass either way. |
Follow-up to #1824, as agreed in review.
Statestructsave()/restore()had to rewind seventeen separate wrapper-side members, and did it by hand:Save()built aSavedStylefrom a positional initializer list,Restore()copied each field back out one at a time. The field list was written out three times over -- the members, theSavedStylemirror, and the two copy loops -- so adding an attribute meant editing four places, and missing one silently produced an attribute that never rewound. That is exactly the bug class #1824 spent several commits fixing.They are now a single
Statestruct, pushed and popped whole:Save/Restoreno longer name a single attribute, so a new one is rewound correctly by construction.No behavior change: the saved set is exactly what
SavedStyleheld. The current path stays outside the stack, since per specsave()does not save it, andm_fontsstays out too -- it is a context-wide face cache, not an attribute.globalAlpha(found by the refactor)Collecting the fields made a dead one obvious:
globalAlphawas pushed and popped by everysave()/restore(), but had zero readers. It was registered write-only ---- so
ctx.globalAlpharead backundefined, and the setter passed the value straight tonvgGlobalAlphawithout ever mirroring it.Registered the getter and mirrored the value. Rendering was already correct (
nvgSave/nvgRestorerewinds nanovg's own copy), so this only changes what the attribute reports.While validating the value I also applied the spec's range rule: a value that is not finite, or outside
[0, 1], is ignored and leaves the previous value in place, rather than being clamped or thrown. The old code forwardedNaNand out-of-range values to nanovg unchecked.Validation
ran=305 passed=305 failed=0; "Native Canvas" unchanged at 1.850%.globalAlphaassertions and nothing else, so the tests are actually gated on the fix.