When serializing the 'background' shorthand, put 'background-color' at the beginning of the bottom layer

VERIFIED FIXED in mozilla38

Status

()

Core
CSS Parsing and Computation
P4
trivial
VERIFIED FIXED
5 years ago
2 years ago

People

(Reporter: Sebastian Zartner, Assigned: dbaron)

Tracking

({regression})

Trunk
mozilla38
regression
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

5 years ago
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

5 years ago
Keywords: regression
(Assignee)

Comment 1

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

5 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

5 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
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

2 years ago
In today's teleconference the WG agreed to do this.
Assignee: nobody → dbaron
Status: NEW → ASSIGNED
(Assignee)

Comment 6

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=145612e73033
https://tbpl.mozilla.org/?tree=Try&rev=145612e73033
(Assignee)

Comment 7

2 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

2 years ago
Created attachment 8552927 [details] [diff] [review]
Serialize background-color at the beginning of the last item of the background shorthand

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

2 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.
OK, makes sense.
(Assignee)

Comment 12

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/e2bd0737c77f
https://hg.mozilla.org/mozilla-central/rev/e2bd0737c77f
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Works nice. Thank you!

Sebastian
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.