Bug 86152 – The order of CSS properties is wrong in background shorthand

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

[Joe Thomas]

As per the specification http://www.w3.org/TR/css3-background/#background, 
"If one <box> value is present then it sets both ‘background-origin’ and ‘background-clip’ to that value. If two values are present, then the first sets ‘background-origin’ and the second ‘background-clip’."

The parsing of these values are done correctly in CSSParser, but the order of these properties returned in StylePropertySet::getPropertyValue for background shorthand property is wrong.

[Joe Thomas]

Created attachment 141284 [details]
simple test case

The order of "border-box padding-box" in the alert message is different from what we set.

[Joe Thomas]

Created attachment 141287 [details]
Patch

[Eric Seidel (no email)]

It seems we could test for this generic class of bugs by feeding the output of getPropertyValue into the cssText and validating that the same result came back out. :)

[Eric Seidel (no email)]

It seems we could test for this generic class of bugs by feeding the output of getPropertyValue into the cssText and validating that the same result came back out. :)

[Joe Thomas]

(In reply to comment #4)
> It seems we could test for this generic class of bugs by feeding the output of getPropertyValue into the cssText and validating that the same result came back out. :)

getPropertyValue for background shorthand property does not return background-orgin and background-clip. Bug 86155 is created to address this.

[Eric Seidel (no email)]

Oh, I was not suggesting you needed to change your testing here.  I was only mentioning that we could ensure more generally that we don't have any other bugs like this with a separate more generic effort.

[Joe Thomas]

(In reply to comment #6)
> Oh, I was not suggesting you needed to change your testing here.  I was only mentioning that we could ensure more generally that we don't have any other bugs like this with a separate more generic effort.

OK :)

[WebKit Review Bot]

Comment on attachment 141287 [details]
Patch

Attachment 141287 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12664577

New failing tests:
plugins/embed-attributes-style.html

[WebKit Review Bot]

Created attachment 141330 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Joe Thomas]

(In reply to comment #8)
> (From update of attachment 141287 [details])
> Attachment 141287 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/12664577
> 
> New failing tests:
> plugins/embed-attributes-style.html

Not related to the patch.

This testcase is failing on Chromium bot also. (http://build.webkit.org/builders/Chromium%20Linux%20Release%20%28Tests%29/builds/32397/steps/layout-test/logs/stdio)

[Joe Thomas]

(In reply to comment #10)
> (In reply to comment #8)
> > (From update of attachment 141287 [details] [details])
> > Attachment 141287 [details] [details] did not pass chromium-ews (chromium-xvfb):
> > Output: http://queues.webkit.org/results/12664577
> > 
> > New failing tests:
> > plugins/embed-attributes-style.html
> 
> Not related to the patch.
> 
> This testcase is failing on Chromium bot also. (http://build.webkit.org/builders/Chromium%20Linux%20Release%20%28Tests%29/builds/32397/steps/layout-test/logs/stdio)

Bug 86170.

[Joe Thomas]

Created attachment 141461 [details]
patch

Re-posting the patch to make Chromium build green.

[Alexis Menard (darktears)]

(In reply to comment #4)
> It seems we could test for this generic class of bugs by feeding the output of getPropertyValue into the cssText and validating that the same result came back out. :)

fast/css/getComputedStyle/getComputedStyle-border-radius-shorthand.html has a checkComputedStyleValue() which is almost what you describe. I think we should generalize this pattern. Whatever output we produce it should be valid output that you can set back somewhere else.

[Alexis Menard (darktears)]

Comment on attachment 141461 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=141461&action=review

> LayoutTests/ChangeLog:9
> +        "If one <box> value is present then it sets both âbackground-originâ and âbackground-clipâ to that value. 

Some weird characters here : âbackground-originâ and âbackground-clipâ

> LayoutTests/fast/backgrounds/background-clip-background-origin-order.html:10
> +     layoutTestController.dumpAsText();

Do you need that?

[Joe Thomas]

Created attachment 141717 [details]
Patch-Updated

With review comments incorporated.

[Joe Thomas]

(In reply to comment #14)
> (From update of attachment 141461 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=141461&action=review
> 
> > LayoutTests/ChangeLog:9
> > +        "If one <box> value is present then it sets both âbackground-originâ and âbackground-clipâ to that value. 
> 
> Some weird characters here : âbackground-originâ and âbackground-clipâ
> 
Removed.
> > LayoutTests/fast/backgrounds/background-clip-background-origin-order.html:10
> > +     layoutTestController.dumpAsText();
> 
> Do you need that?
Not required. Removed.

[Tony Chang]

Comment on attachment 141717 [details]
Patch-Updated

It would be nice to have a roundtrip test. That is, you should verify something like:
var originalStyle = foo.style.background;
foo.style.background = foo.style.background;
shouldBe(foo.style.background, originalStyle);

[Ryosuke Niwa]

Comment on attachment 141717 [details]
Patch-Updated

View in context: https://bugs.webkit.org/attachment.cgi?id=141717&action=review

This looks landable. As for round-trip tests, we should probably create a generic one that tests all CSS properties (in a separate bug/patch of course).

> LayoutTests/fast/backgrounds/background-clip-background-origin-order.html:9
> + description("Bug 86152 - The order of background-origin and background-clip is wrong in background shorthand");

We don't normally use one-space indentation. Please outdent or use 4-spaces.

[Joe Thomas]

(In reply to comment #18)
> (From update of attachment 141717 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=141717&action=review
> 
> This looks landable. As for round-trip tests, we should probably create a generic one that tests all CSS properties (in a separate bug/patch of course).
> 
I already created round-trip test for background shorthand property in Bug 86782. I will update this patch soon. 

Also we are planning to change the order of properties to match the spec. Please see https://bugs.webkit.org/show_bug.cgi?id=86155#c6

> > LayoutTests/fast/backgrounds/background-clip-background-origin-order.html:9
> > + description("Bug 86152 - The order of background-origin and background-clip is wrong in background shorthand");
> 
> We don't normally use one-space indentation. Please outdent or use 4-spaces.
Sure. I will change it.

[Joe Thomas]

Created attachment 142827 [details]
Patch

The new order of properties will be 

CSSPropertyBackgroundImage
CSSPropertyBackgroundPosition
CSSPropertyBackgroundSize
CSSPropertyBackgroundRepeat
CSSPropertyBackgroundAttachment
CSSPropertyBackgroundOrigin
CSSPropertyBackgroundClip
CSSPropertyBackgroundColor

[WebKit Review Bot]

Comment on attachment 142827 [details]
Patch

Attachment 142827 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12726640

New failing tests:
fast/dom/background-shorthand-csstext.html

[WebKit Review Bot]

Created attachment 142846 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Joe Thomas]

Created attachment 142865 [details]
Patch-fixes-layout-test

[Ryosuke Niwa]

Comment on attachment 142865 [details]
Patch-fixes-layout-test

View in context: https://bugs.webkit.org/attachment.cgi?id=142865&action=review

> Source/WebCore/css/StylePropertyShorthand.cpp:39
>          CSSPropertyBackgroundOrigin,
> -        CSSPropertyBackgroundPositionX,
> -        CSSPropertyBackgroundPositionY,
> -        CSSPropertyBackgroundSize
> +        CSSPropertyBackgroundClip,

The said specification doesn't seem to say anything about origin and clip.

[Joe Thomas]

(In reply to comment #24)
> (From update of attachment 142865 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=142865&action=review
> 
> > Source/WebCore/css/StylePropertyShorthand.cpp:39
> >          CSSPropertyBackgroundOrigin,
> > -        CSSPropertyBackgroundPositionX,
> > -        CSSPropertyBackgroundPositionY,
> > -        CSSPropertyBackgroundSize
> > +        CSSPropertyBackgroundClip,
> 
> The said specification doesn't seem to say anything about origin and clip.

Paraphrasing from the spec http://www.w3.org/TR/css3-background/#background:

If one <box> value is present then it sets both ‘background-origin’ and ‘background-clip’ to that value. If two values are present, then the first sets ‘background-origin’ and the second ‘background-clip’.

[Ryosuke Niwa]

Comment on attachment 142865 [details]
Patch-fixes-layout-test

View in context: https://bugs.webkit.org/attachment.cgi?id=142865&action=review

>>> Source/WebCore/css/StylePropertyShorthand.cpp:39
>>> +        CSSPropertyBackgroundClip,
>> 
>> The said specification doesn't seem to say anything about origin and clip.
> 
> Paraphrasing from the spec http://www.w3.org/TR/css3-background/#background:
> 
> If one <box> value is present then it sets both ‘background-origin’ and ‘background-clip’ to that value. If two values are present, then the first sets ‘background-origin’ and the second ‘background-clip’.

Ok.

[Ryosuke Niwa]

Comment on attachment 142865 [details]
Patch-fixes-layout-test

hang on, where did fast/backgrounds/background-clip-background-origin-order.html go?

[Joe Thomas]

(In reply to comment #27)
> (From update of attachment 142865 [details])
> hang on, where did fast/backgrounds/background-clip-background-origin-order.html go?

The order of the CSS properties are very well tested with the existing test case LayoutTests/fast/backgrounds/background-shorthand-with-backgroundSize-style.html after I added round-trip testing to this with bug 86782. So I thought there is no need to introduce one more test case.

[Ryosuke Niwa]

Comment on attachment 142865 [details]
Patch-fixes-layout-test

(In reply to comment #28)
> (In reply to comment #27)
> > (From update of attachment 142865 [details] [details])
> > hang on, where did fast/backgrounds/background-clip-background-origin-order.html go?
> 
> The order of the CSS properties are very well tested with the existing test case LayoutTests/fast/backgrounds/background-shorthand-with-backgroundSize-style.html after I added round-trip testing to this with bug 86782. So I thought there is no need to introduce one more test case.

Okay. Fair enough.

[WebKit Review Bot]

Comment on attachment 142865 [details]
Patch-fixes-layout-test

Clearing flags on attachment: 142865

Committed r117694: <http://trac.webkit.org/changeset/117694>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Joe Thomas]

Thanks for the review !