The following test in Ringmark is failing on Firefox 52: var bgcolor, elem = document.createElement("div"); // Same as rgba(), in fact, browsers re-map hsla() to rgba() internally, // except IE9 who retains it as hsla elem.style.cssText = "background-color:hsla(120,40%,100%,.5)"; bgcolor = elem.style.backgroundColor; assert( H.test.string( bgcolor, "rgba" ) || H.test.string( bgcolor, "hsla" ), "HSLA color supported" ); In Firefox 51, bgcolor is rgba(255, 255, 255, 0.5) In Firefox 52, bgcolor is rgb(255, 255, 255, 0.5) This code was changed in bug 1295456 and/or bug 1302787.

Seems like this should go by the principle of serializing to the more-compatible syntax, and not change the serialization to the new syntax. Or does the spec say otherwise? If it does, have any other browsers implemented what it says?

(In reply to David Baron :dbaron: ⌚️UTC-7 from comment #1) > Seems like this should go by the principle of serializing to the > more-compatible syntax, and not change the serialization to the new syntax. That's fair. So I think the tentative fix here would be: - When we're serializing: if the alpha is partially transparent, serialize with "rgba" instead of "rgb" (for interop/backwards-compat purposes)? That seems reasonable to me. (Side note: I thought I recalled discussing this in bug 1295456, but I think there we mostly focused on something orthogonal, starting with bug 1295456 comment 48 -- there, we were mostly focusing on: "if an author specifies 'rgba(..., 1.0)', do we need to bother encoding the fact that the author used rgba and had a *hardcoded* 1.0 there, or can we treat it the same as 'rgb(...)' and just disregard the 1.0 alpha component?" For that question, I still think we made the right call.) > Or does the spec say otherwise? If it does, have any other browsers > implemented what it says? The spec does not say otherwise. Actually, I'm now seeing that it *requires* the behavior described in my suggested fix here. Quoting it, paraphrasing/clipping quite a bit: # * The computed value and used value of [...] colors with an explicitly opaque alpha channel # [or] without an alpha channel [...] # is the equivalent numeric value in comma separated rgb() notation omitting the alpha value. # # * The computed value and used value of [...] colors with a non opaque alpha channel [...] # is the equivalent numeric value in comma separated rgba() notation with the alpha value. https://drafts.csswg.org/css-color-4/#resolving-color-values There's also older spec-text at https://www.w3.org/TR/cssom-1/#serializing-css-values which says basically the same thing. Jerry, would you be up for taking this?

(Note: the css-color-4 spec-text that I quoted in comment 2 (the "Resolving Color values" section) was only added 2 months ago, here: https://github.com/w3c/csswg-drafts/commit/193d58bd45b1901b9db5e644e525d14fe3a945be So that's why we didn't see it when landing bug 1295456 (5 months ago). At that time, the best hint that the spec had about this was "for legacy reasons, an rgba() function also exists, with an identical grammar and behavior to rgb().")

OK, so our behavior here is already a little subtle. Some results from the console. Testing rgba with 1.0 alpha: >> $0.style.color = "rgba(1, 2, 3, 1.0)"; $0.style.color > rgb(1, 2, 3) So, we already drop 1.0 "a" values (and collapse rgba to rgb when we do). Testing rgba with 0.5 alpha: >> $0.style.color = "rgba(1, 2, 3, 0.5)"; $0.style.color > rgba(1, 2, 3, 0.5) So, we do preserve rgba when it was specified & when there's a non-opaque alpha component (which I'd forgotten). Testing non-rgba functions with 0.5 alpha: >> $0.style.color = "rgb(1, 2, 3, 0.5)"; $0.style.color > rgb(1, 2, 3, 0.5) >> $0.style.color = "hsla(0deg, 0%, 0%, 0.5)"; $0.style.color > rgb(0, 0, 0, 0.5) This last chunk of output is the problematic stuff, with respect to this bug. This comes from the logic here: https://dxr.mozilla.org/mozilla-central/rev/6d38ad302429c98115c354d643e81987ecec5d3c/layout/style/nsCSSValue.cpp#1668-1673 > bool showAlpha = (a != 255); > > if (unit == eCSSUnit_RGBAColor && showAlpha) { > aResult.AppendLiteral("rgba("); > } else { > aResult.AppendLiteral("rgb("); > } I think we should just drop the "unit" check there -- that'll probably bring us into alignment with the spec text that I quoted in comment 2. We may need a similar change in nsComputedDOMStyle, too -- not sure. And we should include tests (if we don't already have them) for this serialization behavior.

Right. So this is a regression from: https://hg.mozilla.org/mozilla-central/rev/a29d5cb24af486be4c8d66cdc2a024d6badf787b where the nsCSSValue.cpp change didn't consider hsla().

I will try to fix this today.

MozReview-Commit-ID: 44T8gy7UWFJ

MozReview-Commit-ID: 36qT5LxhB9Z

MozReview-Commit-ID: DQln6jtxolg

Comment on attachment 8847505 [details] [diff] [review] only omit the alpha component for full opaque color value. Review of attachment 8847505 [details] [diff] [review]: ----------------------------------------------------------------- r=me with nits addressed. First, the commit message needs some changes: > Bug 1347164 - only omit the alpha component for full opaque color value. r=dholbert This doesn't accurately describe the code-change here. (The code about omitting the alpha component is *not* actually changing at all. The only thing changing here is our selection of which color-function *name* to use -- rgb vs rgba.) Please reword to clarify what's actually changing -- maybe something like: Bug 1347164 - serialize colors using "rgba()" as the color-function, if they have a nonopaque alpha channel. r=dholbert ::: layout/style/nsCSSValue.cpp @@ +1680,5 @@ > unit == eCSSUnit_RGBAColor) { > nscolor color = GetColorValue(); > // For brevity, we omit the alpha component if it's equal to 255 (full > + // opaque). > + // e.g. This only partially explains what we're doing here. Please add a note about rgb vs rgba. Perhaps expand this part of the comment to something like the following: // For brevity, we omit the alpha component if it's equal to 255 (full // opaque). Also, we use "rgba" rather than "rgb" when we do include the // alpha value, for backwards-compat (even though they're aliases as of // css-color-4). e.g.:

Comment on attachment 8847506 [details] [diff] [review] css-color computed style test. Review of attachment 8847506 [details] [diff] [review]: ----------------------------------------------------------------- r=me with a comment added: ::: layout/style/test/test_computed_style.html @@ +376,5 @@ > })(); > > +(function test_bug_1347164() { > + var color = [ > + ["rgba(0, 0, 0, 1)", "rgb(0, 0, 0)"], Please add a comment at the top of this to explain what you're testing in general here. I think this would do it: // Test that computed color values are serialized as "rgb()" // IFF they're fully-opaque (and otherwise as "rgba()").

Comment on attachment 8847508 [details] [diff] [review] css-color specified style test. Review of attachment 8847508 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/style/test/test_specified_style.html @@ +1,4 @@ > +<!DOCTYPE HTML> > +<html> > +<head> > + <title>Test for miscellaneous specified style issues</title> r=me with one followup and a comment added. Followup request: So, I suppose this new mochitest makes sense as an analog of the existing "test_computed_style.html" mochitest. HOWEVER, we do *already* have a mochitest called "test_specified_value_serialization.html" which seems like it's trying to do the same thing (test a variety of serialization issues). I suspect the setup in this new test is probably better (it seems more modular, with each part nicely-scoped & cleaning up after itself), so this is fine for now, but could you file a followup on merging test_specified_value_serialization.html into this test, and address it after this bug? I don't want to leave this in a state where we've got two "grab bag" tests that purport to be covering the same thing. @@ +19,5 @@ > +var frame_container = document.getElementById("display"); > + > +(function test_bug_1347164() { > + var color = [ > + ["rgba(0, 0, 0, 1)", "rgb(0, 0, 0)"], As with the previous patch, please add a comment here explaining what you're trying to test. (probably using similar language to the suggested text in Comment 11)

Comment on attachment 8847508 [details] [diff] [review] css-color specified style test. Could these tests be added to test_specified_value_serialization.html instead of adding a new test file? (test_specified_value_serialization is a better name for this test than test_specified_style, and there's already a test with that name that does similar things!)

Nominating for tracking, both since it sounds like ringmark matters to some partners, and since this was a regression that we shouldn't have shipped...

Oops, my comment 13 duplicates dholbert's comment 12 -- but I think it's simple enough to just do that here rather than a followup bug. Just stick the existing body of the test in a (function() {...})();, and then add your new thing after it.

Yeah, good point that the (existing) test_specified_value_serialization is better-named -- we should just extend (and maybe eventually improve the older bits of) that test, rather than creating a new test file and reshuffling things into there. essentially, +1 to comment 15.

Track 53+ as regression.

Patrick Kettner (a PM for Edge at Microsoft) mentioned running into this bug, too -- he says "modernizr's hsla detect breaks because of it": https://twitter.com/patrickkettner/status/842475556113338369 So, sites that use modernizr & that care about hsla support might've been broken (or at least changed their behavior) due to this bug. This is more support for getting this uplifted to Firefox 53 before it ships.

Should we be creating web-platform-tests for this then rather than adding mochitests?

(In reply to Ryan VanderMeulen [:RyanVM] from comment #19) > Should we be creating web-platform-tests for this then rather than adding > mochitests? This won't actually be adding mochitests (once comment 16 is addressed) -- it'll just be extending already-existing mochitests. But ideally yes, it'd be great to get web-platform-tests for this behavior. I don't think we want to gate this fix shipping on those new tests being written, though, given that we're wanting to uplift it all the way to beta & hence time is of the essence (so we can detect & address fallout sooner rather than later)

MozReview-Commit-ID: 44T8gy7UWFJ

move the test into test_specified_value_serialization.html

Comment on attachment 8848263 [details] [diff] [review] css-color specified style test. v2 Review of attachment 8848263 [details] [diff] [review]: ----------------------------------------------------------------- This patch is simultaneously rewriting/reindenting most of this patch, and also adding new test code to it. Could you split it up into two parts, with the new test code for this bug in the second part? (The first part should have a commit message like "Rewrite mochitest test_specified_value_serialization.html to look more like test_computed_style.html", since I think that's what you're doing.) ::: layout/style/test/test_specified_value_serialization.html @@ +8,5 @@ > +<body> > +<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=">Mozilla Bug </a> > +<p id="display"></p> > +<div id="content" style="display: none"> > + end-of-line whitespace

> This patch is simultaneously rewriting/reindenting most of this patch Sorry, I meant to say "...of this test"

The hsla test in http://rng.io/about/ is passed now.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=99eaca1453a5f971d6bd29891f909ef47f81978b

Comment on attachment 8848270 [details] [diff] [review] rewrite mochitest test_specified_value_serialization.html to look more like test_computed_style.html. Review of attachment 8848270 [details] [diff] [review]: ----------------------------------------------------------------- Thanks. r=me

Comment on attachment 8848271 [details] [diff] [review] css-color specified style test. v3 Review of attachment 8848271 [details] [diff] [review]: ----------------------------------------------------------------- r=me

Please land attachment 8848261 [details] [diff] [review] attachment 8848262 [details] [diff] [review] attachment 8848270 [details] [diff] [review] attachment 8848271 [details] [diff] [review] to m-c. try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=99eaca1453a5f971d6bd29891f909ef47f81978b

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/36a2230fbad6 Serialize colors using "rgba()" as the color-function, if they have a non-opaque alpha channel. r=dholbert https://hg.mozilla.org/integration/mozilla-inbound/rev/8af7daaf5f88 css-color computed style test. r=dholbert https://hg.mozilla.org/integration/mozilla-inbound/rev/18c70bc07c6d Rewrite mochitest test_specified_value_serialization.html to look more like test_computed_style.html. r=dholbert https://hg.mozilla.org/integration/mozilla-inbound/rev/bb325d01c421 css-color specified style test. r=dholbert

had to back this out for turning stylo tests perma orange on inbound like https://treeherder.mozilla.org/logviewer.html#?job_id=84513380&repo=mozilla-inbound so seems to be a failure in own test.

Backout by cbook@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/beff3b520fb3 Backed out changeset bb325d01c421 https://hg.mozilla.org/integration/mozilla-inbound/rev/a34e80244b47 Backed out changeset 18c70bc07c6d https://hg.mozilla.org/integration/mozilla-inbound/rev/cad31f90c9b5 Backed out changeset 8af7daaf5f88 https://hg.mozilla.org/integration/mozilla-inbound/rev/71c32198e7c0 Backed out changeset 36a2230fbad6 for perma failures in own test

Currently, stylo doesn't have css-color-4 implementation. Set fail-if for these tests.

new try link for stylo test: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e8912ef4135d1ad34a896360b6ab42ae02afe9b5

Comment on attachment 8848426 [details] [diff] [review] set fail-if for css-color-4 test with stylo. Review of attachment 8848426 [details] [diff] [review]: ----------------------------------------------------------------- Sorry, instead of disabling the entire tests in stylo, we should update layout/style/test/stylo-failures.md so that it just expects the failures for these color values. If you have trouble coming up with the right modifications to that file to capture the expected failures, please needinfo xidorn.

Currently, stylo doesn't have css-color-4 implementation. Set expected-fail for these tests. The try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1246bc0a704651d6a2ae8cf7ead98c09b9b6a976

Please land attachment 8848261 [details] [diff] [review] attachment 8848262 [details] [diff] [review] attachment 8848270 [details] [diff] [review] attachment 8848271 [details] [diff] [review] attachment 8849030 [details] [diff] [review] to m-c. The try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1246bc0a704651d6a2ae8cf7ead98c09b9b6a976

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/049b04f008d6 serialize colors using "rgba()" as the color-function, if they have a non-opaque alpha channel. r=dholbert https://hg.mozilla.org/integration/mozilla-inbound/rev/c7aff74601c8 css-color computed style test. r=dholbert https://hg.mozilla.org/integration/mozilla-inbound/rev/12704956b400 rewrite mochitest test_specified_value_serialization.html to look more like test_computed_style.html. r=dholbert https://hg.mozilla.org/integration/mozilla-inbound/rev/6e0f51a3a733 css-color specified style test. r=dholbert https://hg.mozilla.org/integration/mozilla-inbound/rev/a49749aa7711 set expected-fail for css-color-4 test with stylo. r=xidorn

https://hg.mozilla.org/mozilla-central/rev/049b04f008d6 https://hg.mozilla.org/mozilla-central/rev/c7aff74601c8 https://hg.mozilla.org/mozilla-central/rev/12704956b400 https://hg.mozilla.org/mozilla-central/rev/6e0f51a3a733 https://hg.mozilla.org/mozilla-central/rev/a49749aa7711

Hi Daniel and Gerry, This is marked tracking-firefox53+. Are we going to uplift to 53, 54 and esr52?

(In reply to Jerry Shih[:jerry] (UTC+8) from comment #43) > This is marked tracking-firefox53+. Are we going to uplift to 53, 54 and > esr52? I think we should, per comment 18 & comment 20. To get that ball rolling, please request aurora/beta/esr52 approval at https://bugzilla.mozilla.org/attachment.cgi?id=8848261&action=edit

MozReview-Commit-ID: 44T8gy7UWFJ --HG-- extra : rebase_source : 58f8621c70e0acff99f95d3305ef7ef0cc11c870

MozReview-Commit-ID: 36qT5LxhB9Z

MozReview-Commit-ID: 4XSBQXtYSth

MozReview-Commit-ID: 6RRhz8ftEVO

Currently, stylo doesn't have css-color-4 implementation. Set expected-fail for these tests.

MozReview-Commit-ID: 44T8gy7UWFJ

MozReview-Commit-ID: 36qT5LxhB9Z

MozReview-Commit-ID: 4XSBQXtYSth

MozReview-Commit-ID: 6RRhz8ftEVO

MozReview-Commit-ID: 44T8gy7UWFJ

Comment on attachment 8849819 [details] [diff] [review] [aurora] serialize colors using "rgba()" as the color-function, if they have a non-opaque alpha channel. Approval Request Comment [Feature/Bug causing the regression]: bug 1295456 [User impact if declined]: The hsla test will falied in http://rng.io/about/ [Is this code covered by automated tests?]: Yes. But the tests are only at m-c. Should we need to uplift the test case? If yes, there are 4 additional file need to be uplifted. The try for m-c: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1246bc0a704651d6a2ae8cf7ead98c09b9b6a976 [Has the fix been verified in Nightly?]: Yes. The hsla test is passed in http://rng.io/about/ . [Needs manual test from QE? If yes, steps to reproduce]: no. [List of other uplifts needed for the feature/fix]: Should we need to uplift the test case? If yes, there are 4 additional files need to be uplifted. [Is the change risky?]: no. [Why is the change risky/not risky?]: We will turn to use more-compatible syntax for css-color value serializing. [String changes made/needed]: none

Comment on attachment 8849831 [details] [diff] [review] [beta] serialize colors using "rgba()" as the color-function, if they have a non-opaque alpha channel. Approval Request Comment [Feature/Bug causing the regression]: bug 1295456 [User impact if declined]: The hsla test will be falied in http://rng.io/about/ [Is this code covered by automated tests?]: Yes. But the tests are only at m-c. Should we need to uplift the test case? If yes, there are 4 additional file need to be uplifted. The try for m-c: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1246bc0a704651d6a2ae8cf7ead98c09b9b6a976 [Has the fix been verified in Nightly?]: Yes. The hsla test is passed in http://rng.io/about/ . [Needs manual test from QE? If yes, steps to reproduce]: no. [List of other uplifts needed for the feature/fix]: Should we need to uplift the test case? If yes, there are 4 additional files need to be uplifted. [Is the change risky?]: no. [Why is the change risky/not risky?]: We will turn to use more-compatible syntax for css-color value serializing. [String changes made/needed]: none

Comment on attachment 8849835 [details] [diff] [review] [esr52] serialize colors using "rgba()" as the color-function, if they have a non-opaque alpha channel. [Approval Request Comment] If this is not a sec:{high,crit} bug, please state case for ESR consideration: We are try to use more-compatible syntax for css-color value serializing after bug 1295456 changes. User impact if declined: The hsla test will falied in http://rng.io/about/ Fix Landed on Version: firefox55 Risk to taking this patch (and alternatives if risky): No. We will use more-compatible syntax for css-color value serializing. String or UUID changes made by this patch: None.

Hi :jerry, Please also uplift the test cases. Thanks.

My assumption was that the test patches didn't require rebasing, only the main one. We'll make sure to land the tests too once approved :) Also, is this still on the radar for dot release consideration?

(In reply to Ryan VanderMeulen [:RyanVM] from comment #59) > Also, is this still on the radar for dot release consideration? Given that we're still unaware of any actual site breakage from this (and given that there's a small risk to any behavior change), I think we should just let this ship in 53 (via uplift to 53 beta) - no need to rush it in a dot release.

wontfix for 52 per comment 60.

Comment on attachment 8849819 [details] [diff] [review] [aurora] serialize colors using "rgba()" as the color-function, if they have a non-opaque alpha channel. Fix a regression related to site-compat. Aurora54+ & Beta53+.

Tomcat, see the recent commments. The tests need uplifting too.

thanks ryan, jerry uplifting the tests, but the set expected-fail for css-color-4 test with stylo test/patch need rebasing: grafting 406330:a49749aa7711 "Bug 1347164 - set expected-fail for css-color-4 test with stylo. r=xidorn" merging layout/style/test/stylo-failures.md merging layout/style/test/test_computed_style.html merging layout/style/test/test_specified_value_serialization.html warning: conflicts while merging layout/style/test/stylo-failures.md! (edit, then use 'hg resolve --mark') abort: unresolved conflicts, can't continue (use 'hg resolve' and 'hg graft --continue')

remote: https://hg.mozilla.org/releases/mozilla-aurora/rev/f72b90d90e18040b473e8234405d78c20ed682eb remote: https://hg.mozilla.org/releases/mozilla-aurora/rev/f7876bdb3d04937ea5d7f18f5ce8c7408701e819 remote: https://hg.mozilla.org/releases/mozilla-aurora/rev/06b2530392f938ee62ade6659bdf40dee658b5cb

We aren't running Stylo tests on !trunk, are we? That sounds like a bug if we are :)

(In reply to Ryan VanderMeulen [:RyanVM] from comment #67) > We aren't running Stylo tests on !trunk, are we? That sounds like a bug if > we are :) yup just trunk so its not a urgent thing

Why would we need to backport that patch at all then?

https://hg.mozilla.org/releases/mozilla-beta/rev/4951da1c3b0a https://hg.mozilla.org/releases/mozilla-beta/rev/5da5aba0ab8c https://hg.mozilla.org/releases/mozilla-beta/rev/350359bc6c89 https://hg.mozilla.org/releases/mozilla-beta/rev/3b72a4e09399

Comment on attachment 8849823 [details] [diff] [review] [aurora] set expected-fail for css-color-4 test with stylo. Thanks, Ryan and Carsten. Here is the rebased stylo failure-list patch for aurora if we still try to pass the stylo test at aurora. We need to disable some tests with stylo. The css-color-4 is not implemented in stylo. And here is its try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fee38a43df204217b9266e0311e747f0949565b7

thanks jerry, will check this in

Comment on attachment 8849835 [details] [diff] [review] [esr52] serialize colors using "rgba()" as the color-function, if they have a non-opaque alpha channel. webcompat fix, esr52+

https://hg.mozilla.org/releases/mozilla-esr52/rev/b35a6d6dcdca https://hg.mozilla.org/releases/mozilla-esr52/rev/a42fc05969b9 https://hg.mozilla.org/releases/mozilla-esr52/rev/f78ac1ac0a37 https://hg.mozilla.org/releases/mozilla-esr52/rev/795a3d48a775

Setting qe-verify- based on Jerry's assessment on manual testing needs and the fact that this fix has automated coverage (see Comment 56).