Closed Bug 453916 Opened 11 years ago Closed 11 years ago

Kill off transparency as a notion separate from color alpha

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9.1b1

People

(Reporter: zwol, Assigned: zwol)

References

Details

Attachments

(1 file, 2 obsolete files)

Attached patch (rev 1) proposed patch (obsolete) — Splinter Review
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.
Attachment #337163 - Flags: superreview?(dbaron)
Attachment #337163 - Flags: review?(dbaron)
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.
Attachment #337163 - Attachment is obsolete: true
Attachment #338190 - Flags: superreview?(dbaron)
Attachment #338190 - Flags: review?(dbaron)
Attachment #337163 - Flags: superreview?(dbaron)
Attachment #337163 - Flags: review?(dbaron)
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
Attachment #338190 - Flags: superreview?(dbaron)
Attachment #338190 - Flags: superreview+
Attachment #338190 - Flags: review?(dbaron)
Attachment #338190 - Flags: review+
(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.
Attachment #338190 - Attachment is obsolete: true
note to anyone landing this: please consider doing 453566 at the same time.
Keywords: checkin-needed
Shouldn't this be marked FIXED now?
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Depends on: 455093
See bug 455093 -- looks like either this or bug 453566 regressed some reftests on seamonkey/macOS
See bug 455217 also.
Depends on: 455217
Attachment #338253 - Attachment description: (rev 3) with dbaron's requested changes → (rev 3) with dbaron's requested changes [Checkin: Comment 12]
Keywords: checkin-needed
Target Milestone: --- → mozilla1.9.1b1
You need to log in before you can comment on or make changes to this bug.