Skip to content

hslToRgb function is wrong - Proposed fix. #938

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
ShanonJackson opened this issue Feb 26, 2020 · 1 comment
Closed

hslToRgb function is wrong - Proposed fix. #938

ShanonJackson opened this issue Feb 26, 2020 · 1 comment
Labels
kind: bug Something isn't working

Comments

@ShanonJackson
Copy link

ShanonJackson commented Feb 26, 2020

🐛 Bug Report

The function that converts hslToRgb in this library looks like it originates from this stackoverflow answer:
https://stackoverflow.com/questions/2353211/hsl-to-rgb-color-conversion
However, This answer is wrong this method of converting hsl to rgb is fundamentally flawed the correct way is documented here
https://www.rapidtables.com/convert/color/hsl-to-rgb.html
in which a code-translation version of the math turns into the refined more accurate version:

function hslToRgb(h: number, s: number, l: number): [number, number, number] {
		const C = (1 - Math.abs(2 * l - 1)) * s;
		const X = C * (1 - Math.abs(((h / 60) % 2) - 1));
		const M = l - C / 2;
		const hueToRgb = () => {
			if (h < 60) return [C, X, 0];
			if (h < 120) return [X, C, 0];
			if (h < 180) return [0, C, X];
			if (h < 240) return [0, X, C];
			if (h < 300) return [X, 0, C];
			return [C, 0, X];
		};
		const [R1, G1, B1] = hueToRgb();
		return [Math.round((R1 + M) * 255), Math.round((G1 + M) * 255), Math.round((B1 + M) * 255)];
	}

hslToRgb(158, 0.83, 0.46)// [20, 215, 143] (new way)
hslToRgbOldWay(158, 0.83, 0.46) // [20, 20, 20] (old way);

To Reproduce

No steps needed to reproduce other than check the inputs/outputs of that function against any reputable online color converter.

Expected behavior

Expected behavior is that it swaps between hsl / rgb in an extremely "non-lossy" way

Link to repro

Source of problem is here.
https://github.com/react-spring/react-spring/blob/e73385837e7a75ef64e82d17a28e7c86fb5c0e49/src/shared/normalizeColors.ts

Environment

react-spring any version.

EDIT: Quick edit to say if this could be fixed on react-spring 8 aswell as 9 that would be amazing

@ShanonJackson ShanonJackson added the kind: bug Something isn't working label Feb 26, 2020
@ShanonJackson
Copy link
Author

Also ran some common sense checks against a known source of truth (the DOM) to double check the accuracy; Didn't run many of these but just a few just to make sure it wasn't unnecessarily lossy.

// Native DOM
const ele = document.createElement("div")
ele.style.color = "hsl(158, 83%, 46%)"
console.log(ele.style.color) // rgb(20,215,143)     (converted by DOM automatically)

// New HSL -> RGB javascript algorithm
hslToRgb(158, 0.83, 0.46)/
// [20, 215, 143]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants