Uh oh!
There was an error while loading. Please reload this page.
fix(masks): use luminosity mask by default - CU-86c97q74b - #2
Conversation
There was a problem hiding this comment.
Pull request overview
Updates SVG mask handling so that <mask> defaults to a luminosity-based SMask (instead of alpha), while still supporting explicit alpha masks, and extends ICC profile metadata to better handle CMYK-backed masking.
Changes:
- Add a luminosity SMask implementation and make it the default for SVG
<mask>(with opt-in alpha viamask-type="alpha"). - Split mask application into explicit alpha vs luminosity helpers and adjust clipPath to continue using alpha masking semantics.
- Extend ICC profile handling with a
typefield (and plumb it throughiccProfile) to support CMYK-specific mask backdrop configuration.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| package.json | Updates package version (currently appears to regress). |
| lib/write_svg.js | Implements luminosity masks as default and supports mask-type switching between alpha and luminosity. |
| lib/mixins/color_space.js | Extends iccProfile API with an optional type parameter. |
| lib/icc_profile.js | Stores ICC profile type (defaults based on channel count). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "vector" | ||
| ], | ||
| "version": "0.17.56", | ||
| "version": "0.17.6", |
There was a problem hiding this comment.
The package version change looks like a regression/typo: it goes from 0.17.56 to 0.17.6 (lower version and missing a digit). This can break publishing/upgrade paths; please use the correct next semver (likely 0.17.57 or keep the existing patch format).
| "version": "0.17.6", | |
| "version": "0.17.57", |
| const maskType = this.attr('mask-type') ?? 'luminance'; | ||
| this.clip(); | ||
| this.drawChildren(false, true); | ||
| doc.restore(); | ||
| docEndGroup(group); | ||
| docApplyMask(group); | ||
| if (maskType === 'alpha') { | ||
| docApplyAlphaMask(group); | ||
| } else { | ||
| docApplyMask(group); | ||
| } |
There was a problem hiding this comment.
mask-type is read via getAttribute and then compared case-sensitively to 'alpha'. Since mask-type is a CSS keyword/presentation attribute, values should be treated case-insensitively; consider normalizing (e.g., String(...).toLowerCase()) or using the style getter so valid inputs like Alpha/ALPHA work.
| function docApplyAlphaMask(group) { | ||
| const name = 'M' + (doc._maskCount = (doc._maskCount || 0) + 1); | ||
| const gstate = doc.ref({ | ||
| Type: 'ExtGState', | ||
| CA: 1, | ||
| ca: 1, |
There was a problem hiding this comment.
Inside docApplyAlphaMask, the inline comment We only support alpha mask for now has become misleading now that luminosity masks are also supported via docApplyMask. Please reword it to clarify this helper applies an alpha SMask specifically (rather than implying alpha is the only supported mask mode overall).
| iccProfile(label, data, channels, alternate, type) { | ||
| const profile = new ICCProfile(label, data, channels, alternate, type); | ||
| profile.embed(this); | ||
| this._colorProfiles[label] = profile; |
There was a problem hiding this comment.
The public iccProfile(...) API now accepts a new type parameter, but the TypeScript declarations still expose only 4 parameters (see types/pdfkit.d.ts). Please update the .d.ts signature to keep the public API consistent for typed consumers.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.