372770 - Reading style attribute omits the 'a' part of rgba/hsla colors

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P3)

Description

[Jesse Ruderman]

Attachment: testcase

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).

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

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.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 3

[Boris Zbarsky [:bzbarsky]]

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.

Comment 4

[Boris Zbarsky [:bzbarsky]]

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

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.)

Comment 6

[Boris Zbarsky [:bzbarsky]]

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.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 8

[Boris Zbarsky [:bzbarsky]]

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

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: Test

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

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

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

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated test

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: Test with the space issue fixed

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

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 15

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 16

[Boris Zbarsky [:bzbarsky]]

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

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(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 18

[Boris Zbarsky [:bzbarsky]]

Attachment: Test with the boolean and other comments addressed

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 20

[Boris Zbarsky [:bzbarsky]]

Attachment: With those todos.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Checked in the test.

Comment 22

[Boris Zbarsky [:bzbarsky]]

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...

Comment 23

[Boris Zbarsky [:bzbarsky]]

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

Comment 24

[Robert Sayre]

(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.

Comment 25

[Jesse Ruderman]

> 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.

Comment 26

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 27

[Boris Zbarsky [:bzbarsky]]

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

Comment 28

[Robert Sayre]

(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

Comment 29

[Boris Zbarsky [:bzbarsky]]

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...

Comment 30

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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