Closed Bug 1339394 Opened 8 years ago Closed 8 years ago

Don't serialize transparent colors to "transparent" keyword in various cases

Categories

(Core :: CSS Parsing and Computation, defect)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla54
Tracking Flags:
Tracking Status
firefox54 --- fixed

People

(Reporter: xidorn, Assigned: xidorn)

References

Details

(Keywords: dev-doc-complete, site-compat)

Attachments

(1 file)

Bug 1339394 - Don't serialize transparent color to transparent keyword when not necessary.
59 bytes, text/x-review-board-request
heycam
: review+
jaws
: review+
Details
Currently, for specified value, we always serialize rgba(0, 0, 0, 0) to "transparent". In addition, for resolved value (from getComputedStyle), we serialize any color whose alpha channel is zero to "transparent". This doesn't match the spec, nor other browsers' behavior. We should fix it. Servo handles it correctly, which triggers unexpected pass in Gecko's style system test.
One thing to note is that Edge treats "transparent" as a valid computed value (but Chrome and Safari don't). But it is clear from the spec that transparent should compute to rgba(0, 0, 0, 0).
Comment on attachment 8837106 [details] Bug 1339394 - Don't serialize transparent color to transparent keyword when not necessary. https://reviewboard.mozilla.org/r/112352/#review113976 ::: layout/style/test/test_bug372770.html:38 (Diff revision 1) > // This code is only here because of bug 372783. Once that's fixed, this test > // for "rgba(0, 0, 0, 0)" will fail. Is this comment out of date with this fix (and can you then resolve that bug)?
Attachment #8837106 - Flags: review?(cam) → review+
Keywords: dev-doc-needed
Comment on attachment 8837106 [details] Bug 1339394 - Don't serialize transparent color to transparent keyword when not necessary. https://reviewboard.mozilla.org/r/112352/#review113976 > Is this comment out of date with this fix (and can you then resolve that bug)? For the testcase here, yeah the comment is no longer relevant, so we can remove it. The original bug is not completely resolved, though. We still serialize `rgba(r, g, b, 1.0)` to `rgb(r, g, b)` (and hsl similar), which we may still want to fix?
Pushed by xquan@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/ae26b4e4d59b Don't serialize transparent color to transparent keyword when not necessary. r=heycam
Backout by ihsiao@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/036b32fc9bc4 Backed out changeset ae26b4e4d59b for developer's request
Pushed by xquan@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/4a964c3f1759 Don't serialize transparent color to transparent keyword when not necessary. r=heycam
backed this out since this seem to have caused failures like https://treeherder.mozilla.org/logviewer.html#?job_id=77581628&repo=mozilla-inbound
Flags: needinfo?(xidorn+moz)
Backout by cbook@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/828c1f39d71a Backed out changeset 4a964c3f1759 for suspicion of causing failures on OS X browser_selectpopup.js tests
Pushed by xquan@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/4ea36a9c2dfd Don't serialize transparent color to transparent keyword when not necessary. r=heycam
I'll stare at the commit this time :)
Flags: needinfo?(xidorn+moz)
> The original bug is not completely resolved, though. We still serialize > `rgba(r, g, b, 1.0)` to `rgb(r, g, b)` (and hsl similar), which we may still > want to fix? Servo does this too, so please file a cssparser bug if you plan to change it.
(In reply to Emilio Cobos Álvarez [:emilio] from comment #16) > Servo does this too, so please file a cssparser bug if you plan to change it. Have no plan to change this :)
Backed out for the browser_selectpopup.js failures returning: https://hg.mozilla.org/integration/mozilla-inbound/rev/5c25a3cb838e
Flags: needinfo?(xidorn+moz)
r? jaws for the change to toolkit/modules/SelectParentHelper.jsm. Comparing color value from getComputedStyle with "rgba(0, 0, 0, 0)" isn't strictly same as comparing with "transparent" before the change, since getComputedStyle returned "transparent" whenever alpha channel is zero. But I guess that isn't a big issue here.
Flags: needinfo?(xidorn+moz)
Comment on attachment 8837106 [details] Bug 1339394 - Don't serialize transparent color to transparent keyword when not necessary. https://reviewboard.mozilla.org/r/112352/#review114526 Thanks. I never understood why a color value that is rgb(192, 132, 123, 0) has a computed value of rgb(0, 0, 0, 0); It seems that this is now fixed to always return the correct RGB values?
Attachment #8837106 - Flags: review?(jaws) → review+
> Thanks. I never understood why a color value that is rgb(192, 132, 123, 0) has a computed value of rgb(0, 0, 0, 0); It seems that this is now fixed to always return the correct RGB values? Yes, that's right.
Pushed by xquan@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/107fdc32d411 Don't serialize transparent color to transparent keyword when not necessary. r=heycam,jaws
Backout by xquan@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/a53d44999d4c Backed out changeset 107fdc32d411 for potential failures on merge
Pushed by xquan@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/d21d9c97740a Don't serialize transparent color to transparent keyword when not necessary. r=heycam,jaws
https://hg.mozilla.org/mozilla-central/rev/d21d9c97740a
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
I've documented this on MDN. First of all, I've checked that the transparent description sounds correct: https://developer.mozilla.org/en-US/docs/Web/CSS/color_value#transparent_keyword Do you want me to add a specific line about the change in behaviour? It doesn't really sound like something that would affect developers. I've also added a note to the Fx54 release notes: https://developer.mozilla.org/en-US/Firefox/Releases/54#CSS Let me know if there's anything else you need. Thanks.
Keywords: dev-doc-needed → dev-doc-complete
See Also: → 1445599
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: