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

Named color parsing #15128

Merged
merged 1 commit into from Sep 14, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
26 changes: 21 additions & 5 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions package.json
Expand Up @@ -44,6 +44,7 @@
"url": "https://opencollective.com/openlayers"
},
"dependencies": {
"color-name": "^2.0.0",
"earcut": "^2.2.3",
"geotiff": "^2.0.7",
"pbf": "3.2.1",
Expand Down
23 changes: 12 additions & 11 deletions src/ol/color.js
@@ -1,6 +1,7 @@
/**
* @module ol/color
*/
import colorNames from 'color-name';
import {clamp} from './math.js';

/**
Expand Down Expand Up @@ -42,20 +43,20 @@ export function asString(color) {
}

/**
* Return named color as an rgba string.
* Return named color as an rgb(a) string.
* @param {string} color Named color.
* @return {string} Rgb string.
* @return {string} RGB(A) string.
*/
function fromNamed(color) {
const el = document.createElement('div');
el.style.color = color;
if (el.style.color !== '') {
document.body.appendChild(el);
const rgb = getComputedStyle(el).color;
document.body.removeChild(el);
return rgb;
const name = color.toLowerCase();
if (!colorNames.hasOwnProperty(name)) {
if (name === 'transparent') {
return 'rgba(0,0,0,0)';
}
return '';
Copy link
Member

Choose a reason for hiding this comment

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

is this a good default value? It is a string but no valid rgb(a) string. most likely i would suspect this should not cause an error, because normally if colors are allowed, an empty strings is also ok.

Also this technically differs from the previous implementation because the browsers would actually accept any color name and just parse it randomly. I think this can be neglected as it is an unspecified case - also browsers handle this differently.

Copy link
Member

Choose a reason for hiding this comment

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

as this is used in fromStringInternal_ and not exported the empty string makes sense.

Copy link
Member Author

Choose a reason for hiding this comment

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

The previous behavior in this case was not really predictable. If you el.style.color = 'oops', then getComputedStyle(el).color depends on what other CSS rules might apply. For example, on this page, that returns rgb(31, 35, 40). On about:blank, it returns 'rgb(0, 0, 0)'. I thought that I previously saw a case where the result was '', but cannot reproduce that now.

I think that the new behavior with the exported methods is probably more "correct" now. Calling asArray('oops') now throws new Error('Invalid color') whereas previously I think the behavior would depend on CSS rules that might be on the page.

}
return '';
const rgb = colorNames[name];
return 'rgb(' + rgb[0] + ',' + rgb[1] + ',' + rgb[2] + ')';
}

/**
Expand Down Expand Up @@ -180,7 +181,7 @@ function fromStringInternal_(s) {
}

/**
* TODO this function is only used in the test, we probably shouldn't export it
* Exported for the tests.
* @param {Color} color Color.
* @return {Color} Clamped color.
*/
Expand Down
95 changes: 63 additions & 32 deletions test/browser/spec/ol/color.test.js → test/node/ol/color.test.js
@@ -1,85 +1,116 @@
import expect from '../expect.js';
import {
asArray,
asString,
fromString,
isStringColor,
normalize,
toString,
} from '../../../../src/ol/color.js';
} from '../../../src/ol/color.js';

describe('ol.color', function () {
describe('asArray()', function () {
it('returns the same for an array', function () {
describe('ol/color', () => {
describe('asArray()', () => {
it('returns the same for an array', () => {
const color = [1, 2, 3, 0.4];
const got = asArray(color);
expect(got).to.be(color);
});

it('returns an array given an rgba string', function () {
it('returns an array given an rgba string', () => {
const color = asArray('rgba(1,2,3,0.4)');
expect(color).to.eql([1, 2, 3, 0.4]);
});

it('returns an array given an rgb string', function () {
it('returns an array given an rgb string', () => {
const color = asArray('rgb(1,2,3)');
expect(color).to.eql([1, 2, 3, 1]);
});

it('returns an array given a hex string', function () {
it('returns an array given a hex string', () => {
const color = asArray('#00ccff');
expect(color).to.eql([0, 204, 255, 1]);
});

it('returns an array given a hex string with alpha', function () {
it('returns an array given a hex string with alpha', () => {
const color = asArray('#00ccffb0');
expect(color).to.eql([0, 204, 255, 176 / 255]);
});
});

describe('asString()', function () {
it('returns the same for a string', function () {
describe('asString()', () => {
it('returns the same for a string', () => {
const color = 'rgba(0,1,2,0.3)';
const got = asString(color);
expect(got).to.be(color);
});

it('returns a string given an rgba array', function () {
it('returns a string given an rgba array', () => {
const color = asString([1, 2, 3, 0.4]);
expect(color).to.eql('rgba(1,2,3,0.4)');
});

it('returns a string given an rgb array', function () {
it('returns a string given an rgb array', () => {
const color = asString([1, 2, 3]);
expect(color).to.eql('rgba(1,2,3,1)');
});
});

describe('fromString()', function () {
it('can parse 3-digit hex colors', function () {
describe('fromString()', () => {
describe('with named colors', () => {
const cases = [
['red', [255, 0, 0, 1]],
['green', [0, 128, 0, 1]],
['blue', [0, 0, 255, 1]],
['yellow', [255, 255, 0, 1]],
['orange', [255, 165, 0, 1]],
['purple', [128, 0, 128, 1]],
['violet', [238, 130, 238, 1]],
['white', [255, 255, 255, 1]],
['black', [0, 0, 0, 1]],
['wheat', [245, 222, 179, 1]],
['olive', [128, 128, 0, 1]],
['transparent', [0, 0, 0, 0]],
['oops', 'Invalid color'],
];
for (const c of cases) {
it(`works for ${c[0]}`, () => {
const expected = c[1];
if (typeof expected === 'string') {
expect(() => {
fromString(c[0]);
}).to.throwException(expected);
return;
Comment on lines +79 to +82
Copy link
Member

Choose a reason for hiding this comment

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

Why does this throw an exception? I would expect an empty string.

Copy link
Member

Choose a reason for hiding this comment

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

nevermind, this calls fromString not fromNamed

}
expect(fromString(c[0])).to.eql(c[1]);
});
}
});

it('can parse 3-digit hex colors', () => {
expect(fromString('#087')).to.eql([0, 136, 119, 1]);
});

it('can parse 4-digit hex colors', function () {
it('can parse 4-digit hex colors', () => {
expect(fromString('#0876')).to.eql([0, 136, 119, 102 / 255]);
});

it('can parse 6-digit hex colors', function () {
it('can parse 6-digit hex colors', () => {
expect(fromString('#56789a')).to.eql([86, 120, 154, 1]);
});

it('can parse 8-digit hex colors', function () {
it('can parse 8-digit hex colors', () => {
expect(fromString('#56789acc')).to.eql([86, 120, 154, 204 / 255]);
});

it('can parse rgb colors', function () {
it('can parse rgb colors', () => {
expect(fromString('rgb(0, 0, 255)')).to.eql([0, 0, 255, 1]);
});

it('ignores whitespace before, between & after numbers (rgb)', function () {
it('ignores whitespace before, between & after numbers (rgb)', () => {
expect(fromString('rgb( \t 0 , 0 \n , 255 )')).to.eql([0, 0, 255, 1]);
});

it('can parse rgba colors', function () {
it('can parse rgba colors', () => {
// opacity 0
expect(fromString('rgba(255, 255, 0, 0)')).to.eql([255, 255, 0, 0]);
// opacity 0.0 (simple float)
Expand Down Expand Up @@ -108,49 +139,49 @@ describe('ol.color', function () {
).to.eql([255, 255, 0, 0.12345678901234567890123456789]);
});

it('ignores whitespace before, between & after numbers (rgba)', function () {
it('ignores whitespace before, between & after numbers (rgba)', () => {
expect(fromString('rgba( \t 0 , 0 \n , 255 , 0.4711 )')).to.eql(
[0, 0, 255, 0.4711]
);
});

it('throws an error on invalid colors', function () {
it('throws an error on invalid colors', () => {
const invalidColors = ['tuesday', '#12345', '#1234567'];
let i, ii;
for (i = 0, ii = invalidColors.length; i < ii; ++i) {
expect(function () {
expect(() => {
fromString(invalidColors[i]);
}).to.throwException();
}
});
});

describe('normalize()', function () {
it('clamps out-of-range channels', function () {
describe('normalize()', () => {
it('clamps out-of-range channels', () => {
expect(normalize([-1, 256, 0, 2])).to.eql([0, 255, 0, 1]);
});

it('rounds color channels to integers', function () {
it('rounds color channels to integers', () => {
expect(normalize([1.2, 2.5, 3.7, 1])).to.eql([1, 3, 4, 1]);
});
});

describe('toString()', function () {
it('converts valid colors', function () {
describe('toString()', () => {
it('converts valid colors', () => {
expect(toString([1, 2, 3, 0.4])).to.be('rgba(1,2,3,0.4)');
});

it('rounds to integers if needed', function () {
it('rounds to integers if needed', () => {
expect(toString([1.2, 2.5, 3.7, 0.4])).to.be('rgba(1,3,4,0.4)');
});

it('sets default alpha value if undefined', function () {
it('sets default alpha value if undefined', () => {
expect(toString([0, 0, 0])).to.be('rgba(0,0,0,1)');
});
});

describe('isValid()', function () {
it('correctly detects valid colors', function () {
describe('isValid()', () => {
it('correctly detects valid colors', () => {
expect(isStringColor('rgba(1,3,4,0.4)')).to.be(true);
expect(isStringColor('rgb(1,3,4)')).to.be(true);
expect(isStringColor('lightgreen')).to.be(true);
Expand Down