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.

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?

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

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

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

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

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

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

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

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.

passes reftests and layout mochitests.

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

Shouldn't this be marked FIXED now?

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

See bug 455217 also.