Bug 27573 – [CSS3 Backgrounds and Borders] Add support for the "contain" value for background-size

• Component: CSS
• Status: RESOLVED FIXED

[Dave Hyatt]

background-size has a new "contain" value:

Scale the image, while preserving its intrinsic aspect ratio (if any), to the largest size such that both its width and its height can fit inside the background positioning area.

[Beth Dakin]

Created attachment 35132 [details]
Patch for the CSS part of contain and cover

Here is a patch for the CSS part of supporting contain and cover as values for background-size. There are a few things that I am unsure about:

1. Should the code that is share with WebKitMaskSize be broken out so that WebKitMaskSize does not support contain and cover? Right now, I implemented it to continue matching background-size.
2. In CSSPrimitiveValueMappings.h I was unsure what to return in the normal case when the value is a pair of numbers.

Those are the main things, I think.

Comment on attachment 35132 [details]
Patch for the CSS part of contain and cover

Review comments. I has them.

> +        Return contain or cover when appropriate, and otherwise so what we 
> +        used to do.

Typo: “so”.

> +        backgroundSize() now returns an EBackgroundSize, and 
> +        backgroundSizeLength() returns the numbers

I think you meant “lengths” since they can still be 'auto' or a percentage.



>  #
> +# css_prop_background_size
> +#

We no longer have these weird enum values for the properties, and I never understood why the comments were in term of those. You can just say “background-size”.

> Index: WebCore/rendering/RenderBoxModelObject.cpp
> ===================================================================
> --- WebCore/rendering/RenderBoxModelObject.cpp	(revision 47461)
> +++ WebCore/rendering/RenderBoxModelObject.cpp	(working copy)
> @@ -497,8 +497,8 @@ IntSize RenderBoxModelObject::calculateB
>      if (bgLayer->isSizeSet()) {

Not sure why you didn’t change this one to isSizeLengthSet()…

> Index: WebCore/rendering/RenderObject.cpp
> ===================================================================
> --- WebCore/rendering/RenderObject.cpp	(revision 47461)
> +++ WebCore/rendering/RenderObject.cpp	(working copy)
> @@ -656,8 +656,8 @@ static bool mustRepaintFillLayers(const 
>      if (!layer->xPosition().isZero() || !layer->yPosition().isZero())
>          return true;
>  
> -    if (layer->isSizeSet()) {
> -        if (layer->size().width().isPercent() || layer->size().height().isPercent())
> +    if (layer->isSizeLengthSet()) {
> +        if (layer->sizeLength().width().isPercent() || layer->sizeLength().height().isPercent())

…because you did change this one.

I think instead of adding another “is set” bit, you can get rid of the existing one and just add a value to the EBackgroundSize enum representing the “not set” state. But now I am not sure I fully understand why we need this notion in the first place and what’s the difference between “not set” and 'background-size: auto, auto'.

(In reply to comment #2)
> But now I am not sure I fully understand why we need this notion in the
> first place and what’s the difference between “not set” and 'background-size:
> auto, auto'.

Oh, it’s about filling missing values.

[Beth Dakin]

Created attachment 35175 [details]
Patch with Dan comments

Here is a patch that addresses all of Dan's comments. Thanks Dan!

[Beth Dakin]

I had a third patch that Dan reviewed, and I committed it with r47630.

[Beth Dakin]

Comment on attachment 35175 [details]
Patch with Dan comments

R-minusing this one since I had a third patch that has already been reviewed and landed.

Created attachment 38406 [details]
Rendering changes for "contain" and "cover"

[Beth Dakin]

Comment on attachment 38406 [details]
Rendering changes for "contain" and "cover"

Looks good!

Landed the rendering changes as <http://trac.webkit.org/projects/webkit/changeset/47650>.