Bug 76697 – border-image should be implemented like a shorthand.

• Component: CSS
• Status: RESOLVED FIXED

[Alexis Menard (darktears)]

border-image should behave like a shorthand.

[Alexis Menard (darktears)]

Created attachment 123287 [details]
Patch

[WebKit Review Bot]

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

[Alexis Menard (darktears)]

(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.

[Alexis Menard (darktears)]

Created attachment 123322 [details]
Patch

[Darin Adler]

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.

[Alexis Menard (darktears)]

(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.

[WebKit Review Bot]

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

[Alexis Menard (darktears)]

Created attachment 123769 [details]
Patch

[Tony Chang]

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?

[Luke Macpherson]

Bonus points if you can do the same kind of cleanup with -webkit-border-image in a follow-up patch :)

[Alexis Menard (darktears)]

Created attachment 123925 [details]
Patch

[Alexis Menard (darktears)]

Created attachment 123926 [details]
Patch

[Alexis Menard (darktears)]

(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.

[Alexis Menard (darktears)]

Created attachment 123955 [details]
Patch

[Alexis Menard (darktears)]

(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.

[Tony Chang]

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).

[Alexis Menard (darktears)]

Created attachment 123961 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 123961 [details]
Patch for landing

Clearing flags on attachment: 123961

Committed r105901: <http://trac.webkit.org/changeset/105901>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Alexis Menard (darktears)]

(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).