Summary: | CSS property value serialization order is out of whack | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Dimitri Glazkov (Google) <dglazkov> | ||||||||
Component: | CSS | Assignee: | 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
Dimitri Glazkov (Google)
2009-06-19 09:00:27 PDT
Created attachment 31545 [details]
Test case (almost layout test).
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.
Perhaps you could bring up this issue on #webkit and see if there are ideas that come out of that? 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. 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 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.
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). 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, Would gladly fix the patch, but where should the reference be placed? In Changelog. Moreover, adding a layout test would be better. 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 on attachment 38486 [details]
A Fix
r=me, totally looks fine.
Comment on attachment 38486 [details]
A Fix
Clearing review flag on obsolete patch
Patch to 25489 should have fixed this one as well. |