Summary: | The order of CSS properties is wrong in background shorthand | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Joe Thomas <joethomas> | ||||||||||||||||||
Component: | CSS | Assignee: | Joe Thomas <joethomas> | ||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||
Severity: | Normal | CC: | dglazkov, eric, kling, koivisto, macpherson, menard, rniwa, tony, webkit.review.bot | ||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||
Bug Depends on: | 86739 | ||||||||||||||||||||
Bug Blocks: | |||||||||||||||||||||
Attachments: |
|
Description
Joe Thomas
2012-05-10 15:49:23 PDT
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. :) 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 ! |