Premultiply alpha correctly in polar spaces, and handle none/zero alpha#754
Conversation
Premultiplication multiplied every coordinate by alpha, including the hue angle of polar spaces, which is meaningless. Per CSS Color 4 13.3, the hue angle is never premultiplied, and un-premultiplication must not divide by a zero or none alpha. - Add ColorSpace#hueId and ColorSpace#hueIndex, so the hue coordinate is looked up from the space definition rather than assumed to be named h. isPolar is now expressed in terms of hueId. - Skip the hue angle when premultiplying and when undoing premultiplication. - Leave the coords untouched when undoing premultiplication with an alpha of zero, rather than dividing by zero and producing NaN. - Carry a none alpha forward from the other color before premultiplying, per CSS Color 4 13.2, and leave alpha as none if both colors are missing it. - Fix util.interpolate, which tested isNaN for missing components. none is null here, not NaN, so a none endpoint was coerced to 0 instead of taking the other endpoint's value. As a result, a none alpha used to premultiply every coordinate to zero, turning the color black. Routing the hue lookup through hueId also fixes hue interpolation in Jzczhz, whose hue coordinate is named hz and so never matched the old check. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
✅ Deploy Preview for colorjs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Note that as a side effect, |
|
A test to verify this behavior would be good to ensure we don't ever break this in the future. |
Agreed. Note that this PR already fixes some silent breakage that we hadn't noticed and wasn't tested:
There are a bunch of examples in CSS Color 4, I was thinking of adapting them to our test framework. |
…nents Covers, in Oklch, Jzczhz, HSL, LCH, Lab and sRGB: - the hue angle is not premultiplied, and is not divided when premultiplication is undone - the hue coordinate is found from the space definition, so Jzczhz (hz) and HSL (hue first, not last) work too - un-premultiplying by a zero alpha does not divide by zero - a missing alpha is carried forward from the other color before premultiplying, and stays missing if both colors are missing it - a missing hue takes the other color's hue Expected values are hand-derived from CSS Color 4 13, or taken from the worked examples in 13.2 and 13.3. 18 of these 24 tests fail against the previous code. Missing components are compared strictly, because check.proximity() matches null against 0, which is the very confusion these tests are meant to catch. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Tests added, all the polar ones failed against the existing color.js and pass with this patch. |
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@facelessuser @LeaVerou tests added, plus ESLint and Prettier run (just whitespace changes) |
LeaVerou
left a comment
LeaVerou
left a comment
There was a problem hiding this comment.
LGTM. Note that it is baking an assumption in that any angular coordinate = hue, but that does seem to be true in practice.
I wonder if we should have an option to disable premultiplication? 🤔
Yes. I'm not aware of any real space with a non-hue angle. (Of course I could invent one, trivially).
Since the goal is full compliance with CSS Color 4 and 5, which does not make premultiplication optional, I don't see the benefit. Merge? |
3 participants
Premultiplication multiplied every coordinate by alpha, including the hue angle of polar spaces, which is meaningless. Per CSS Color 4 13.3, the hue angle is never premultiplied, and un-premultiplication must not divide by a zero or none alpha.
Routing the hue lookup through hueId also fixes hue interpolation in Jzczhz, whose hue coordinate is named hz and so never matched the old check.