Skip to content

feat: Switch to culori color library - #7962

Merged
camdecoster merged 20 commits into
v4.0from
cam/7961/switch-to-culori-library
Aug 17, 2026
Merged

feat: Switch to culori color library#7962
camdecoster merged 20 commits into
v4.0from
cam/7961/switch-to-culori-library

Conversation

@camdecoster

@camdecoster camdecoster commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Description

Switch to the culori color processing library.

Closes #7961.

Changes

  • Remove color library
  • Add culori library
  • Update color functions per library change
  • Update call sites
  • Update and add tests

Screenshots

color_syntax_formats mock results

Before After
image image

Testing

  • Check CI results
  • Review the color_syntax_formats mock and try changing some of the color specifiers to valid/invalid values
  • Try entering some of the color specifiers from the mock into an element through browser devtools and see how it compares to the after results

Notes

  • This library bumps up the bundle size a bit (+17KB gzipped) but it's worth the added CSS 4 compatibility
  • We can mitigate this in the future by switching to ESM for this file (so it can be tree-shaken)
  • I removed the color-normalize dependency in favor of managing that internally

@camdecoster
camdecoster marked this pull request as ready for review August 14, 2026 17:35

nodes.each(function (_, i) {
const { fill } = this.style;
const other = Color.equals(fill, Color.background) ? Color.defaultLine : Color.background;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const other = Color.equals(fill, Color.background) ? Color.defaultLine : Color.background;
// Ensure text fill color matches either Color.background or Color.defaultLine
expect([Color.background, Color.defaultLine]).toContain(fill);
const other = Color.equals(fill, Color.background) ? Color.defaultLine : Color.background;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch.

Comment thread test/jasmine/tests/axes_test.js Outdated
Comment on lines +570 to +572
// A translucent axis color is what exercises the rule: the channel weight
// scales by the alpha difference, so mixing toward an opaque background
// moves the channels less than a plain interpolation would.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// A translucent axis color is what exercises the rule: the channel weight
// scales by the alpha difference, so mixing toward an opaque background
// moves the channels less than a plain interpolation would.
// axis.gridcolor is determined by mixing the axis color with the paper and plot background colors.
// If the axis color contains an alpha channel, its weight should scale by the alpha value, so that
// more-transparent colors are weighted less.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.

};

supplyLayoutDefaults(layoutIn, layoutOut, fullData);
expect(layoutOut.xaxis.gridcolor).toEqual('rgba(255, 247, 0, 0.95)');

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add a comment here stating what gridcolor we would expect if the weight was not scaled by alpha?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's reasonable. I'll add it.

Comment on lines +22 to +23
Color.fill({ style: (o) => seen.push(o) }, undefined); // Mock the selection to track it's call

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Color.fill({ style: (o) => seen.push(o) }, undefined); // Mock the selection to track it's call
Color.fill({ style: (o) => seen.push(o) }, undefined);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to leave this one because I think it's non-obvious as to why the object is being passed in.

Comment on lines +42 to +43
Color.stroke({ style: (o) => seen.push(o) }, undefined); // Mock the selection to track it's call

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Color.stroke({ style: (o) => seen.push(o) }, undefined); // Mock the selection to track it's call
Color.stroke({ style: (o) => seen.push(o) }, undefined);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to leave this one because I think it's non-obvious as to why the object is being passed in.

Comment thread test/jasmine/tests/color_test.js Outdated
Comment on lines +95 to +96
const other = Color.equals(picked, Color.background) ? Color.defaultLine : Color.background;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const other = Color.equals(picked, Color.background) ? Color.defaultLine : Color.background;
// Ensure selected color matches either Color.background or Color.defaultLine
expect([Color.background, Color.defaultLine]).toContain(picked);
const other = Color.equals(picked, Color.background) ? Color.defaultLine : Color.background;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.

Comment thread test/jasmine/tests/color_test.js Outdated
Comment on lines +115 to +117
// Drawing code needs the alpha of a color it is about to paint, which is
// not the same question `opacity` answers. A color that is simply unset
// still gets painted, so it resolves to opaque black.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Drawing code needs the alpha of a color it is about to paint, which is
// not the same question `opacity` answers. A color that is simply unset
// still gets painted, so it resolves to opaque black.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.

Comment thread test/jasmine/tests/color_test.js Outdated
// Drawing code needs the alpha of a color it is about to paint, which is
// not the same question `opacity` answers. A color that is simply unset
// still gets painted, so it resolves to opaque black.
it('treats a missing color as opaque black, without warning', () => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The description implies that the test checks all channels of the color, but it only checks the alpha. Either the description or the test should be updated (I don't think it checks for a warning either, although maybe our test infrastructure does surface warnings).

Same for the next test.

Suggested change
it('treats a missing color as opaque black, without warning', () => {
it('treats a missing color as opaque', () => {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to update these to be less focused on alpha.

});
});

describe('parse', () => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly all of these parse tests could probably be rolled into other sections

Comment thread test/jasmine/tests/color_test.js Outdated
BAD.forEach((v) => expect(Color.isValid(v)).toBe(false));
});

// Null channels used to reach the WebGL buffers through this path.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Null channels used to reach the WebGL buffers through this path.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.

it('sets alpha', () => {
expect(Color.addOpacity('red', 0.5)).toBe('rgba(255, 0, 0, 0.5)');
expect(Color.addOpacity('rgba(255, 0, 0, 0.5)', 1)).toBe('rgb(255, 0, 0)');
expect(Color.addOpacity('red', 2)).toBe('rgb(255, 0, 0)');

@emilykl emilykl Aug 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No opacity is added here: is that because 2 is an invalid value so the function does nothing, or because a ceiling is applied so that 2 becomes 1?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, alpha is clamped to the [0, 1] range. Since 2 is greater than 1, 1 is passed into the formatter. culori drops alpha in this case, so rgb is returned.

Comment thread test/jasmine/tests/colorscale_test.js Outdated
Comment on lines +769 to +772
expect(fills.length).toBe(5);
expect(fills.every((f) => f === 'rgb(0, 0, 0)')).toBe(false, 'all points black');
expect(new Set(fills).size).toBeGreaterThan(1, 'every point the same color');
fills.forEach((f) => expect(f).toMatch(/^rgba?\(/, `not a usable color: ${f}`));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of all this, could you just expect(fills).toBe(...) and copy the actual array expected for the Viridis colorscale?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, that seems more reasonable.

Comment thread src/components/color/index.js Outdated
Comment on lines +36 to +43
* Parse a color specifier, falling back to opaque black.
*
* A missing color falls back quietly, because it means the caller left the
* attribute unset rather than gave a bad value. Callers that treat a missing
* color as nothing to paint test for it themselves, as `opacity` does.
*
* @param {*} cstr - color specifier
* @return {Color} color object
* @param {Boolean} [silent] - skip the warning, for callers that run per data point

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* Parse a color specifier, falling back to opaque black.
*
* A missing color falls back quietly, because it means the caller left the
* attribute unset rather than gave a bad value. Callers that treat a missing
* color as nothing to paint test for it themselves, as `opacity` does.
*
* @param {*} cstr - color specifier
* @return {Color} color object
* @param {Boolean} [silent] - skip the warning, for callers that run per data point
* Parse a color specifier string and return it as a culori rgb color object.
* If the input is not a string or cannot be parsed, fall back to opaque black (#fff).
*
* @param {String} cstr - color specifier
* @param {Boolean} [silent] - if true, do not emit a warning for un-parseable colors

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.

Comment on lines 60 to +61
* Convert any color specifier to a normalized `rgb(r, g, b)` string.
* Force alpha to 1 so that it gets dropped in the result.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this function calls parse(), that means any invalid input will return rgb(0, 0, 0), right?

Might be worth putting that info in the docstring.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair point, but I think it makes more sense to leave that note in the docstring for parse. It seems better to keep it there since so many functions call parse.

Comment thread src/components/color/index.js Outdated
* Return the alpha channel of a color (0 if falsy).
*
* @param {*} cstr - color specifier
* @return {Number}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the range for the number, is it [0, 1]?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, culori clamps to that range.

Comment thread src/components/color/index.js Outdated
* @param {*} cstr - color specifier
* @return {Color} color object
* @param {Boolean} [silent] - skip the warning, for callers that run per data point
* @return {Object} culori rgb color

@emilykl emilykl Aug 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A culori rgb object is just an object that looks like

{ mode: 'rgb', r: _, g: _, b: _, alpha: _ }

right? Maybe add that info to the docstring since parse() is used everywhere.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added.

Comment thread src/components/color/index.js Outdated
/**
* Convert a color specifier to a 4-element `[r, g, b, a]` representation.
* Accepts strings, numeric float arrays (`[0, 1]`), or uint8 arrays (`[0, 255]`).
* Falls back to opaque black rather than null: WebGL paths index the result.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* Falls back to opaque black rather than null: WebGL paths index the result.
* Returns opaque black ([0, 0, 0, 0]) if color specifier is invalid.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I opted to add a note at the top of the file and remove this one because most of the functions would get the same note.

Comment thread src/components/color/index.js Outdated
* @param {*} cstr - color specifier
* @param {Number} op - opacity in [0, 1]
* @return {String}
* @param {Number} op - opacity in [0, 1], clipped to that range

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clipped to that range

Unclear — does this mean the input must be clipped to [0, 1] before passing to this function, or that if it's outside [0, 1] this function will clip it?

In any case, I see the clipping for values above 1, but what happens when the value is below 0?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll update the description to be clearer. If the value is below zero, it gets set to zero. I'm going to rename the function to clamp01.

Comment thread src/components/color/index.js Outdated

/**
* Combine two colors into one apparent color by compositing `front` over `back`.
* If `back` is missing or transparent, the module `background` is assumed behind it.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* If `back` is missing or transparent, the module `background` is assumed behind it.
* If `back` is missing, the module `background` is assumed behind it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.

Comment thread src/components/color/index.js Outdated
* @param {*} cstr - color specifier
* @param {Number} delta - lightness shift in HSL percentage points
* @return {Color} adjusted color object
* @return {String} resulting color string

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* @return {String} resulting color string
* @return {String} resulting color string as rgb

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.

Comment thread src/components/color/index.js Outdated
Comment on lines +216 to +224
const newColor = isDark(cstr)
? lightAmount
? adjustLightness(c, lightAmount)
: color(background)
? adjustLightness(cstr, lightAmount)
: background
: darkAmount
? adjustLightness(c, -darkAmount)
: color(defaultLine);
? adjustLightness(cstr, -darkAmount)
: defaultLine;

return newColor.rgb().string();
return formatRgb(parse(newColor));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If newColor is the result of adjustLightness(), then it's already a formatted rgb string and the extra round-trip through formatRgb(parse(...)) is unnecessary, right?

Suggested change
const newColor = isDark(cstr)
? lightAmount
? adjustLightness(c, lightAmount)
: color(background)
? adjustLightness(cstr, lightAmount)
: background
: darkAmount
? adjustLightness(c, -darkAmount)
: color(defaultLine);
? adjustLightness(cstr, -darkAmount)
: defaultLine;
return newColor.rgb().string();
return formatRgb(parse(newColor));
const newColor = isDark(cstr)
? lightAmount
? adjustLightness(cstr, lightAmount)
: formatRgb(parse(background));
: darkAmount
? adjustLightness(cstr, -darkAmount)
: formatRgb(parse(defaultLine));
return newColor;

Honestly it would probably make sense to convert the constants to RGB just once at the top of the file and reuse them.

const backgroundRGB = formatRgb(parse(background))
const defaultLineRGB = formatRgb(parse(defaultLine))

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like your first suggestion (and I'm going to get rid of the nested ternary). I'll add the constants as well. This is the only place where backgroundRGB/defaultLineRGB would get used, but they'll get used a lot so we might as well save the function calls.

Comment thread src/components/color/index.js Outdated
Comment on lines +230 to +232
* A missing color paints opaque black. Shapes and annotations leave
* `line.color` unset when the user gives none, and the outline still has to
* show. Use `opacity` instead when a missing color means "nothing to paint".

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* A missing color paints opaque black. Shapes and annotations leave
* `line.color` unset when the user gives none, and the outline still has to
* show. Use `opacity` instead when a missing color means "nothing to paint".
* A missing or invalid color specifier applies opaque black.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What you deleted is out of date, so I removed it. I also removed the note about black in favor of the top level comment.

Comment thread src/components/color/index.js Outdated
/**
* Apply `fill` and `fill-opacity` styles to a D3 selection.
*
* A missing color paints opaque black, the same as `stroke`.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* A missing color paints opaque black, the same as `stroke`.
* A missing or invalid color specifier applies opaque black.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed the comment in favor of the module note at the top of the file.

Comment on lines +308 to +309
const w = 2 * p - 1;
const w2 = ((w * d === -1 ? w : (w + d) / (1 + w * d)) + 1) / 2;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sure this is fine, but do you know where these equations come from?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It came from Qix-/color, which got it from SASS. I'll add a note.

};

/**
* Convert any color specifier to an `rgb(...)` or `rgba(...)` string,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably all of these functions should specify that passing an invalid color specifier returns black.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The top level comment will cover these.

Comment thread src/components/color/index.js Outdated
Comment on lines +363 to +365
* Channels as `[r, g, b, a]`, with `r`/`g`/`b` in [0, 255] and `a` in [0, 1].
* An array rather than an object so callers cannot depend on the color library's
* shape. Unrounded, since callers do further arithmetic.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* Channels as `[r, g, b, a]`, with `r`/`g`/`b` in [0, 255] and `a` in [0, 1].
* An array rather than an object so callers cannot depend on the color library's
* shape. Unrounded, since callers do further arithmetic.
* Returns the given color specifier as an `[r, g, b, a]` array,
* with `r`/`g`/`b` in [0, 255] and `a` in [0, 1].

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.

}
// `toRgb` omits alpha when it's 1; make sure it's added since we expect it
c.alpha ??= 1;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any reason not to clip each of the r, g, b values before returning the color object?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It originally got clipped here, but it was causing some issues. I moved the clipping to where it was needed.

Comment thread src/lib/gl_format_color.js Outdated
var colorOut = rgba(colorIn);
// A per-point color may be raw channels rather than a color string, which
// `Color.isValid` rejects but `Color.normalize` handles.
if (!isArrayOrTypedArray(colorIn) && !Color.isValid(colorIn)) return colorDfltRgba;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to use Color.isChannelArray() here?

Suggested change
if (!isArrayOrTypedArray(colorIn) && !Color.isValid(colorIn)) return colorDfltRgba;
if (!Color.isChannelArray(colorIn) && !Color.isValid(colorIn)) return colorDfltRgba;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But actually this logic feels like a code smell. How about adding a parameter arrayAllowed to Color.isValid() so that you could write

if (!Color.isValid(colorIn, true)) return colorDfltRgba;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to keep isValid specific to color strings because it gets used in the coerce test. I'll use your first suggestion. validateColor is essentially a wrapper that does what you're suggesting we add to isValid. If this becomes a more common pattern, we can update isValid at that time.

@emilykl emilykl left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a bunch of comments, but nothing blocking 🌈

@camdecoster
camdecoster merged commit 635182a into v4.0 Aug 17, 2026
84 of 85 checks passed
@camdecoster
camdecoster deleted the cam/7961/switch-to-culori-library branch August 17, 2026 22:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants