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

Use color property whitelist #223

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@tdewolff
Copy link
Owner

commented Aug 25, 2018

Fixes #217

Uses a whitelist to employ color transformations (name/hex to hex). Probably missing some properties in the whitelist, need to do some tests on large files.

@coveralls

This comment has been minimized.

Copy link

commented Aug 25, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling 6d29438 on css-color-whitelist into 69886b4 on master.

if data[7] == 'f' {
data = data[:7]
} else if data[7] == '0' {
data = transparentBytes

This comment has been minimized.

Copy link
@RReverser

RReverser Aug 30, 2018

Contributor

KeepCSS2 condition seems missing.

data[2] = data[3]
data[3] = data[5]
data = data[:4]
} else if len(data) == 9 && data[1] == data[2] && data[3] == data[4] && data[5] == data[6] && data[7] == data[8] {

This comment has been minimized.

Copy link
@RReverser

RReverser Aug 30, 2018

Contributor

Wonder if this condition could/should be somehow nested into previous as most conditions and operations are shared.

@RReverser
Copy link
Contributor

left a comment

Should preserve KeepCSS2 (transparentBytes can't be used in that context) and have more tests for different cases (KeepCSS2, identifiers that look like colours but aren't, etc.).

@@ -155,7 +155,7 @@ func TestCSSInline(t *testing.T) {
{"content: \"a\\\nb\";", "content:\"ab\""},
{"content: \"a\\\r\nb\\\r\nc\";", "content:\"abc\""},
{"content: \"\";", "content:\"\""},
{"x: white , white", "x:#fff,#fff"},
{"color: white , white", "color:#fff,#fff"},

This comment has been minimized.

Copy link
@RReverser

RReverser Aug 31, 2018

Contributor

Could you add some tests that make sure color-like identifier are preserved in other contexts? (e.g. Apple Pay from original report)

This comment has been minimized.

Copy link
@RReverser

RReverser Aug 31, 2018

Contributor

Same for KeepCSS2 tests.

@tdewolff

This comment has been minimized.

Copy link
Owner Author

commented Dec 10, 2018

Not sure if this PR moves into the right direction. We probably want better handling of value types (color, time, length, ...) depending on the declaration name in general, not just for color. White listing an entire declaration dictates nothing about the specific values it may have (maybe the first value is a color but the second is not). See #234 on plans to handling different value types.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.