Last Comment Bug 743392 - When serializing the 'background' shorthand, put 'background-color' at the beginning of the bottom layer
: When serializing the 'background' shorthand, put 'background-color' at the be...
Status: VERIFIED FIXED
: regression
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: Trunk
: All All
: P4 trivial (vote)
: mozilla38
Assigned To: David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-06 16:41 PDT by Sebastian Zartner
Modified: 2015-01-23 23:40 PST (History)
4 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Serialize background-color at the beginning of the last item of the background shorthand (11.19 KB, patch)
2015-01-21 23:04 PST, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
cam: review+
Details | Diff | Review

Description Sebastian Zartner 2012-04-06 16:41:35 PDT
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-7 (review requests must explain patch) 2012-04-06 17:03:29 PDT
(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) 2012-04-06 18:16:48 PDT
> 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 2012-05-31 23:15:24 PDT
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] 2014-12-12 01:38:25 PST
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-7 (review requests must explain patch) 2015-01-21 09:49:49 PST
In today's teleconference the WG agreed to do this.
Comment 6 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2015-01-21 11:18:37 PST
https://treeherder.mozilla.org/#/jobs?repo=try&revision=145612e73033
https://tbpl.mozilla.org/?tree=Try&rev=145612e73033
Comment 7 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2015-01-21 21:17:46 PST
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-7 (review requests must explain patch) 2015-01-21 23:04:48 PST
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.
Comment 9 Cameron McCormack (:heycam) 2015-01-22 16:36:45 PST
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-7 (review requests must explain patch) 2015-01-22 16:50:11 PST
(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) 2015-01-22 16:51:35 PST
OK, makes sense.
Comment 12 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2015-01-22 17:00:36 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/e2bd0737c77f
Comment 13 Ryan VanderMeulen [:RyanVM] 2015-01-23 09:41:09 PST
https://hg.mozilla.org/mozilla-central/rev/e2bd0737c77f
Comment 14 Sebastian Zartner [:sebo] 2015-01-23 23:40:06 PST
Works nice. Thank you!

Sebastian

Note You need to log in before you can comment on or make changes to this bug.