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

Status: VERIFIED FIXED

(Core :: CSS Parsing and Computation, defect, P4)

Description

[Sebastian Zartner [:sebo]]

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

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(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

[Steve Roussey (:sroussey)]

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

Comment 3

[Sebastian Zartner [:sebo]]

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

Comment 4

[Sebastian Zartner [:sebo]]

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

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

In today's teleconference the WG agreed to do this.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=145612e73033
https://tbpl.mozilla.org/?tree=Try&rev=145612e73033

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: 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.

Comment 9

[Cameron McCormack (:heycam)]

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?

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(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

[Cameron McCormack (:heycam)]

OK, makes sense.

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e2bd0737c77f

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e2bd0737c77f

Comment 14

[Sebastian Zartner [:sebo]]

Works nice. Thank you!

Sebastian