Skip to content
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

Percentages mishandled in color(lab L a b) #70

Open
svgeesus opened this issue Feb 5, 2021 · 7 comments
Open

Percentages mishandled in color(lab L a b) #70

svgeesus opened this issue Feb 5, 2021 · 7 comments
Assignees
Labels
bug Something isn't working

Comments

@svgeesus
Copy link
Member

svgeesus commented Feb 5, 2021

In color(lab L a b), same as lab(L a b), the L parameter is required to have a percentage and 50% resolves to an L value of 50, not 0.5.

However, color.js has two errors:

a) it allows the "%" to be omitted, for examplecolor(lab 54.903 -54.15 55.313)

b) if the percent is used, 100% resolves to an L of 1, not 100. For example color(lab 54.903% -54.15 55.313) is a near-black.

Note that lab(54.903% -54.15 55.313) is handled correctly. Although lab(54.903 -54.15 55.313) is also accepted, and treated as identical to the percentage form.

@svgeesus svgeesus added the bug Something isn't working label Feb 5, 2021
@svgeesus svgeesus self-assigned this Feb 5, 2021
@LeaVerou LeaVerou self-assigned this Feb 5, 2021
@svgeesus
Copy link
Member Author

svgeesus commented Feb 6, 2021

I suspect the problem is here

if (/%$/.test(arg)) {
	// Convert percentages to 0-1 numbers
	let n = new Number(+arg.slice(0, -1) / 100);
	n.percentage = true;
	return n;
}

but then that would also affect the lab() and lch() functions. Ah, they have special handling:

	let L = parsed.args[0];

	// Percentages in lab() don't translate to a 0-1 range, but a 0-100 range
	if (L.percentage) {
		parsed.args[0] = L * 100;
	}

so that same test for "is the first argument CIE L" needs to be applied to the general color() parsing in src/color.js

@svgeesus
Copy link
Member Author

svgeesus commented Apr 3, 2021

Confirming bug still exists

color(lab 50% 0 0) → color(lab 0.5 0 0)

@LeaVerou
Copy link
Member

Since color(lab L a b) is not valid CSS anymore, perhaps we should just not accept it…

@svgeesus
Copy link
Member Author

Agreed, provided lab(number number number) and lab(percent number number) are handled correctly with L=100% mapping to L=100 and vice-versa.

(There is also lab(percent percent percent) from CSS, but that is unrelated to this issue)

@LeaVerou LeaVerou reopened this Jun 17, 2022
@LeaVerou
Copy link
Member

LeaVerou commented Jun 17, 2022

Reopening because we do still accept it 😛 see #146

@svgeesus
Copy link
Member Author

Note that lab(54.903% -54.15 55.313) is handled correctly. Although lab(54.903 -54.15 55.313) is also accepted, and treated as identical to the percentage form.

Note that CSS Color 4 has changed and using a lightness without a percent is now allowed with 0 being the same as 0% and 100 the same as 100%. So that part is now correct.

We don't accept percent for a and b though, which is also now allowed.

@svgeesus
Copy link
Member Author

Ah, they have special handling:

The reference ranges for all color spaces that accept <percentage> should be part of the color space definition, not special-cased into the code like this.

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

No branches or pull requests

2 participants