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.
Created attachment 141284 [details] simple test case The order of "border-box padding-box" in the alert message is different from what we set.
Created attachment 141287 [details] Patch
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. :)
(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.
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.
(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 :)
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
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
(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)
(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.
Created attachment 141461 [details] patch Re-posting the patch to make Chromium build green.
(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.
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?
Created attachment 141717 [details] Patch-Updated With review comments incorporated.
(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.
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);
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.
(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.
Created attachment 142827 [details] Patch The new order of properties will be CSSPropertyBackgroundImage CSSPropertyBackgroundPosition CSSPropertyBackgroundSize CSSPropertyBackgroundRepeat CSSPropertyBackgroundAttachment CSSPropertyBackgroundOrigin CSSPropertyBackgroundClip CSSPropertyBackgroundColor
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
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
Created attachment 142865 [details] Patch-fixes-layout-test
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.
(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’.
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.
Comment on attachment 142865 [details] Patch-fixes-layout-test hang on, where did fast/backgrounds/background-clip-background-origin-order.html go?
(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.
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.
Comment on attachment 142865 [details] Patch-fixes-layout-test Clearing flags on attachment: 142865 Committed r117694: <http://trac.webkit.org/changeset/117694>
All reviewed patches have been landed. Closing bug.
Thanks for the review !