Skip to content

Commit

Permalink
Reimplement color processing
Browse files Browse the repository at this point in the history
Summary:

**Problem:**

As I was trying to document what color formats we supported, I realized that our current implementation based on the open source project tinycolor supported some crazy things. A few examples that were all valid:

```
tinycolor('abc')
tinycolor(' #abc ')
tinycolor('##abc')
tinycolor('rgb 255 0 0')
tinycolor('RGBA(0, 1, 2)')
tinycolor('rgb (0, 1, 2)')
tinycolor('hsv(0, 1, 2)')
tinycolor({r: 10, g: 10, b: 10})
tinycolor('hsl(1%, 2, 3)')
tinycolor('rgb(1.0, 2.0, 3.0)')
tinycolor('rgb(1%, 2%, 3%)')
```

The integrations of tinycolor were also really bad. processColor added "support" for pure numbers and an array of colors!?? ColorPropTypes did some crazy trim().toString() and repeated a bad error message twice.

**Solution:**

While iteratively cleaning the file, I eventually ended up reimplementing it entierly. Major changes are:
- The API is now dead simple: returns null if it doesn't parse or returns the int32 representation of the color
- Stricter parsing of attributes. Instead of allowing all the arguments to be numbers with or without percentages, now you need to specify the correct type between integer, number and percentage.
- The internals only work on int32 numbers, there's no longer tons of object creations and copy
- The color map is now storing the computed value directly instead of parsing it over and over again
- Made the wrapper functions just delegate to tinycolor instead of implementing diverging logic
- Added unit tests on the allowed and not allowed formats
- Flowified the file
- Fixed a bug where hsl values with s === 0 would return the wrong result

**Risk:**

I'm confident that the values that were supported before still work fine. But there may be cases where people used an unsupported variant that is now going to redbox in dev and ignore the color in prod. I think that it's worth doing as what was supported was really clowny.

Test Plan:
Unit tests for now. I want to gather feedback and if people are happy with it I'll push it forward.
  • Loading branch information
vjeux committed Jan 27, 2016
1 parent 0998273 commit 7ee276c
Show file tree
Hide file tree
Showing 9 changed files with 521 additions and 578 deletions.
21 changes: 12 additions & 9 deletions Libraries/Animated/src/Interpolation.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,10 @@
* @providesModule Interpolation
* @flow
*/
/* eslint no-bitwise: 0 */
'use strict';

var tinycolor = require('tinycolor');
var normalizeColor = require('normalizeColor');

// TODO(#7644673): fix this hack once github jest actually checks invariants
var invariant = function(condition, message) {
Expand Down Expand Up @@ -164,16 +165,18 @@ function interpolate(
return result;
}

function colorToRgba(
input: string
): string {
var color = tinycolor(input);
if (color.isValid()) {
var {r, g, b, a} = color.toRgb();
return `rgba(${r}, ${g}, ${b}, ${a === undefined ? 1 : a})`;
} else {
function colorToRgba(input: string): string {
var int32Color = normalizeColor(input);
if (int32Color === null) {
return input;
}

var a = ((int32Color & 0xff000000) >>> 24) / 255;
var r = (int32Color & 0x00ff0000) >>> 16;
var g = (int32Color & 0x0000ff00) >>> 8;
var b = int32Color & 0x000000ff;

return `rgba(${r}, ${g}, ${b}, ${a})`;
}

var stringShapeRegex = /[0-9\.-]+/g;
Expand Down
14 changes: 7 additions & 7 deletions Libraries/Animated/src/__tests__/Interpolation-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
jest
.dontMock('Interpolation')
.dontMock('Easing')
.dontMock('tinycolor');
.dontMock('normalizeColor');

var Interpolation = require('Interpolation');
var Easing = require('Easing');
Expand Down Expand Up @@ -216,12 +216,12 @@ describe('Interpolation', () => {
it('should work with output ranges as string', () => {
var interpolation = Interpolation.create({
inputRange: [0, 1],
outputRange: ['rgba(0, 100, 200, 0)', 'rgba(50, 150, 250, 0.5)'],
outputRange: ['rgba(0, 100, 200, 0)', 'rgba(50, 150, 250, 0.4)'],
});

expect(interpolation(0)).toBe('rgba(0, 100, 200, 0)');
expect(interpolation(0.5)).toBe('rgba(25, 125, 225, 0.25)');
expect(interpolation(1)).toBe('rgba(50, 150, 250, 0.5)');
expect(interpolation(0.5)).toBe('rgba(25, 125, 225, 0.2)');
expect(interpolation(1)).toBe('rgba(50, 150, 250, 0.4)');
});

it('should work with output ranges as short hex string', () => {
Expand Down Expand Up @@ -249,11 +249,11 @@ describe('Interpolation', () => {
it('should work with output ranges with mixed hex and rgba strings', () => {
var interpolation = Interpolation.create({
inputRange: [0, 1],
outputRange: ['rgba(100, 120, 140, .5)', '#87FC70'],
outputRange: ['rgba(100, 120, 140, .4)', '#87FC70'],
});

expect(interpolation(0)).toBe('rgba(100, 120, 140, 0.5)');
expect(interpolation(0.5)).toBe('rgba(117.5, 186, 126, 0.75)');
expect(interpolation(0)).toBe('rgba(100, 120, 140, 0.4)');
expect(interpolation(0.5)).toBe('rgba(117.5, 186, 126, 0.7)');
expect(interpolation(1)).toBe('rgba(135, 252, 112, 1)');
});

Expand Down
2 changes: 1 addition & 1 deletion Libraries/ReactIOS/requireNativeComponent.js
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ var TypeToProcessorMap = {
CGColor: processColor,
CGColorArray: processColor,
UIColor: processColor,
UIColorArray: processColor,
UIColorArray: colors => colors && colors.map(processColor),
CGImage: resolveAssetSource,
UIImage: resolveAssetSource,
RCTImageSource: resolveAssetSource,
Expand Down
38 changes: 25 additions & 13 deletions Libraries/StyleSheet/ColorPropType.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,26 +9,38 @@
* @providesModule ColorPropType
*/
'use strict';

var ReactPropTypes = require('ReactPropTypes');
var tinycolor = require('tinycolor');

var colorValidator = function (props, propName) {
var normalizeColor = require('normalizeColor');

var ColorPropType = function(props, propName) {
var selectedColor = props[propName];
if (selectedColor === null || selectedColor === undefined || selectedColor.toString().trim() === '') {
return new Error(
`Invalid argument supplied to ${propName}.Expected a string like #123ADF or 'red'.`
);
if (selectedColor === undefined) {
return;
}

if (tinycolor(selectedColor.toString().trim()).isValid()) {
return null;
if (typeof selectedColor === 'number') {
// Developers should not use a number, but we are using the prop type
// both for user provided colors and for transformed ones. This isn't ideal
// and should be fixed but will do for now...
return;
}

return new Error(
`Invalid argument supplied to ${propName}.Expected a string like #123ADF or 'red'.`
);
if (normalizeColor(selectedColor) === null) {
return new Error(
`Invalid color supplied to ${propName}: ${selectedColor}. Valid color formats are
- #f0f (#rgb)
- #f0fc (#rgba)
- #ff00ff (#rrggbb)
- #ff00ff00 (#rrggbbaa)
- rgb(255, 255, 255)
- rgba(255, 255, 255, 1.0)
- hsl(360, 100%, 100%)
- hsla(360, 100%, 100%, 1.0)
- transparent
- red`);
}
};

var ColorPropType = ReactPropTypes.oneOfType([colorValidator, ReactPropTypes.number]);

module.exports = ColorPropType;
112 changes: 112 additions & 0 deletions Libraries/StyleSheet/__tests__/normalizeColor-test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,112 @@
/**
* Copyright (c) 2015-present, Facebook, Inc.
* All rights reserved.
*
* This source code is licensed under the BSD-style license found in the
* LICENSE file in the root directory of this source tree. An additional grant
* of patent rights can be found in the PATENTS file in the same directory.
*/
'use strict';

jest.dontMock('normalizeColor');

var normalizeColor = require('normalizeColor');

describe('normalizeColor', function() {
it('should accept only spec compliant colors', function() {
expect(normalizeColor('#abc')).not.toBe(null);
expect(normalizeColor('#abcd')).not.toBe(null);
expect(normalizeColor('#abcdef')).not.toBe(null);
expect(normalizeColor('#abcdef01')).not.toBe(null);
expect(normalizeColor('rgb(1,2,3)')).not.toBe(null);
expect(normalizeColor('rgb(1, 2, 3)')).not.toBe(null);
expect(normalizeColor('rgb( 1 , 2 , 3 )')).not.toBe(null);
expect(normalizeColor('rgb(-1, -2, -3)')).not.toBe(null);
expect(normalizeColor('rgba(0, 0, 0, 1)')).not.toBe(null);
});

it('should refuse non spec compliant colors', function() {
expect(normalizeColor('#00gg00')).toBe(null);
expect(normalizeColor('rgb(1, 2, 3,)')).toBe(null);
expect(normalizeColor('rgb(1, 2, 3')).toBe(null);

// Used to be accepted by normalizeColor
expect(normalizeColor('abc')).toBe(null);
expect(normalizeColor(' #abc ')).toBe(null);
expect(normalizeColor('##abc')).toBe(null);
expect(normalizeColor('rgb 255 0 0')).toBe(null);
expect(normalizeColor('RGBA(0, 1, 2)')).toBe(null);
expect(normalizeColor('rgb (0, 1, 2)')).toBe(null);
expect(normalizeColor('hsv(0, 1, 2)')).toBe(null);
expect(normalizeColor({r: 10, g: 10, b: 10})).toBe(null);
expect(normalizeColor('hsl(1%, 2, 3)')).toBe(null);
expect(normalizeColor('rgb(1.0, 2.0, 3.0)')).toBe(null);
expect(normalizeColor('rgb(1%, 2%, 3%)')).toBe(null);
});

it('should handle hex6 properly', function() {
expect(normalizeColor('#000000')).toBe(0xff000000);
expect(normalizeColor('#ffffff')).toBe(0xffffffff);
expect(normalizeColor('#ff00ff')).toBe(0xffff00ff);
expect(normalizeColor('#abcdef')).toBe(0xffabcdef);
expect(normalizeColor('#012345')).toBe(0xff012345);
});

it('should handle hex3 properly', function() {
expect(normalizeColor('#000')).toBe(0xff000000);
expect(normalizeColor('#fff')).toBe(0xffffffff);
expect(normalizeColor('#f0f')).toBe(0xffff00ff);
});

it('should handle hex8 properly', function() {
expect(normalizeColor('#00000000')).toBe(0x00000000);
expect(normalizeColor('#ffffffff')).toBe(0xffffffff);
expect(normalizeColor('#ffff00ff')).toBe(0xffffff00);
expect(normalizeColor('#abcdef01')).toBe(0x01abcdef);
expect(normalizeColor('#01234567')).toBe(0x67012345);
});

it('should handle rgb properly', function() {
expect(normalizeColor('rgb(0, 0, 0)')).toBe(0xff000000);
expect(normalizeColor('rgb(-1, -2, -3)')).toBe(0xff000000);
expect(normalizeColor('rgb(0, 0, 255)')).toBe(0xff0000ff);
expect(normalizeColor('rgb(100, 15, 69)')).toBe(0xff640f45);
expect(normalizeColor('rgb(255, 255, 255)')).toBe(0xffffffff);
expect(normalizeColor('rgb(256, 256, 256)')).toBe(0xffffffff);
});

it('should handle rgba properly', function() {
expect(normalizeColor('rgba(0, 0, 0, 0.0)')).toBe(0x00000000);
expect(normalizeColor('rgba(0, 0, 0, 0)')).toBe(0x00000000);
expect(normalizeColor('rgba(0, 0, 0, -0.5)')).toBe(0x00000000);
expect(normalizeColor('rgba(0, 0, 0, 1.0)')).toBe(0xff000000);
expect(normalizeColor('rgba(0, 0, 0, 1)')).toBe(0xff000000);
expect(normalizeColor('rgba(0, 0, 0, 1.5)')).toBe(0xff000000);
expect(normalizeColor('rgba(100, 15, 69, 0.5)')).toBe(0x80640f45);
});

it('should handle hsl properly', function() {
expect(normalizeColor('hsl(0, 0%, 0%)')).toBe(0xff000000);
expect(normalizeColor('hsl(360, 100%, 100%)')).toBe(0xffffffff);
expect(normalizeColor('hsl(180, 50%, 50%)')).toBe(0xff40bfbf);
expect(normalizeColor('hsl(540, 50%, 50%)')).toBe(0xff40bfbf);
expect(normalizeColor('hsl(70, 25%, 75%)')).toBe(0xffcacfaf);
expect(normalizeColor('hsl(70, 100%, 75%)')).toBe(0xffeaff80);
expect(normalizeColor('hsl(70, 110%, 75%)')).toBe(0xffeaff80);
expect(normalizeColor('hsl(70, 0%, 75%)')).toBe(0xffbfbfbf);
expect(normalizeColor('hsl(70, -10%, 75%)')).toBe(0xffbfbfbf);
});

it('should handle hsla properly', function() {
expect(normalizeColor('hsla(0, 0%, 0%, 0)')).toBe(0x00000000);
expect(normalizeColor('hsla(360, 100%, 100%, 1)')).toBe(0xffffffff);
expect(normalizeColor('hsla(360, 100%, 100%, 0)')).toBe(0x00ffffff);
expect(normalizeColor('hsla(180, 50%, 50%, 0.2)')).toBe(0x3340bfbf);
});

it('should handle named colors properly', function() {
expect(normalizeColor('red')).toBe(0xffff0000);
expect(normalizeColor('transparent')).toBe(0x00000000);
expect(normalizeColor('peachpuff')).toBe(0xffffdab9);
});
});
24 changes: 0 additions & 24 deletions Libraries/StyleSheet/__tests__/processColor-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -49,12 +49,6 @@ describe('processColor', () => {
expect(colorFromString).toEqual(expectedInt);
});

it('should convert rgb x, y, z', () => {
var colorFromString = processColor('rgb 10, 20, 30');
var expectedInt = 0xFF0A141E;
expect(colorFromString).toEqual(expectedInt);
});

});

describe('RGBA strings', () => {
Expand All @@ -65,12 +59,6 @@ describe('processColor', () => {
expect(colorFromString).toEqual(expectedInt);
});

it('should convert rgba x, y, z, a', () => {
var colorFromString = processColor('rgba 10, 20, 30, 0.4');
var expectedInt = 0x660A141E;
expect(colorFromString).toEqual(expectedInt);
});

});

describe('HSL strings', () => {
Expand All @@ -81,12 +69,6 @@ describe('processColor', () => {
expect(colorFromString).toEqual(expectedInt);
});

it('should convert hsl x, y%, z%', () => {
var colorFromString = processColor('hsl 318, 69%, 55%');
var expectedInt = 0xFFDB3DAC;
expect(colorFromString).toEqual(expectedInt);
});

});

describe('HSL strings', () => {
Expand All @@ -97,12 +79,6 @@ describe('processColor', () => {
expect(colorFromString).toEqual(expectedInt);
});

it('should convert hsla x, y%, z%, a', () => {
var colorFromString = processColor('hsla 318, 69%, 55%, 0.25');
var expectedInt = 0x40DB3DAC;
expect(colorFromString).toEqual(expectedInt);
});

});

describe('hex strings', () => {
Expand Down
Loading

0 comments on commit 7ee276c

Please sign in to comment.