WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
86155
getComputedStyle for background shorthand property does not return background-origin and background-clip
https://bugs.webkit.org/show_bug.cgi?id=86155
Summary
getComputedStyle for background shorthand property does not return background...
Joe Thomas
Reported
2012-05-10 16:33:31 PDT
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
.
Attachments
simple test case
(355 bytes, text/html)
2012-05-10 16:33 PDT
,
Joe Thomas
no flags
Details
patch
(62.31 KB, text/plain)
2012-05-11 01:21 PDT
,
Joe Thomas
no flags
Details
patch
(81.37 KB, patch)
2012-05-20 08:40 PDT
,
Joe Thomas
no flags
Details
Formatted Diff
Diff
patch
(59.58 KB, patch)
2012-05-22 00:20 PDT
,
Joe Thomas
no flags
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Joe Thomas
Comment 1
2012-05-11 01:21:33 PDT
Created
attachment 141351
[details]
patch
Tony Chang
Comment 2
2012-05-16 15:50:11 PDT
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
Joe Thomas
Comment 3
2012-05-16 16:25:56 PDT
(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.
Tony Chang
Comment 4
2012-05-16 16:29:39 PDT
(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?
Joe Thomas
Comment 5
2012-05-16 16:41:56 PDT
(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,
Tony Chang
Comment 6
2012-05-16 16:58:56 PDT
(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!
Joe Thomas
Comment 7
2012-05-17 08:36:06 PDT
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.
Joe Thomas
Comment 8
2012-05-18 12:29:09 PDT
Comment on
attachment 141351
[details]
patch making the patch obsolete. I will update the patch soon.
Joe Thomas
Comment 9
2012-05-20 08:40:01 PDT
Created
attachment 142905
[details]
patch
Tony Chang
Comment 10
2012-05-21 09:23:55 PDT
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)?
Joe Thomas
Comment 11
2012-05-21 09:37:43 PDT
(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.
Ryosuke Niwa
Comment 12
2012-05-21 09:41:04 PDT
(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.
Tony Chang
Comment 13
2012-05-21 10:37:53 PDT
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.
Ryosuke Niwa
Comment 14
2012-05-21 10:43:54 PDT
(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.
Joe Thomas
Comment 15
2012-05-21 12:04:05 PDT
(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.
Joe Thomas
Comment 16
2012-05-21 16:31:29 PDT
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.
Ryosuke Niwa
Comment 17
2012-05-21 16:45:09 PDT
(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.
Joe Thomas
Comment 18
2012-05-21 20:19:30 PDT
> (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?
Ryosuke Niwa
Comment 19
2012-05-21 20:24:38 PDT
(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.
Joe Thomas
Comment 20
2012-05-22 00:20:12 PDT
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>
Joe Thomas
Comment 21
2012-05-22 00:29:40 PDT
(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.
Tony Chang
Comment 22
2012-05-22 10:14:35 PDT
(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.
Joe Thomas
Comment 23
2012-05-23 11:37:16 PDT
(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.
Joe Thomas
Comment 24
2012-05-31 11:33:49 PDT
(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
Joe Thomas
Comment 25
2012-05-31 16:10:47 PDT
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
).
Joe Thomas
Comment 26
2012-05-31 22:16:41 PDT
As per the discussion in the www-style WG(
comment #25
), the patch looks valid. Can someone please review it?
Ryosuke Niwa
Comment 27
2012-05-31 23:15:06 PDT
(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.
Tony Chang
Comment 28
2012-06-01 10:46:04 PDT
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?
Ryosuke Niwa
Comment 29
2012-06-01 11:17:28 PDT
(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 :( )
Tony Chang
Comment 30
2012-06-01 11:42:40 PDT
(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
WebKit Review Bot
Comment 31
2012-06-01 12:00:42 PDT
Comment on
attachment 143209
[details]
patch Clearing flags on attachment: 143209 Committed
r119259
: <
http://trac.webkit.org/changeset/119259
>
WebKit Review Bot
Comment 32
2012-06-01 12:00:48 PDT
All reviewed patches have been landed. Closing bug.
Ryosuke Niwa
Comment 33
2012-06-01 12:19:40 PDT
(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.
Joe Thomas
Comment 34
2012-06-01 12:30:38 PDT
(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.
Joe Thomas
Comment 35
2012-06-05 12:09:37 PDT
(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.
Ryosuke Niwa
Comment 36
2012-06-05 13:18:09 PDT
(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.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug