Bug 27573 - [CSS3 Backgrounds and Borders] Add support for the "contain" value for background-size
Summary: [CSS3 Backgrounds and Borders] Add support for the "contain" value for backgr...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 27569
  Show dependency treegraph
 
Reported: 2009-07-22 15:43 PDT by Dave Hyatt
Modified: 2009-08-21 17:34 PDT (History)
2 users (show)

See Also:


Attachments
Patch for the CSS part of contain and cover (20.55 KB, patch)
2009-08-19 12:45 PDT, Beth Dakin
no flags Details | Formatted Diff | Diff
Patch with Dan comments (20.04 KB, patch)
2009-08-19 18:43 PDT, Beth Dakin
bdakin: review-
Details | Formatted Diff | Diff
Rendering changes for "contain" and "cover" (606.93 KB, patch)
2009-08-21 15:28 PDT, mitz
bdakin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dave Hyatt 2009-07-22 15:43:19 PDT
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.
Comment 1 Beth Dakin 2009-08-19 12:45:35 PDT
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 2 mitz 2009-08-19 14:18:38 PDT
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'.
Comment 3 mitz 2009-08-19 14:20:30 PDT
(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.
Comment 4 Beth Dakin 2009-08-19 18:43:00 PDT
Created attachment 35175 [details]
Patch with Dan comments

Here is a patch that addresses all of Dan's comments. Thanks Dan!
Comment 5 Beth Dakin 2009-08-21 12:33:41 PDT
I had a third patch that Dan reviewed, and I committed it with r47630.
Comment 6 Beth Dakin 2009-08-21 12:35:17 PDT
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.
Comment 7 mitz 2009-08-21 15:28:37 PDT
Created attachment 38406 [details]
Rendering changes for "contain" and "cover"
Comment 8 Beth Dakin 2009-08-21 15:42:41 PDT
Comment on attachment 38406 [details]
Rendering changes for "contain" and "cover"

Looks good!
Comment 9 mitz 2009-08-21 17:34:12 PDT
Landed the rendering changes as <http://trac.webkit.org/projects/webkit/changeset/47650>.