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

RGBA and HSLA are compiled incorrectly #116

Closed
livtanong opened this issue Aug 22, 2016 · 27 comments
Closed

RGBA and HSLA are compiled incorrectly #116

livtanong opened this issue Aug 22, 2016 · 27 comments
Assignees
Labels
Milestone

Comments

@livtanong
Copy link

livtanong commented Aug 22, 2016

e.g.

{:box-shadow [[0 0 (px 8) (rgba 0 0 0 0.2)]]}

is converted to:

box-shadow: 0 0 8px hsla(0,0,0,0.2);
@noprompt
Copy link
Owner

I think that was intentional and, I do admit, is probably surprising. It's been a while but I think my rational was that rgba and hsla, when supported, are typically in supported tandem by the browser and that HSLA is a simpler model to understand with respect to color than RGBA.

The RGBA model, in my opinion, places more of a cognitive load on the reader since they need to mix the colors in their head. The HSLA model on the other hand only requires an understanding of the color wheel it's based on, the angle (modulo 360), and then adjustment of saturation. IIRC most of the color operations are computed in terms of HSL because of it's simplicity.

Again, this is my personal opinion and I understand if it's confusing or surprising. If it's important the design can be adjusted to persist a rgba instead of hsla.

@ejlo
Copy link

ejlo commented Aug 31, 2016

Saturation and lightness should be percentages, though. It seems the % is dropped when the value is zero, which creates an invalid property value.

@noprompt
Copy link
Owner

@ejlo I'm not certain that it is invalid. My memory could be off but I don't recall that being the case in the CSS spec.

@noprompt
Copy link
Owner

@ejlo See here.

em { color: rgb(255,0,0) }      /* integer range 0 - 255 */
em { color: rgba(255,0,0,1)     /* the same, with explicit opacity of 1 */
em { color: rgb(100%,0%,0%) }   /* float range 0.0% - 100.0% */
em { color: rgba(100%,0%,0%,1) } /* the same, with explicit opacity of 1 */

It isn't clear to me if 0 and 0% are legally interchangeable with respect to the spec or if it is something left up to browsers to handle. The fact the YUI compressor will convert 0% to 0 does give me some confidence that this shouldn't be a problem.

@livtanong
Copy link
Author

livtanong commented Sep 1, 2016

@noprompt Chrome is complaining though. Perhaps it's an issue specifically with box-shadow?

screen shot 2016-09-01 at 9 51 07 am

@noprompt
Copy link
Owner

noprompt commented Sep 1, 2016

@levitanong No, I just confirmed this by editing a style on the page. It definitely looks like we need to emit 0% here.

@ejlo
Copy link

ejlo commented Sep 1, 2016

The spec seems to indicate that hsl is different than rgb.

For rgb you can have either integers or percentages (but all three values must be of the same type), whereas for hsl the first (hue) is an integer, but the other two should be percentages.

@noprompt
Copy link
Owner

noprompt commented Sep 1, 2016

@ejlo Yeah, I suspected as much. I'll convert this issue to a bug and fix it as fast as I can.

@levitanong Thanks for reporting this.

@noprompt noprompt added the bug label Sep 1, 2016
@noprompt noprompt changed the title rgba is compiled to hsla RGBA and HSLA are compiled incorrectly Sep 1, 2016
@noprompt noprompt self-assigned this Sep 1, 2016
@livtanong
Copy link
Author

@noprompt My pleasure :)

@thedavidmeister
Copy link

i've seen the "hsl is simpler to understand" argument floating around for a while, but at the end of the day, this is a personal preference.

my preference is rgb and know plenty of others who work with rgb almost exclusively (because we like it :P).

willing to accept that it might make sense to use hsl as a default when you don't know what the user prefers.

OTOH, unconditionally converting to hsl even when someone has explicitly asked for an rgba value is less OK. What benefits are gained by changing formats between the clj source and rendered css?

I can see a few drawbacks:

  • The magic conversion has resulted in at least one bug (this thread)
  • It is surprising, and thus violates the principle of least surprise
  • It leaves the user with less control over their codebase

@noprompt
Copy link
Owner

@thedavidmeister Thanks for your contribution to the discussion. I've already admitted to it being surprising and that my original decision to automatically convert to HSL/HSLA was based on personal opinion. I have no disagreements with the notion that what you type should be what you get (you can still write in RGB/RGBA, the compilation just needs to be correct). My intention is to resolve this issue in the near future. I've been hard at work at creating a new release of this library but my free time is somewhat limited.


Regarding the drawbacks, I think the first and third are worthy points but the second is not. This is just a nitpick, but the principle of least surprise is highly amendable to interpretation and, honestly, I don't think it's a relevant talking point. A good example of this was given by Yukihiro Matsumoto, the creator of Ruby, who, in an interview stated

The principle of least surprise is not for you only. The principle of least surprise means principle of least my surprise.

Since this principle largely depends on individual interpretation, emphasizing it doesn't add much value to the conversation.

To give a personal example, when I originally made the decision to compile to HSL/HSLA, I did not anticipate end users being concerned by this because it worked and I felt that the color semantic was more important than the compiled notation. If the color expected to be rendered to the screen was rendered to the screen as expected, why emphasize the notation? To make my point, it wasn't surprising to me and I didn't think it would be surprising to others since my emphasis was on semantic and not notation.

This aside, it's still no excuse for the bug persisting as it stands today.

@thedavidmeister
Copy link

is the conversion from rgb to hsl lossless?

@noprompt
Copy link
Owner

@thedavidmeister Aside from the compiler bug and according to color math, yes.

@noprompt
Copy link
Owner

@thedavidmeister I could verify this in garden.color with some generative tests though. I may stand corrected by that if it's not 100% that's the case.

@thedavidmeister
Copy link

yeah cool, a few months back someone in my team had problems where they wanted to extrapolate a palette of colours from a starting colour with a modulo operation that was supposed to bring them back to the start after a few "shifts" of the colour based on some calculation.

because they were accidentally triggering an implicit lossy conversion between rgb and hsl in the library they were using, the palette generation would "sometimes" hang - it never actually got back to the starting values due to rounding errors along the way.

i'm not saying this library has the same issue, but i thought it could be helpful to share an example of how this type of thing could cause real world problems

@noprompt
Copy link
Owner

Okay. So the main problem here is that there is currently only one type for representing colors, garden.color/CSSColor. Unless a hex code is use all colors end up being represented by this class. This means there's currently no way to distinguish between a color that should be rendered as RGB and on that should be rendered as HSL. This is what happens when you try to overload a single type with too much responsibility.

The only way forward I can see is by breaking the API and releasing a new major version which uses the garden.color library which was supposed to be integrated a couple years ago but I never got around to doing it. Now seems like the right time since this is a major bug. The library is fairly robust and works very well but the test suite lacks enough testing to prove that (I've only confirmed it works properly from the REPL).

Since I'd like the next version to address more than that, I will start by releasing an alpha version which will include the updates to the color namespace using the extracted library code. I will leave a note here when that's available.

@noprompt noprompt added this to the v2.0.0 milestone Oct 26, 2016
@noprompt
Copy link
Owner

This is fixed on the v2.0.0 branch. See #123 and pay close attention to the Changelog. Once v2.0.0 is merged I will close this issue.

noprompt added a commit that referenced this issue Nov 29, 2016
This patch rethinks the architecture of Garden in terms of well
defined boundaries between parsing and compiling. Instead of
directly compiling objects into CSS, Garden will first parse the objects
into a CSS AST which can then be consumed by the compiler. This
architecture is closer to what one might expect to find in a
programming language implementation setting. This new architecture
yields greater flexibility to introduce new features and semantics
while vastly simplifying the work of the compiler.

As a result many requested features and solutions to bugs have now
been either implemented, solved, or are now trivial to implement or
solve.

Changelog updates forthcoming.

Closes #31
Closes #109
Closes #115
Closes #116
Closes #119
Closes #120
Closes #121
@hkjels
Copy link

hkjels commented Jun 7, 2017

I've been rummaging a lot of code trying to find out why colors where being output incorrectly here. I ended up adding a slight rotation to all colors with an alpha-channel. Is v2.0.0 far from mergeable?

@noprompt
Copy link
Owner

noprompt commented Jun 7, 2017

@hkjels It's not. It's usable but I'm waiting for clojure 1.9 and the spec to settle down or at least the latter. v2.0.0 depends on spec to assist with parsing and I want I don't want to release any major version that depends on alpha libraries.

@noprompt
Copy link
Owner

noprompt commented Jun 7, 2017

@hkjels You can actually clone this repository, switch to the v2.0.0 branch and lein install it.

@hkjels
Copy link

hkjels commented Jun 8, 2017

I tried that earlier, but it failed with some cryptic error-message. Maybe I'll have yet another look

@noprompt
Copy link
Owner

noprompt commented Jun 8, 2017

@hkjels You can post the error message here if you like.

@hkjels
Copy link

hkjels commented Jun 9, 2017

@noprompt So, it's just that I'm relying on a newer version of Clojure and thereby clojure.spec.alpha

@wpcarro
Copy link
Contributor

wpcarro commented Nov 18, 2017

A few of the colors that I was generating with garden.color weren't rendering properly because of dropped percentages.

@noprompt If we're okay with using Clojure's alpha libraries, is the solution in the meantime to:

clone this repository, switch to the v2.0.0 branch and lein install it

My hackish solution in the short-term was to turn pretty-printing off.

@fl00r
Copy link

fl00r commented Aug 22, 2018

The problem with the percent sign happens when compiling not pretty printed

(garden.core/css {:pretty-print? false} [:foo {:color (garden.color/rgba 0 0 0 0.5)}])
#=> "foo{color:hsla(0,0,0,0.5)}"

(garden.core/css {:pretty-print? true} [:foo {:color (garden.color/rgba 0 0 0 0.5)}])
#=> "foo {\n  color: hsla(0, 0%, 0%, 0.5);\n}"

@fl00r
Copy link

fl00r commented Aug 22, 2018

And that's a bug in YUI Compressor

@minikomi
Copy link

Also got bitten w/ this with pretty print off

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

8 participants