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

Fixes Tint, Shade, and Contrast Alpha Interference Issue #26

Merged
merged 4 commits into from Jul 15, 2017

Conversation

Projects
None yet
4 participants
@tylergaw
Collaborator

tylergaw commented Jan 9, 2017

The problem

The tint, shade, and contrast adjusters incorrectly modify the alpha value of the base color.

Current results:

convert('red a(40%) tint(50%)') // rgba(255, 204, 204, 0.7)
convert('red a(40%) shade(50%)') // rgba(51, 0, 0, 0.7)
convert('red a(40%) contrast(99%)') // rgba(0, 0, 0, 0.994)

Expected results:

convert('red a(40%) tint(50%)') // rgba(255, 128, 128, 0.4)
convert('red a(40%) shade(50%)') // rgba(128, 0, 0, 0.4)
convert('red a(40%) contrast(99%)') // rgba(0, 0, 0, 0.4)

From usage and reading the spec, I expected tint, shade, and contrast to leave the alpha value of the base color as is.

The cause

These three adjusters make use of Color.mix. Tint and shade do so here https://github.com/ianstormtaylor/css-color-function/blob/master/lib/adjusters.js#L41 and contrast here https://github.com/ianstormtaylor/css-color-function/blob/master/lib/adjusters.js#L82

Looking at Qix-/color, the mix method is ported from the Sass mix https://github.com/Qix-/color/blob/0.11.3/index.js#L295 According to the Sass docs http://sass-lang.com/documentation/Sass/Script/Functions.html#mix-instance_method the mix method mixes color channels and the alpha channel.

Mixes two colors together. Specifically, takes the average of each of the RGB components, optionally weighted by the given percentage. The opacity of the colors is also considered when weighting the components.
(bolding is mine)

Reading through the Color Level 4 spec, it seems like the only adjusters that should modify the alpha value of the base color are alpha and blenda.

My solution in this PR

First, I wrote failing tests for tint, shade, and contrast. A thing to note about both Tint and Shade is that order mattered. If tint/shade came before alpha, the expected result was produced. If alpha came first, the unexpected value was produced.

Code changes; What I did in both the blend method and within the contrast method was hold on to the initial alpha value of the base color. Then set the alpha to 1, then call Color.mix. After the mix was complete, set the alpha back to the initial alpha value.

This ensures Color.mix does not attempt to mix an alpha value less than one.

Background
I first hit this issue in tylergaw/colorme#3. Found it first in the colorme.io UI just from usage. Fiddled with the adjusters enough and found the pattern. From there, I just followed the thread until I bumped into the Color.mix usage.

I have a PR in for colorme pointing it to my branch of css-color-function tylergaw/colorme#6 for now. And that branch is deployed at https://colorme.io

Thanks, let me know if there's anything else you want me to tweak.

@MoOx

This comment has been minimized.

Collaborator

MoOx commented Feb 1, 2017

@Semigradsky

This comment has been minimized.

Semigradsky commented May 12, 2017

@ianstormtaylor any progress?

@MoOx

This comment has been minimized.

Collaborator

MoOx commented May 12, 2017

@ianstormtaylor I guess you are super busy and you maybe don't do css anymore. Could you add collaborator to this module and transfert it maybe?

@ianstormtaylor

This comment has been minimized.

Owner

ianstormtaylor commented May 14, 2017

Oh shoot for some reason I thought that you were a collaborator on this one already @MoOx! I'm definitely happy to add you and anyone else who wants to maintain it.

@ianstormtaylor

This comment has been minimized.

Owner

ianstormtaylor commented May 14, 2017

Just added @MoOx and @tylergaw as collaborators and GitHub and as owners on NPM, so you should have full access to push changes. Let me know if you need anything else!

@tylergaw

This comment has been minimized.

Collaborator

tylergaw commented May 14, 2017

Excellent, thanks @ianstormtaylor.

I need to take another look at this to refresh my memory on what I was up to. But @MoOx you wanna give this another look and a 👍 if it's ready to go? Anyone else too, if you see anything that needs fixin', holler

@MoOx

This comment has been minimized.

Collaborator

MoOx commented Jul 4, 2017

My time is limited but I gave a quick look: it seems you know what you are talking about, so I will go for merge if you are still confident that this PR is a proper bugfix.

@tylergaw

This comment has been minimized.

Collaborator

tylergaw commented Jul 15, 2017

Finally had a look at this to get myself back up to speed. All looks good. Tests still passing. Gonna merge.

@tylergaw tylergaw merged commit 7ab4d00 into ianstormtaylor:master Jul 15, 2017

@tylergaw tylergaw deleted the tylergaw:tg-ignore-alpha-on-mix branch Jul 15, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment