Created attachment 141291 [details] simple test case background-origin and background-clip are not returned in getComputedStyle for background shorthand property. The specification related to this is at http://www.w3.org/TR/css3-background/#background.
Created attachment 141351 [details] patch
Comment on attachment 141351 [details] patch Why does the position come last? In the spec, it looks like background-origin and background-clip come last. http://www.w3.org/TR/css3-background/#background
(In reply to comment #2) > (From update of attachment 141351 [details]) > Why does the position come last? In the spec, it looks like background-origin and background-clip come last. > http://www.w3.org/TR/css3-background/#background I think the order is not so strict. Even CSSParser allows the properties to be specified in any order in the background shorthand property. I think it is only strict about background-origin to be specified before background-clip and background-position before background-size. Please correct me if am wrong.
(In reply to comment #3) > (In reply to comment #2) > > (From update of attachment 141351 [details] [details]) > > Why does the position come last? In the spec, it looks like background-origin and background-clip come last. > > http://www.w3.org/TR/css3-background/#background > > I think the order is not so strict. Even CSSParser allows the properties to be specified in any order in the background shorthand property. I think it is only strict about background-origin to be specified before background-clip and background-position before background-size. Please correct me if am wrong. That's true, both are something that we can parse correctly. However, I think we should strive to follow the order that it is spec'ed to better match what people will write. E.g., borders are often written as "1px solid blue" so getComputedStyle on a border should return the values in that order. background-origin and background-clip are pretty new. Do you think that people will likely write it in the order we've chosen?
(In reply to comment #4) > (In reply to comment #3) > > (In reply to comment #2) > > > (From update of attachment 141351 [details] [details] [details]) > > > Why does the position come last? In the spec, it looks like background-origin and background-clip come last. > > > http://www.w3.org/TR/css3-background/#background > > > > I think the order is not so strict. Even CSSParser allows the properties to be specified in any order in the background shorthand property. I think it is only strict about background-origin to be specified before background-clip and background-position before background-size. Please correct me if am wrong. > > That's true, both are something that we can parse correctly. However, I think we should strive to follow the order that it is spec'ed to better match what people will write. E.g., borders are often written as "1px solid blue" so getComputedStyle on a border should return the values in that order. > > background-origin and background-clip are pretty new. Do you think that people will likely write it in the order we've chosen? Ok. I will make the change. Currently CSSPropertyBackgroundColor is mentioned first, but we need to move it towards the end as per spec. The new order will be CSSPropertyBackgroundImage, CSSPropertyBackgroundPosition, CSSPropertyBackgroundSize, CSSPropertyBackgroundRepeat, CSSPropertyBackgroundAttachment, CSSPropertyBackgroundOrigin, CSSPropertyBackgroundClip, CSSPropertyBackgroundColor,
(In reply to comment #5) > Ok. I will make the change. Currently CSSPropertyBackgroundColor is mentioned first, but we need to move it towards the end as per spec. The new order will be > > CSSPropertyBackgroundImage, > CSSPropertyBackgroundPosition, > CSSPropertyBackgroundSize, > CSSPropertyBackgroundRepeat, > CSSPropertyBackgroundAttachment, > CSSPropertyBackgroundOrigin, > CSSPropertyBackgroundClip, > CSSPropertyBackgroundColor, Yeah, that seems better. Thanks!
Changing the position of background-size from the end to the middle uncovered a bug in CSSParser. The issue is that when the background-size is specified by a single value in background shorthand, parsing is getting failed. Bug 86739 is created for it.
Comment on attachment 141351 [details] patch making the patch obsolete. I will update the patch soon.
Created attachment 142905 [details] patch
Comment on attachment 142905 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=142905&action=review > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2695 > + static const CSSPropertyID properties1[2] = { CSSPropertyBackgroundImage, CSSPropertyBackgroundPosition }; > + > + static const CSSPropertyID properties2[6] = { CSSPropertyBackgroundSize, CSSPropertyBackgroundRepeat, Why do we use 2 arrays here? Can we name these better than properties1 and properties2 (the variables names may make it more clear why there are 2 arrays here)?
(In reply to comment #10) > (From update of attachment 142905 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=142905&action=review > > > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2695 > > + static const CSSPropertyID properties1[2] = { CSSPropertyBackgroundImage, CSSPropertyBackgroundPosition }; > > + > > + static const CSSPropertyID properties2[6] = { CSSPropertyBackgroundSize, CSSPropertyBackgroundRepeat, > > Why do we use 2 arrays here? Can we name these better than properties1 and properties2 (the variables names may make it more clear why there are 2 arrays here)? For adding the '/' between background-position and background-size properties, I am creating a SlashSeperatedList and appending the lists I created from properties1 array and properties2 array. Actually I could not think of any good names for these arrays, so I just named them properties1 and properties2. Please let me know if there are good names for these arrays.
(In reply to comment #11) > For adding the '/' between background-position and background-size properties, I am creating a SlashSeperatedList and appending the lists I created from properties1 array and properties2 array. Actually I could not think of any good names for these arrays, so I just named them properties1 and properties2. Please let me know if there are good names for these arrays. Can old UAs (i.e. IE6) can parse these results correctly? For editing purposes (and for other apps that store CSS values on server-side), it's very important the shorthand values we generate can be parsed by old UAs.
View in context: https://bugs.webkit.org/attachment.cgi?id=142905&action=review >>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2695 >>> + static const CSSPropertyID properties2[6] = { CSSPropertyBackgroundSize, CSSPropertyBackgroundRepeat, >> > For adding the '/' between background-position and background-size properties, I am creating a SlashSeperatedList and appending the lists I created from properties1 array and properties2 array. Actually I could not think of any good names for these arrays, so I just named them properties1 and properties2. Please let me know if there are good names for these arrays. How about propertiesBeforeSlash and propertiesAfterSlash or propertiesBeforeSeparator? (In reply to comment #12) > > Can old UAs (i.e. IE6) can parse these results correctly? For editing purposes (and for other apps that store CSS values on server-side), it's very important the shorthand values we generate can be parsed by old UAs. I think this should be brought up on a spec mailing list (www-style, I guess). It's only recently that WebKit started generating shorthand values. I'm not sure if FireFox or Opera have tried to maintain any backwards compatibility here.
(In reply to comment #13) > (In reply to comment #12) > > Can old UAs (i.e. IE6) can parse these results correctly? For editing purposes (and for other apps that store CSS values on server-side), it's very important the shorthand values we generate can be parsed by old UAs. > > I think this should be brought up on a spec mailing list (www-style, I guess). It's only recently that WebKit started generating shorthand values. I'm not sure if FireFox or Opera have tried to maintain any backwards compatibility here. We absolutely must. If we're not going to maintain the backward compatibility for regular shorthands, then we'll need a separate mechanism to generate backward compatible shorthand notations for editing purposes.
(In reply to comment #14) > (In reply to comment #13) > > (In reply to comment #12) > > > Can old UAs (i.e. IE6) can parse these results correctly? For editing purposes (and for other apps that store CSS values on server-side), it's very important the shorthand values we generate can be parsed by old UAs. > > > > I think this should be brought up on a spec mailing list (www-style, I guess). It's only recently that WebKit started generating shorthand values. I'm not sure if FireFox or Opera have tried to maintain any backwards compatibility here. > > We absolutely must. If we're not going to maintain the backward compatibility for regular shorthands, then we'll need a separate mechanism to generate backward compatible shorthand notations for editing purposes. Webkit started supporting background shorthand in getComputedStyle from Jan 2012 (http://trac.webkit.org/changeset/104169). The current order of CSS properties returned by webkit is as per CSS2.1 spec http://www.w3.org/TR/CSS21/colors.html#background-properties. background-position, background-clip and background-origin are not part of background shorthand property in CSS2.1 spec. Since Webkit already supports all the properties mentioned in background shorthand property in the CSS3 spec (http://www.w3.org/TR/css3-background/#the-background) and considering the fact that webkit started supporting background shorthand in getComputedStyle only recently, it would be better to start generating the background shorthand property as per CSS3 specification. The new order of properties introduced in this patch is as per CSS3 spec.
Checked the behavior of getComputedStyle for background shorthand property on various browsers IE 9 and Firefox 12.0 - return empty. Opera 11.64 - Order mentioned in CSS2.1 spec followed by three new properties added to the shorthand in CSS3(<bg-size>, <bg-origin>, <bg-clip>). Chrome 19.0 - CSS2.1 order Latest Webkit - CSS2.1 order followed by background-size. background-size is added as part of http://trac.webkit.org/changeset/116645 CSS2.1 order - <'background-color'> || <bg-image> || <repeat-style> || <attachment> || <position> CSS3 order - <bg-image> || <position> [ / <bg-size> ]? || <repeat-style> || <attachment> || <bg-origin> || <bg-clip> || <'background-color'> I wil post a query to www-style mailing list asking for clarification on the order.
(In reply to comment #15) > Since Webkit already supports all the properties mentioned in background shorthand property in the CSS3 spec (http://www.w3.org/TR/css3-background/#the-background) and considering the fact that webkit started supporting background shorthand in getComputedStyle only recently, it would be better to start generating the background shorthand property as per CSS3 specification. The new order of properties introduced in this patch is as per CSS3 spec. Not necessarily. The most important question is other UAs, particularly old ones such as IE6, can parse the generated value or not. (In reply to comment #16) > Checked the behavior of getComputedStyle for background shorthand property on various browsers > > IE 9 and Firefox 12.0 - return empty. > > Opera 11.64 - Order mentioned in CSS2.1 spec followed by three new properties added to the shorthand in CSS3(<bg-size>, <bg-origin>, <bg-clip>). > > Chrome 19.0 - CSS2.1 order > > Latest Webkit - CSS2.1 order followed by background-size. background-size is added as part of http://trac.webkit.org/changeset/116645 I'm more interested in knowing whether they can parse the generated values or not, and we definitely need to be compatible with IE6 at least. There are mail clients out there that don't even support as CSS as IE6.
> (In reply to comment #16) > > Checked the behavior of getComputedStyle for background shorthand property on various browsers > > > > IE 9 and Firefox 12.0 - return empty. > > > > Opera 11.64 - Order mentioned in CSS2.1 spec followed by three new properties added to the shorthand in CSS3(<bg-size>, <bg-origin>, <bg-clip>). > > > > Chrome 19.0 - CSS2.1 order > > > > Latest Webkit - CSS2.1 order followed by background-size. background-size is added as part of http://trac.webkit.org/changeset/116645 > > I'm more interested in knowing whether they can parse the generated values or not, and we definitely need to be compatible with IE6 at least. There are mail clients out there that don't even support as CSS as IE6. Tested it on IE 6 on Win XP and here are the observations: IE-6 parses the background shorthand correctly if we set the newly introduced properties to the background shorthand in CSS3 spec towards the end. This is same as what Opera is doing currently. The order will be <bg-color> || <bg-image> || <repeat-style> || <attachment> || <position> || / <bg-size> || <bg-origin> || <bg-clip> IE6 parses the first five properties correctly and ignores the last three. Shall we go ahead with this order?
(In reply to comment #18) > Tested it on IE 6 on Win XP and here are the observations: > > IE-6 parses the background shorthand correctly if we set the newly introduced properties to the background shorthand in CSS3 spec towards the end. This is same as what Opera is doing currently. The order will be > > <bg-color> || <bg-image> || <repeat-style> || <attachment> || <position> || / <bg-size> || <bg-origin> || <bg-clip> > > IE6 parses the first five properties correctly and ignores the last three. > > Shall we go ahead with this order? Yes, please! Backward compatibility is pretty darn important here.
Created attachment 143209 [details] patch Patch with order of properties in background shorthand: <bg-color> || <bg-image> || <repeat-style> || <attachment> || <position> || / <bg-size> || <bg-origin> || <bg-clip>
(In reply to comment #19) > (In reply to comment #18) > > Tested it on IE 6 on Win XP and here are the observations: > > > > IE-6 parses the background shorthand correctly if we set the newly introduced properties to the background shorthand in CSS3 spec towards the end. This is same as what Opera is doing currently. The order will be > > > > <bg-color> || <bg-image> || <repeat-style> || <attachment> || <position> || / <bg-size> || <bg-origin> || <bg-clip> > > > > IE6 parses the first five properties correctly and ignores the last three. > > > > Shall we go ahead with this order? > > Yes, please! Backward compatibility is pretty darn important here. Patch Updated. Thanks.
(In reply to comment #20) > Created an attachment (id=143209) [details] > patch > > Patch with order of properties in background shorthand: <bg-color> || <bg-image> || <repeat-style> || <attachment> || <position> || / <bg-size> || <bg-origin> || <bg-clip> Please provide this feedback to the www-style working group. I bet we can get the spec changed.
(In reply to comment #22) > (In reply to comment #20) > > Created an attachment (id=143209) [details] [details] > > patch > > > > Patch with order of properties in background shorthand: <bg-color> || <bg-image> || <repeat-style> || <attachment> || <position> || / <bg-size> || <bg-origin> || <bg-clip> > > Please provide this feedback to the www-style working group. I bet we can get the spec changed. Sure. I sent the mail to www-style@w3.org yesterday but it's yet to show up in http://lists.w3.org/Archives/Public/www-style/2012May/. I read that messages from first time posters will take 1-2 days to show up.
(In reply to comment #22) > (In reply to comment #20) > > Created an attachment (id=143209) [details] [details] > > patch > > > > Patch with order of properties in background shorthand: <bg-color> || <bg-image> || <repeat-style> || <attachment> || <position> || / <bg-size> || <bg-origin> || <bg-clip> > > Please provide this feedback to the www-style working group. I bet we can get the spec changed. http://lists.w3.org/Archives/Public/www-style/2012May/1202.html
Summarizing the discussion on www-style WG. 1/ The order of background properties are not strict in http://www.w3.org/TR/css3-background/#the-background. || implies that one or more of the alternatives can occur, in any order, as described in http://www.w3.org/TR/CSS21/about.html#value-defs 2/ If any server / server-side app wants to store the generated result from a CSS3 compliant UA and later feed the result into CSS2.1 compliant UA like IE6, the correct solution would be parse it at the server side and serve only the parts that IE6 understands(http://lists.w3.org/Archives/Public/www-style/2012May/1242.html).
As per the discussion in the www-style WG(comment #25), the patch looks valid. Can someone please review it?
(In reply to comment #25) > 2/ If any server / server-side app wants to store the generated result from a CSS3 compliant UA and later feed the result into CSS2.1 compliant UA like IE6, the correct solution would be parse it at the server side and serve only the parts that IE6 understands(http://lists.w3.org/Archives/Public/www-style/2012May/1242.html). I don't think that's a viable option. Anyway, your current patch uses an order that IE6 can parse, so it looks good to me.
Comment on attachment 143209 [details] patch rniwa: Do we use getComputedStyle when serializing HTML that goes into the clipboard? Are we worried about compat there?
(In reply to comment #28) > (From update of attachment 143209 [details]) > rniwa: Do we use getComputedStyle when serializing HTML that goes into the clipboard? Are we worried about compat there? We do. See comments #18 - #20. Given that the new format can be parsed by IE6 (trusting Joe's analysis), this change is probably safe to make. (Btw, I intended to r+ the patch in comment #27. Not sure what happened here :( )
(In reply to comment #29) > (In reply to comment #28) > > (From update of attachment 143209 [details] [details]) > > rniwa: Do we use getComputedStyle when serializing HTML that goes into the clipboard? Are we worried about compat there? > > We do. See comments #18 - #20. Given that the new format can be parsed by IE6 (trusting Joe's analysis), this change is probably safe to make. I bet newer IE7 or IE8 (or Firefox, or older Chrome) will fail to parse it. The CSS spec says that if you come across an unknown token, we should ignore the whole line. IE6 happens to work by not being spec compliant. dbaron[1] points out that this is going to be relatively uncommon at first, since only backgrounds with a background-color, background-clip or background-size will contain incompatible CSS. If this is really a problem, we could have the code for serializing HTML go through a different code path than just calling getComputedStyle from JS. [1] http://lists.w3.org/Archives/Public/www-style/2012Jun/0020.html
Comment on attachment 143209 [details] patch Clearing flags on attachment: 143209 Committed r119259: <http://trac.webkit.org/changeset/119259>
All reviewed patches have been landed. Closing bug.
(In reply to comment #30) > I bet newer IE7 or IE8 (or Firefox, or older Chrome) will fail to parse it. The CSS spec says that if you come across an unknown token, we should ignore the whole line. IE6 happens to work by not being spec compliant. That's annoying :/ Could someone check this? (In reply to comment #30) > If this is really a problem, we could have the code for serializing HTML go through a different code path than just calling getComputedStyle from JS. Yeah, I've started to think we might need that for editing.
(In reply to comment #33) > (In reply to comment #30) > > I bet newer IE7 or IE8 (or Firefox, or older Chrome) will fail to parse it. The CSS spec says that if you come across an unknown token, we should ignore the whole line. IE6 happens to work by not being spec compliant. > > That's annoying :/ Could someone check this? > Sure. I will check this.
(In reply to comment #33) > (In reply to comment #30) > > I bet newer IE7 or IE8 (or Firefox, or older Chrome) will fail to parse it. The CSS spec says that if you come across an unknown token, we should ignore the whole line. IE6 happens to work by not being spec compliant. > > That's annoying :/ Could someone check this? > IE6, IE7 and IE9 parses the tokens correctly till it encounters an unknown token. It will not drop the whole line. So the order defined in this patch works well with IEs. (did not get a IE8 to test with) Latest Opera already supports all the properties as per CSS3 spec. It parses all the properties even if they are out-of-order Firefox and Chrome 19 drops the entire rule if it encounters unknown tokens.
(In reply to comment #35) > > Latest Opera already supports all the properties as per CSS3 spec. It parses all the properties even if they are out-of-order > > Firefox and Chrome 19 drops the entire rule if it encounters unknown tokens. Thanks for the analysis! I'm less worried about Firefox here because Gecko isn't embedded (except Thunderbird of course) in many other applications, particularly by mail clients, like IE is.