Bug 26541 – CSS property value serialization order is out of whack

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Dimitri Glazkov (Google)]

It appears that color is not in the right order (at the end, instead of in front) when reading node.style.background. Haven't checked other attributes to see if this applies to other things. It looks like CSSOM draft suggests that we follow the same order in which the components are listed in CSS spec.

The bug originated here: http://crbug.com/8658

[Dimitri Glazkov (Google)]

Created attachment 31545 [details]
Test case (almost layout test).

[Yuta Kitamura]

Created attachment 32429 [details]
Simple fix (which causes test regression)

I tried to fix this bug, but I'm facing some difficulties on it. I've got a simple fix (attached) but this causes a test regression that is difficult to handle.

LayoutTests/fast/dom/background-shorthand-csstext.html fails, becuase div1's background property value is serialized to "repeat-x, white repeat-y". Apparently this should be "white repeat-x, repeat-y". However, WebKit CSS parser does NOT accept "background: white repeat-x, repeat-y;" as a valid style sheet declaration, so we can't do this. For now, I can't find any resolution that satisfies both CSSOM spec and WebKit parser. It seems too difficult for me to hack the CSS parser (at least for now).

By the way, according to CSS2, the value "repeat-x, repeat-y" for background-repeat property is invalid (you can use "repeat" for this purpose). I assume that this is a WebKit extension, but I'm not sure it really is.

[Dimitri Glazkov (Google)]

Perhaps you could bring up this issue on #webkit and see if there are ideas that come out of that?

[Yaar Schnitman]

Created attachment 38486 [details]
A Fix

Yuta's fix is correct and the layout test is wrong. 

WebKit implements CSS3's layered backgrounds, as described here: http://www.w3.org/TR/2008/WD-css3-background-20080910/#layering

With layers, repeat-x, repeat-y isn't equivalent to repeat. Rather, it means layer 1 should repeat-x and layer 2 should repeat-y. Also, background-image is by default applied to the last layer. Therefore, the style "background-color:white; background-repeat: repeat-x,repeat-y" should inded serialize into: "repeat-x, white repeat-y".

The change list includes Yuta's fix as well as the changes to the layout test that broke.

p.s. This is my first submission to WebKit. I hope I adhered to all guidelines.

[Eric Seidel (no email)]

In general this looks fine.  Could you please point me to the line of the spec where this is required?  Ideally your ChangeLog would also include a reference to said line.

[Eric Seidel (no email)]

Comment on attachment 38486 [details]
A Fix

I reviewed another conflicting change to this one yesterday.  I believe also from you.

Need a spec reference still.  r- because this won't apply anymore.

[Yaar Schnitman]

See: http://dev.w3.org/csswg/cssom/

Quote:
4.1. Parsing and Serializing CSS Constructs
...
4.1.1. CSS Value Rules
...
8. Where multiple values may appear in any order without changing the meaning of the value (typically represented by a double bar || in the value syntax), user agents are to use the canonical order as given in the syntax (e.g. <border-width> <border-style> <color> for the border short-hand property).

[Yuta Kitamura]

Thanks for your follow up, Yaar. I didn't know about layers.

I think the patch should be fine if the reference to the CSSOM spec is added. Could you kindly revise the patch? I can do it for you if you don't have enough time.

Thanks,

[Yaar Schnitman]

Would gladly fix the patch, but where should the reference be placed?

[Yuta Kitamura]

In Changelog.

Moreover, adding a layout test would be better.

[Yaar Schnitman]

As patches that fixes this bug as well as 25489 has been attached to 25489. Once 25489 is closed, this bug can be closed as well.

[Dave Hyatt]

Comment on attachment 38486 [details]
A Fix

r=me, totally looks fine.

[Adam Barth]

Comment on attachment 38486 [details]
A Fix

Clearing review flag on obsolete patch

[Yaar Schnitman]

Patch to 25489 should have fixed this one as well.