WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
8353
CSS3: Implement background-size property
https://bugs.webkit.org/show_bug.cgi?id=8353
Summary
CSS3: Implement background-size property
Beth Dakin
Reported
2006-04-12 21:16:54 PDT
We should implement the CSS3 background-size property.
Attachments
First try at implementing background-size
(30.48 KB, patch)
2006-04-12 21:25 PDT
,
Beth Dakin
hyatt
: review-
Details
Formatted Diff
Diff
Patch with feedback
(42.41 KB, patch)
2006-04-13 14:23 PDT
,
Beth Dakin
no flags
Details
Formatted Diff
Diff
Passes layout tests
(42.31 KB, patch)
2006-04-13 16:42 PDT
,
Beth Dakin
no flags
Details
Formatted Diff
Diff
A few adjustments for Darin's comments
(42.19 KB, patch)
2006-04-14 10:26 PDT
,
Beth Dakin
no flags
Details
Formatted Diff
Diff
Includes layout tests
(65.97 KB, patch)
2006-04-14 11:47 PDT
,
Beth Dakin
hyatt
: review-
Details
Formatted Diff
Diff
Okay, better
(37.79 KB, patch)
2006-04-17 15:38 PDT
,
Beth Dakin
hyatt
: review-
Details
Formatted Diff
Diff
Most newest
(37.71 KB, patch)
2006-04-17 20:41 PDT
,
Beth Dakin
no flags
Details
Formatted Diff
Diff
No, this one!
(36.66 KB, patch)
2006-04-18 09:53 PDT
,
Beth Dakin
hyatt
: review+
Details
Formatted Diff
Diff
Show Obsolete
(7)
View All
Add attachment
proposed patch, testcase, etc.
Beth Dakin
Comment 1
2006-04-12 21:25:47 PDT
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 2
2006-04-13 03:04:46 PDT
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
Comment 3
2006-04-13 11:16:20 PDT
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
Comment 4
2006-04-13 14:23:50 PDT
Created
attachment 7682
[details]
Patch with feedback
Beth Dakin
Comment 5
2006-04-13 14:26:27 PDT
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
Comment 6
2006-04-13 14:50:17 PDT
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
Comment 7
2006-04-13 16:42:38 PDT
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 8
2006-04-14 08:35:51 PDT
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
Comment 9
2006-04-14 10:26:37 PDT
Created
attachment 7702
[details]
A few adjustments for Darin's comments Here is a new patch with Darin's comments all addressed.
Beth Dakin
Comment 10
2006-04-14 11:47:11 PDT
Created
attachment 7703
[details]
Includes layout tests
Dave Hyatt
Comment 11
2006-04-14 17:25:12 PDT
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
Comment 12
2006-04-17 15:38:20 PDT
Created
attachment 7784
[details]
Okay, better Here is my newest patch with Dave's latest comments taken into account.
Dave Hyatt
Comment 13
2006-04-17 15:58:02 PDT
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
Comment 14
2006-04-17 20:41:21 PDT
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
Comment 15
2006-04-18 09:53:50 PDT
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 16
2006-04-18 13:55:18 PDT
Comment on
attachment 7803
[details]
No, this one! r=me, file a followup bug about the shorthand.
Beth Dakin
Comment 17
2006-04-18 15:19:14 PDT
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
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug