Bug 26541

Summary: CSS property value serialization order is out of whack
Product: WebKit Reporter: Dimitri Glazkov (Google) <dglazkov>
Component: CSSAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: eric, hyatt, yaar, yaar, yutak
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
URL: http://shopping.pchome.com.tw/?mod=area&func=style_show&RG_NO=DHAC&BB=3c
Bug Depends on: 25489    
Bug Blocks:    
Attachments:
Description Flags
Test case (almost layout test).
none
Simple fix (which causes test regression)
none
A Fix none

Description Dimitri Glazkov (Google) 2009-06-19 09:00:27 PDT
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
Comment 1 Dimitri Glazkov (Google) 2009-06-19 09:01:06 PDT
Created attachment 31545 [details]
Test case (almost layout test).
Comment 2 Yuta Kitamura 2009-07-07 23:01:31 PDT
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.
Comment 3 Dimitri Glazkov (Google) 2009-07-08 10:15:27 PDT
Perhaps you could bring up this issue on #webkit and see if there are ideas that come out of that?
Comment 4 Yaar Schnitman 2009-08-24 10:27:08 PDT
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.
Comment 5 Eric Seidel (no email) 2009-08-24 13:38:54 PDT
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.
Comment 6 Eric Seidel (no email) 2009-08-25 10:47:02 PDT
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.
Comment 7 Yaar Schnitman 2009-08-26 11:28:34 PDT
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).
Comment 8 Yuta Kitamura 2009-09-01 02:24:24 PDT
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,
Comment 9 Yaar Schnitman 2009-09-01 09:52:46 PDT
Would gladly fix the patch, but where should the reference be placed?
Comment 10 Yuta Kitamura 2009-09-01 19:02:13 PDT
In Changelog.

Moreover, adding a layout test would be better.
Comment 11 Yaar Schnitman 2009-09-03 13:54:02 PDT
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.
Comment 12 Dave Hyatt 2009-09-03 18:17:34 PDT
Comment on attachment 38486 [details]
A Fix

r=me, totally looks fine.
Comment 13 Adam Barth 2009-09-06 21:54:17 PDT
Comment on attachment 38486 [details]
A Fix

Clearing review flag on obsolete patch
Comment 14 Yaar Schnitman 2009-09-09 10:03:36 PDT
Patch to 25489 should have fixed this one as well.