Reading the style attribute omits the 'a' part of rgba/hsla colors. So does elem.style.color. Not to be confused with bug 347912, which is about computed style. Computed style and reading the style attribute use different code (as bz pointed out to me in that bug).

Thanks to bz for reminding me that I need to output a float rather than an integer, and partly writing the code for that bit.

(And we should probably add the rounding code to the computed style code as well.)

Comment on attachment 257457 [details] [diff] [review] patch >@@ -402,25 +402,42 @@ nsCSSDeclaration::AppendCSSValueToString >+ if (a == 0) { >+ aResult.AssignLiteral("transparent"); Do we want that here? If it started out as the keyword, we won't be in this code to start with, and if not it makes more sense to hand back rgba() than the keyword... Other than that, looks great. Good catch on the truncation/rounding thing; I'd forgotten that the (int) cast truncates.

I should have said r+sr=bzbarsky once we agree on the "transparent" thing.

Since I added support for transparent for all color values (as css3-color has it), I think we can end up there. And I just tested that (on Jesse's testcase, with the value changed to transparent) we do in fact end up there. Now that we don't support non-cairo anymore, I should really look at removing the special handling for transparent borders and background-colors. (We can still get the performance optimizations by checking the alpha-component of the color.)

OK. Would it make sense to only output "transparent" if all the rgb components are 0 or whatever the keyword sets them too? I'd really like to keep this as nice as possible for editor roundtripping, basically.

If we're worried about that, we probably need to store rgb() vs. hsl() as well.

I'd thought about that, but that's a much bigger change, whereas doing the right thing for "transparent" here is easy...

This tests what that patch currently implements (complete with "transparent" for all alpha == 0 colors). I should file a bug on the trailing whitespace...

Revised to use 'transparent' only for NS_RGBA(0, 0, 0, 0).

Fix checked in to trunk. Filed bug 372781 on comment 5 paragraph 2. Filed bug 372782 on comment 2. Filed bug 372783 on comment 7 / comment 8.

Let me know if you want the space to go in a separate bug? It didn't seem worth it....

Comment on attachment 257473 [details] [diff] [review] Test with the space issue fixed >Index: layout/style/nsCSSDeclaration.cpp >+ } else if (!aValue.IsEmpty() && aValue.Last() == PRUnichar(' ')) { Seems like the second half of this if() should really just be an assertion inside the {}. >+ // We appended an extra space. Let's get rid of it >+ aValue.Truncate(aValue.Length() - 1); >Index: layout/style/test/test_bug372770.html >+ if (i == 100) { >+ color1 = "rgb(128, 128, 128)"; >+ color2 = "rgb(175, 63, 27)"; >+ } You should comment that this code is working around a round-tripping bug, and probably either file it separately or expand bug 372783 to include it, and either way include the bug number in the comment. r+sr=dbaron with that

> Seems like the second half of this if() should really just be an assertion We might have had no background properties to append... and someone might have called us to start with with a nonempty string, no? > You should comment that this code is working around a round-tripping bug Will do.

I guess what I should really do is keep track of a boolean indicating that something was appended....

(In reply to comment #15) > > Seems like the second half of this if() should really just be an assertion > > We might have had no background properties to append... and someone might have > called us to start with with a nonempty string, no? Ah, right, I forgot about caller starting with a nonempty string.

Comment on attachment 257477 [details] [diff] [review] Test with the boolean and other comments addressed >+// This code is only here because of bug 372783. Once that's fixed, this test >+// for "rgba(0, 0, 0, 0)" will fail. >+style1.color = "rgba(0, 0, 0, 0)"; >+style2.color = "rgba(0, 0, 0, 0)"; >+is(style1.color, "transparent", >+ "Inline style should give transparent for rgba(0,0,0,0)"); >+is(style2.color, "transparent", >+ "Rule style should give transparent for rgba(0,0,0,0)"); Maybe you should also have todos for what they should be in addition to tests to make sure we don't regress? r+sr=dbaron

Checked in the test.

Hmm. So the test passes locally and on the Windows/Mac tinderboxen. On Linux it fails for two values: *** 4947 ERROR FAIL | Inline style color roundtripping failed at opacity 70 | got "rgba(128, 128, 128, 0.698)", expected "rgba(128, 128, 128, 0.7)" *** 5027 ERROR FAIL | Inline style color roundtripping failed at opacity 90 | got "rgba(128, 128, 128, 0.898)", expected "rgba(128, 128, 128, 0.9)" 0.698 * 255 = 177.9899999 0.7 * 255 = 178.5 Not sure how we're getting these results, exactly...

I just made the test skip those two values for now; I'll look in more detail tomorrow.

(In reply to comment #23) > I just made the test skip those two values for now; I'll look in more detail > tomorrow. these work for me on amd64 + ubuntu 6.06.

> Not sure how we're getting these results, exactly... My guess is that style parsing turns 0.7 into 178/255 (rounding down from 178.5), but this code thinks 0.7 would become 179/255 (rounding up) and decides to use three digits instead of two as a result.

> My guess is that style parsing turns 0.7 into 178/255 It shouldn't. The relevant code in nsCSSParser is: 3154 PRInt32 value = NSToIntRound(mToken.mNumber*255); and then we have: 81 inline PRInt32 NSToIntRound(float aValue) 82 { 83 return NS_lroundf(aValue); 84 } 60 inline NS_HIDDEN_(PRInt32) NS_lroundf(float x) 61 { 62 return x >= 0.0f ? PRInt32(x + 0.5f) : PRInt32(x - 0.5f); 63 } So 0.7 should become 179. And in fact does, over here and on the windows and mac boxen.

I tried enabling the test again, and it broke again. So I filed bug 372845 on the tinderbox and disabled the test again...

(In reply to comment #27) > I tried enabling the test again, and it broke again. So I filed bug 372845 on > the tinderbox and disabled the test again... > Are we seeing this bug? http://gcc.gnu.org/bugzilla/show_bug.cgi?id=323

That bug is an issue when doing "==" compares on floats, which one is not supposed to do anyway, really. This patch only does such a compare on an integer, so that bug shouldn't be biting us...

Perhaps the floating point problem is related to bug 360282? Still, not sure how that would affect a test machine.