Bug 8353 – CSS3: Implement background-size property

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Beth Dakin]

We should implement the CSS3 background-size property.

[Beth Dakin]

Created attachment 7666 [details]
First try at implementing background-size

Here is my first crack at implementing background-size. This patch does not take care of parsing the shorthand background-size property yet because we are still waiting to hear back from the CSS working group about the right syntax for this. I have been testing this a lot and have fixed all of the problems I have found. Except for the two layout test failures, which I can't quite figure out. These two tests fail:

fast/inspector/style.html
fast/css/invalidation-errors-2.html

I am not sure the first is a real failure, but the second is definitely some problem with invalidation. There are some other things in the implementation that I am not sure are quite right, so I would love some feedback.

[Darin Adler]

Comment on attachment 7666 [details]
First try at implementing background-size

I think scaleRect is not really an arbitrary rect; it's more like just a size (a rect where the pont is ignored)? It looks like in some code paths only the size of scaleRect is used and in other paths the entire rect is used. That seems wrong. Instead you should just take a size.

+typedef struct BackgroundSize {
+    Length width;
+    Length height;
+} BackgroundSize;

In C++ there's no need to do this. Just "struct BackgroundSize { }" does the same thing, without the typedef or repeating the type twice. I also think that the type should perhaps not include the name Background, since it's just a "Length-based size". We should consider further what the right name for it is. This would be confusing, but a consistent name for it would be LengthSize (consistent with IntSize and FloatSize).

+    void setBackgroundSize(BackgroundSize b) { m_backgroundSize = b; m_backgroundSizeSet = true; }

Probably best to use a const BackgroundSize& here for the parameter.

+    static BackgroundSize initialBackgroundSize() {
+        BackgroundSize b;
+        b.width = Length(Auto);
+        b.height = Length(Auto);
+        return b;
+    }

No need for the code here. Length already defaults to Auto, so just "return BackgroundSize()" will do the same thing.

+    m_backgroundSize.width = o.m_backgroundSize.width;
+    m_backgroundSize.height = o.m_backgroundSize.height;

No need to assign the two separately. Can just do m_backgroundSize = o.m_backgroundSize.

+            curr->m_backgroundSize.width = pattern->m_backgroundSize.width;
+            curr->m_backgroundSize.height = pattern->m_backgroundSize.height;

Same here.

Comment about Pair claims we only use it for border-radius.

Would be nice to have a constructor for Pair instead of having to call setFirst and setSecond separately.

[Dave Hyatt]

I agree with all of Darin's comments.  In addition, there's a real API problem now with the names drawTiledImage and drawScaledAndTiledImage.  With your changes, drawTiledImage now scales.

Unfortunately drawScaledAndTiledImage doesn't really do what you want either, and studying it shows me that border-image *does* in fact have a high DPI deficiency and does not give you a way of specifying the tile size.

I think we should perhaps just go ahead and rename these methods to actually reflect what they're drawing rather than trying to make them generically named.  So my suggestion would be to rename drawTiledImage to drawBackgroundImage and to rename drawScaledAndTiledImage to drawBorderImage.

Then rename the Image API methods to have the same name (drop the "InRect" from them):

drawInRect -> draw
tileInRect -> drawBackgroundImage
scaleAndTileInRect -> drawBorderImage

Also, you're breaking Windows.  You need to patch its GraphicsContext and Image code as well.

[Beth Dakin]

Created attachment 7682 [details]
Patch with feedback

[Beth Dakin]

Here is a new patch that I believe addresses all of the issues that Dave and Darin brought up in their comments. I also changed background-size to be called -khtml-background-size. I would particularly like some feedback on the patched Cairo code since there was a lot of guess-work there as I cannot currently compile said code. I am now investigating the two layout test failures.

[Beth Dakin]

I just talked with Tim about my layout test failures, and one of them is not a failure. I just need to generate new results for that one. The other one is definitely a real problem though (fast/css/invalidation-errors-2.html). Investigating that now...

[Beth Dakin]

Created attachment 7689 [details]
Passes layout tests

The one remaining layout test failure was because I had included background-size in the list of css background shorthand properties even though we haven't added support for actually parsing it yet. SO, this patch passes all of the layout tests, addresses all of the feedback I have recieved so far, and tries to also patch the Windows side. It still doesn't parse background-size as part of the background shorthand.

[Darin Adler]

Comment on attachment 7689 [details]
Passes layout tests

This can almost certainly land as-is, but I have a few nit picks (optional to deal with these).

I think we can come up with a better name for the argument:

    const IntSize& tileRect

This is a size, not a rect. And as long as we're changing the interface for all callers I suggest that sx and sy be combined into const IntPoint& srcPoint, at some point. We'll be changing this again, if only to get rid of "nativeData", so I suppose we don't have to do this now.

I think that Image::drawBackgroundImage should be Image::drawBackground and Image::drawBorderImage should be Image::drawBorder.

I think drawTiled is still a better name than drawBackground. I agree drawBorder is the best name for the one that formerly had a long name.

Why is the scale parameter to Image::drawBackgroundImage the only int-based parameter? All the rest are floats.

-            int pixh = bg->imageSize().height();
+            int pixh = bg->imageSize().height();            

This change seems to be addition of trailing spaces. Lets no do that.

+                case CSS_PROP__KHTML_BACKGROUND_SIZE: {
+                    currValue = parseBackgroundSize();
+                    if (currValue)
+                        valueList->next();
+                    break;
+                }

No braces needed here.

+    else if(firstType == CSSPrimitiveValue::CSS_PERCENTAGE)

+    else if(secondType == CSSPrimitiveValue::CSS_PERCENTAGE)

Missing spaces on these lines.

[Beth Dakin]

Created attachment 7702 [details]
A few adjustments for Darin's comments

Here is a new patch with Darin's comments all addressed.

[Beth Dakin]

Created attachment 7703 [details]
Includes layout tests

[Dave Hyatt]

Comment on attachment 7703 [details]
Includes layout tests

(1) Let's just revert the image methods back to their original names.

(2) The norepeat argument doesn't really work.  The higher level code can just call draw directly with the right dstRect.  You should not have to deal with norepeat explicitly in the tiling method.

(3) For both Cairo and Mac, the case where the single tile is bigger than the destination rect is wrong.  It's basically backwards.  You should always be drawing into the original dstRect, and then you have to go from the scaled tile back to the original unscaled portion of the image.  Then when the draw happens the scale will occur in the right direction.

[Beth Dakin]

Created attachment 7784 [details]
Okay, better

Here is my newest patch with Dave's latest comments taken into account.

[Dave Hyatt]

There's a place where you check for m_backgroundSize.width == o.m_backgroundSize.width && height == height, and you can just compare the two m_backgroundSizes.

In css_valueimpl.cpp, remove background-size from the shorthand for now.

Make sure the Cairo code works.

We need background-size tests for inlines that wrap to multiple lines.  Take some of these tests and apply them to <span>s that wrap to multiple lines.

[Beth Dakin]

Created attachment 7792 [details]
Most newest

Here is a patch that addresses all of Hyatt's most recent comments. Actually, though, it didn't change the: m_backgroundSize.width == o.m_backgroundSize.width && height == height, that Dave noticed because I got a build error when I just tried to compare the background sized directly. Other than that, though, this patch addresses everything else and even works on windows!

[Beth Dakin]

Created attachment 7803 [details]
No, this one!

I noticed a few minor things to clean up in the last patch a posted, so here is a newer one.

[Dave Hyatt]

Comment on attachment 7803 [details]
No, this one!

r=me, file a followup bug about the shorthand.

[Beth Dakin]

I committed the fix and filed the following follow-up bugs:

http://bugzilla.opendarwin.org/show_bug.cgi?id=8464 Add background-size to the background shorthand

http://bugzilla.opendarwin.org/show_bug.cgi?id=8465 getComputedStyle() will not recgonize a Pair as a valid CSSPrimitiveValue