Closed
Bug 743392
Opened 13 years ago
Closed 10 years ago
When serializing the 'background' shorthand, put 'background-color' at the beginning of the bottom layer
Categories
(Core :: CSS Parsing and Computation, defect, P4)
Core
CSS Parsing and Computation
Tracking
()
VERIFIED
FIXED
mozilla38
People
(Reporter: sebo, Assigned: dbaron)
Details
(Keywords: regression)
Attachments
(1 file)
11.19 KB,
patch
|
heycam
:
review+
|
Details | Diff | Splinter Review |
That's a split of bug 720945. "Some Firebug users were already complaining about the order of the properties. They want the 'background-color' property at the first position again as it was in FF 3.0. Currently it's the last part. See http://code.google.com/p/fbug/issues/detail?id=4180." Steven Roussey: "The W3C spec for CSS2.1 [1] always showed the value as: [<'background-color'> || <'background-image'> || <'background-repeat'> || <'background-attachment'> || <'background-position'>] So the tutorials over the years always show things in that order. Firefox used to return them in that order previously. Now it puts color at the end, which feels unnatural to many. Was: #E8E8E8 none repeat scroll 0 0 Now: none repeat scroll 0 0 #E8E8E8 (Due to the layers aspect in CSS3 [2] and that color is only allowed on the last one, someone somewhere probably thought "hey, if color is at the end, then no one will get confused", but instead people are confused now with simpler and far more common backgrounds, and it just makes Firebug and Firefox look buggy.) [1] http://www.w3.org/TR/CSS21/colors.html#background-properties [2] http://www.w3.org/TR/css3-background/#the-background" Sebastian
Reporter | ||
Updated•13 years ago
|
Keywords: regression
Assignee | ||
Comment 1•13 years ago
|
||
(In reply to Sebastian Zartner from comment #0) > (Due to the layers aspect in CSS3 [2] and that color is only allowed on the > last one, someone somewhere probably thought "hey, if color is at the end, > then no one will get confused", but instead people are confused now with > simpler and far more common backgrounds, and it just makes Firebug and > Firefox look buggy.) That's not true, as I said in bug 720945 comment 6: I think it was actually that continuing to put color at the start of the last item would make the code more complicated. To understand why, have a look at the code: http://hg.mozilla.org/mozilla-central/file/edf8075b0333/layout/style/Declaration.cpp#l397 It's actually a bit of extra code to tell whether an iteration of the loop is going to be the last time through.
Comment 2•13 years ago
|
||
> It's actually a bit of extra code to tell whether an iteration of the
> loop is going to be the last time through.
Yes, I saw that. (My comment was copied from the previous bug, it is not a new assertion). It is not a difficult fix; then again, I haven't made a patch for it...
Assignee | ||
Updated•13 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P4
Summary: When serializing the 'background' shorthand, put 'background-color' at the beginning → When serializing the 'background' shorthand, put 'background-color' at the beginning of the bottom layer
Reporter | ||
Comment 3•12 years ago
|
||
FYI: There's currently a discussion going on at the www-style mailinglist: http://lists.w3.org/Archives/Public/www-style/2012May/1202.html Sebastian
Reporter | ||
Comment 4•10 years ago
|
||
As I didn't see an outcome of the discussion mentioned in comment 3 in regard of this issue, I started a new discussion for this specific topic: http://lists.w3.org/Archives/Public/www-style/2014Dec/0194.html I assume the actual implementation isn't that hard, so can this be marked with [good first bug]? Sebastian
Assignee | ||
Comment 5•10 years ago
|
||
In today's teleconference the WG agreed to do this.
Assignee: nobody → dbaron
Status: NEW → ASSIGNED
Assignee | ||
Comment 6•10 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=145612e73033 https://tbpl.mozilla.org/?tree=Try&rev=145612e73033
Assignee | ||
Comment 7•10 years ago
|
||
And hopefully with all the test failures fixed: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c91c8319ded3 https://tbpl.mozilla.org/?tree=Try&rev=c91c8319ded3
Assignee | ||
Comment 8•10 years ago
|
||
This matches what Chrome and WebKit do for a single-item background shorthand, per discussion on today's CSS teleconference. (At least Chrome has bugs for multiple-items.) The added test fails without the patch and passes with the patch.
Attachment #8552927 -
Flags: review?(cam)
Comment 9•10 years ago
|
||
Comment on attachment 8552927 [details] [diff] [review] Serialize background-color at the beginning of the last item of the background shorthand Review of attachment 8552927 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/devtools/styleinspector/test/browser_ruleview_colorpicker-and-image-tooltip_01.js @@ +22,5 @@ > content.document.body.innerHTML = PAGE_CONTENT; > let {toolbox, inspector, view} = yield openRuleView(); > > let value = getRuleViewProperty(view, "body", "background").valueSpan; > + let swatch = value.querySelectorAll(".ruleview-colorswatch")[1]; I don't know what this is doing but I trust it's needed?
Attachment #8552927 -
Flags: review?(cam) → review+
Assignee | ||
Comment 10•10 years ago
|
||
(In reply to Cameron McCormack (:heycam) from comment #9) > browser/devtools/styleinspector/test/browser_ruleview_colorpicker-and-image- > tooltip_01.js > @@ +22,5 @@ > > content.document.body.innerHTML = PAGE_CONTENT; > > let {toolbox, inspector, view} = yield openRuleView(); > > > > let value = getRuleViewProperty(view, "body", "background").valueSpan; > > + let swatch = value.querySelectorAll(".ruleview-colorswatch")[1]; > > I don't know what this is doing but I trust it's needed? Yes. The test needs to manipulate the color-swatch for the gradient start-point in a declaration of the background shorthand. Since transparent moves from the end to the beginning of the declaration, the start-color of the gradient is now the second color swatch rather than the first.
Comment 11•10 years ago
|
||
OK, makes sense.
Assignee | ||
Comment 12•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/e2bd0737c77f
Comment 13•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/e2bd0737c77f
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in
before you can comment on or make changes to this bug.
Description
•