Summary: | border-image should be implemented like a shorthand. | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alexis Menard (darktears) <menard> | ||||||||||||||||
Component: | CSS | Assignee: | Alexis Menard (darktears) <menard> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | darin, dglazkov, hyatt, macpherson, tony, webkit.review.bot | ||||||||||||||||
Priority: | P2 | ||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||
Hardware: | All | ||||||||||||||||||
OS: | All | ||||||||||||||||||
Attachments: |
|
Description
Alexis Menard (darktears)
2012-01-20 04:12:58 PST
Created attachment 123287 [details]
Patch
Comment on attachment 123287 [details] Patch Attachment 123287 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11246097 New failing tests: animations/cross-fade-border-image-source.html (In reply to comment #2) > (From update of attachment 123287 [details]) > Attachment 123287 [details] did not pass chromium-ews (chromium-xvfb): > Output: http://queues.webkit.org/results/11246097 > > New failing tests: > animations/cross-fade-border-image-source.html I can't reproduce that one here. Created attachment 123322 [details]
Patch
Comment on attachment 123322 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=123322&action=review > Source/WebCore/ChangeLog:14 > + No new tests : Existing tests should cover that there is no behavior regression. That doesn’t make sense. I am pretty sure there must be a behavior change, for example, the values you get for the longhand properties from computed style. Or if there is no behavior change here, then why are we making the code change? Seems like enough new code that it’s not worthwhile if it has no perceptible effect. (In reply to comment #5) > (From update of attachment 123322 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=123322&action=review > > > Source/WebCore/ChangeLog:14 > > + No new tests : Existing tests should cover that there is no behavior regression. > > That doesn’t make sense. I am pretty sure there must be a behavior change, for example, the values you get for the longhand properties from computed style. The longhands were properly set even before. > > Or if there is no behavior change here, then why are we making the code change? Seems like enough new code that it’s not worthwhile if it has no perceptible effect. It has no perceptible effect in this patch (though a previous one changed the actual CSSValue type we return for border-image; before it was a custom CSSValue type). The motivation of the code change here is to be consistent with other shorthands, border-image should not be different the way it is implemented than let say border. If it is different today it's because of legacy : -webkit-border-image. With this change the day we want to remove -webkit-border-image (if it happens one day) you can just trash void CSSStyleSelector::mapNinePieceImage(CSSPropertyID property, CSSValue* value, NinePieceImage& image) for example as it is not needed anymore for border-image. It's just about consistency here. Comment on attachment 123322 [details] Patch Attachment 123322 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11246180 New failing tests: animations/cross-fade-border-image-source.html Created attachment 123769 [details]
Patch
Comment on attachment 123769 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=123769&action=review This will change what element.style.length returns when border-image is set, right? Can you add a layout test for that? That's a good behavior change in that it will act more like other shorthands. > Source/WebCore/css/CSSParser.cpp:1483 > + if (parseBorderImage(propId, result, important)) { > + if (propId != CSSPropertyBorderImage) > + addProperty(propId, result, important); I would split the case statements into 2 blocks, one for WebkitBorderImage and one for BorderImage+WebkitMaskBoxImage. > Source/WebCore/css/CSSParser.cpp:5304 > + : m_parser(parser) Rather than passing in the parser to the constructor, can we just pass in the parser to commitBorderImage (and forward it on to commitBorderImageProperty)? > Source/WebCore/css/CSSStyleApplyProperty.cpp:864 > - image.setOutset(LengthBox()); > + image.setOutset(LengthBox(0)); Was this bug revealed by this change or was this not a problem before? Maybe this should be a separate fix with test case? Bonus points if you can do the same kind of cleanup with -webkit-border-image in a follow-up patch :) Created attachment 123925 [details]
Patch
Created attachment 123926 [details]
Patch
(In reply to comment #9) > (From update of attachment 123769 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=123769&action=review > > This will change what element.style.length returns when border-image is set, right? Can you add a layout test for that? That's a good behavior change in that it will act more like other shorthands. Yes it is right, I forgot about that one. Done. > > > Source/WebCore/css/CSSParser.cpp:1483 > > + if (parseBorderImage(propId, result, important)) { > > + if (propId != CSSPropertyBorderImage) > > + addProperty(propId, result, important); > > I would split the case statements into 2 blocks, one for WebkitBorderImage and one for BorderImage+WebkitMaskBoxImage. Fixed. > > > Source/WebCore/css/CSSParser.cpp:5304 > > + : m_parser(parser) > > Rather than passing in the parser to the constructor, can we just pass in the parser to commitBorderImage (and forward it on to commitBorderImageProperty)? Fixed. > > > Source/WebCore/css/CSSStyleApplyProperty.cpp:864 > > - image.setOutset(LengthBox()); > > + image.setOutset(LengthBox(0)); > > Was this bug revealed by this change or was this not a problem before? Maybe this should be a separate fix with test case? Actually the bug was revealed with that change as we now use ApplyPropertyExpanding when we set the initial value of the outset (if the latter is not specified). Existing layout tests were failing when the outset was omitted (because it will get an implicit default value), its initial value was set to auto instead of 0 as specified by border-image-outset spec. Created attachment 123955 [details]
Patch
(In reply to comment #14) > Created an attachment (id=123955) [details] > Patch Just a minor fix in the test. Got rid of a useless line. Comment on attachment 123955 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=123955&action=review > Source/WebCore/page/animation/AnimationBase.cpp:1123 > CSSPropertyBorderSpacing, > + CSSPropertyBorderImage, Nit: alphabetize > LayoutTests/fast/css/border-image-style-length.html:19 > +e = document.getElementById('test'); > +e.style.borderImage = "url(dummy://test.png) 10 / 13px 1.5em 1em 10px"; > +shouldBe("e.style.length", "5"); Nit: I would also add a test case for webkitBorderImage (make sure it doesn't expand). Created attachment 123961 [details]
Patch for landing
Comment on attachment 123961 [details] Patch for landing Clearing flags on attachment: 123961 Committed r105901: <http://trac.webkit.org/changeset/105901> All reviewed patches have been landed. Closing bug. (In reply to comment #10) > Bonus points if you can do the same kind of cleanup with -webkit-border-image in a follow-up patch :) What do you mean? -webkit-border-image doesn't have longhands, I'm not sure how I could cleanup more as I did (got rid of CSSBorderImageValue, return a CSSValueList in CSSComputedStyleDeclaration). |