453916 - Kill off transparency as a notion separate from color alpha

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Zack Weinberg (:zwol)]

Attachment: (rev 1) proposed patch

Per discussion in bug 372781 and bug 453566, we currently represent the CSS keyword 'transparent' differently from 'rgba(x,y,z,0)' (for arbitrary values of x,y,z) -- the keyword sets a special flag in various style structs, the rgba() notation produces an nscolor whose A component is zero.  The representation difference leads to a bunch of places where they aren't treated the same, but the current css3-color draft says that they should be equivalent in all cases.

The attached patch kills off the special flag and uses nscolors with A component zero consistently everywhere.  Passes reftests and layout mochitests.

Comment 1

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

Places where we serialize colors should probably prefer 'transparent' to 'rgba(0, 0, 0, 0), I think (since it's been around longer).  Does this patch do that?

Comment 2

[Zack Weinberg (:zwol)]

Yes, color serialization consistently goes through nsComputedDOMStyle::SetToRGBAColor() which serializes any color with A component 0 as 'transparent'.  Arguably it should use the rgba() notation for colors with A zero but other components nonzero, but it didn't before my patch, so I just left it alone.  (It's not visible in the patch because it's not changed.)

Comment 3

[Boris Zbarsky [:bzbarsky]]

So if I have <span style="background: transparent">, do we do the right thing when getting span.style.backgroundColor or span.style.cssText?

Comment 4

[Zack Weinberg (:zwol)]

Believe so, but do not have a built copy with the patch right now to test.  Will get back to you when I do.

Comment 5

[Zack Weinberg (:zwol)]

(In reply to comment #3)
> So if I have <span style="background: transparent">, do we do the right thing
> when getting span.style.backgroundColor or span.style.cssText?

Yes, we do; with my patch applied, .backgroundColor comes back out as "transparent" in that case, and .cssText includes the word "transparent" rather than e.g. "rgba(0,0,0,0)".

Comment 6

[Zack Weinberg (:zwol)]

Attachment: (rev 2) update after various things landed

patch needed rediffing after some other changes to the parser landed.

Comment 7

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

Comment on attachment 338190 [details] [diff] [review]
(rev 2) update after various things landed

So the replacement of AreCompositeColorsEqual with
nsBorderColors::Equals is technically incorrect, since you're relying on
calling member functions with this == null, which I don't think you're
really supposed to do (although in this case I agree it should work).

In nsDisplayBackground::IsOpaque, I'm not sure why you pulled out
|border| into its own variable when it's only used once; it seems like
it may as well stay mFrame->GetStyleBorder() inside the argument to
HasNonZeroSide.

In nsCSSCompressedDataBlock::MapRuleInfoInto, you also need to check for
eCSSUnit_EnumColor (and do the SetColorValue if that's the unit; no
additional checks needed).  There, in the comment you added, I'd also
replace "preserve the form of the original color spec" to "have the
value in the form it was specified" (otherwise "spec" sounds like
specification rather than "as specified").

In nsComputedDOMStyle:
+    const nsStyleColor* colorStruct = GetStyleColor();
+    color = colorStruct->mColor;
Could you fix this to be just
  color = GetStyleColor()->mColor;
? 

nsRuleNode.cpp:

+          // we could get -moz-use-text-color here, but we don't
+          // implement that
Don't add this comment, since we do support currentColor, which does the
same thing.  (But see bug 372781 comment 3 and bug 372781 comment 4 on
why combining them is hard.)

Could you remove the nsPresContext* from the constructor of
nsStyleBackground (you'll need to fix nsStyleStruct.h and
nsStyleStruct.cpp and the funny macro in nsStyleStructList.h)?

(Removing NS_STYLE_BG_IMAGE_NONE as a followup patch would probably be
easy.  But don't combine it into this patch.  And then the rest of 
background flags could go away when we implement multiple backgrounds... 
which I have part of a patch to do.)

r+sr=dbaron with those things fixed

Comment 8

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

(In reply to comment #7)
> So the replacement of AreCompositeColorsEqual with
> nsBorderColors::Equals is technically incorrect, since you're relying on
> calling member functions with this == null, which I don't think you're
> really supposed to do (although in this case I agree it should work).

Note that I think I'm suggesting that nsBorderColors probably shouldn't have a member equals method, since the common case is that either one could be null.  I'm not sure if there were any other callers of that, though...

Comment 9

[Zack Weinberg (:zwol)]

Revised patch in ~ half an hour - the changes are invasive enough that I want to re-run some of the test suites.

(In reply to comment #7)
> (From update of attachment 338190 [details] [diff] [review])
> So the replacement of AreCompositeColorsEqual with
> nsBorderColors::Equals is technically incorrect, since you're relying on
> calling member functions with this == null, which I don't think you're
> really supposed to do (although in this case I agree it should work).

Oh, ick.  That possibility didn't even occur to me; I just happened to notice that the body of AreCompositeColorsEqual was nearly the same as the body of the existing nsBorderColors::Equal method.  I've turned nsBorderColors::Equals into a two-argument static method, and renamed it to ::Equal, consistent with other such static methods in layout/style.  The only other caller exposed by recompilation had an explicit guard against calling it with this == null.

> In nsDisplayBackground::IsOpaque, I'm not sure why you pulled out
> |border| into its own variable when it's only used once; it seems like
> it may as well stay mFrame->GetStyleBorder() inside the argument to
> HasNonZeroSide.

Just to keep the line under 80 columns without having to split it at a ->.  I changed it.

> In nsCSSCompressedDataBlock::MapRuleInfoInto, you also need to check for
> eCSSUnit_EnumColor (and do the SetColorValue if that's the unit; no
> additional checks needed).  There, in the comment you added, I'd also
> replace "preserve the form of the original color spec" to "have the
> value in the form it was specified" (otherwise "spec" sounds like
> specification rather than "as specified").

Done.  (Should this function maybe be broken up into subroutines for each eCSSType_* case?  If nothing else it would mean that the code wasn't squashed against the right margin.)

> +    const nsStyleColor* colorStruct = GetStyleColor();
> +    color = colorStruct->mColor;
> Could you fix this to be just
>   color = GetStyleColor()->mColor;

Done.

> +          // we could get -moz-use-text-color here, but we don't
> +          // implement that
> Don't add this comment, since we do support currentColor, which does the
> same thing.

Done.  (Just to be sure, no code change required, right?)

> Could you remove the nsPresContext* from the constructor of
> nsStyleBackground (you'll need to fix nsStyleStruct.h and
> nsStyleStruct.cpp and the funny macro in nsStyleStructList.h)?

Done.  A couple places in nsRuleNode also needed the argument removed.  I didn't do it before only because I wasn't sure it wouldn't cascade through dozens of places.

> (Removing NS_STYLE_BG_IMAGE_NONE as a followup patch would probably be
> easy.  But don't combine it into this patch.)

Will put it on the list of things to do when I get the cleanup itch.

Comment 10

[Zack Weinberg (:zwol)]

Attachment: (rev 3) with dbaron's requested changes [Checkin: Comment 12]

passes reftests and layout mochitests.

Comment 11

[Zack Weinberg (:zwol)]

note to anyone landing this: please consider doing 453566 at the same time.

Comment 12

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

http://hg.mozilla.org/mozilla-central/rev/d426fd05130b

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Shouldn't this be marked FIXED now?

Comment 14

[Daniel Holbert [:dholbert]]

See bug 455093 -- looks like either this or bug 453566 regressed some reftests on seamonkey/macOS

Comment 15

[Joe Sabash [:JoeS1]]

See bug 455217 also.