Bug 27570 - [CSS3 Backgrounds and Borders] Add support for the "space" value for background-repeat
Summary: [CSS3 Backgrounds and Borders] Add support for the "space" value for backgrou...
Status: NEW
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: InRadar
Depends on: 63987
Blocks: 27569
  Show dependency treegraph
 
Reported: 2009-07-22 15:39 PDT by Dave Hyatt
Modified: 2012-10-08 21:38 PDT (History)
14 users (show)

See Also:


Attachments
Patch (73.11 KB, patch)
2011-07-05 04:03 PDT, Alexandru Chiculita
no flags Details | Formatted Diff | Diff
Fixed style (73.10 KB, patch)
2011-07-05 04:09 PDT, Alexandru Chiculita
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Patch (73.70 KB, patch)
2011-07-05 04:24 PDT, Alexandru Chiculita
gyuyoung.kim: commit-queue-
Details | Formatted Diff | Diff
Patch (75.47 KB, patch)
2011-07-05 04:43 PDT, Alexandru Chiculita
no flags Details | Formatted Diff | Diff
Patch (49.94 KB, patch)
2011-07-20 00:44 PDT, Alexandru Chiculita
no flags Details | Formatted Diff | Diff
Patch (49.79 KB, patch)
2011-07-20 01:11 PDT, Alexandru Chiculita
no flags Details | Formatted Diff | Diff
Patch (49.81 KB, patch)
2011-07-21 04:25 PDT, Alexandru Chiculita
no flags Details | Formatted Diff | Diff
Patch (49.84 KB, patch)
2011-07-21 04:33 PDT, Alexandru Chiculita
morrita: review-
morrita: commit-queue-
Details | Formatted Diff | Diff
Patch (50.72 KB, patch)
2011-07-26 00:07 PDT, Alexandru Chiculita
morrita: review-
morrita: commit-queue-
Details | Formatted Diff | Diff
Patch (244.05 KB, patch)
2011-07-26 08:26 PDT, Alexandru Chiculita
webkit.review.bot: commit-queue-
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:39:47 PDT
background-repeat now supports a "space" value that is used to justify tiles, i.e., insert spacing between the tiles such that the edges cleanly align.
Comment 1 Lea Verou 2011-04-14 19:52:38 PDT
Also the round keyword.
Currently they are not even dropped, bu treated like no-repeat which is really lame.
Comment 2 Alexandru Chiculita 2011-07-05 04:03:50 PDT
Created attachment 99695 [details]
Patch
Comment 4 WebKit Review Bot 2011-07-05 04:06:41 PDT
Attachment 99695 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Source/WebCore/rendering/RenderBoxModelObject.cpp:850:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Alexandru Chiculita 2011-07-05 04:09:44 PDT
Created attachment 99696 [details]
Fixed style
Comment 6 WebKit Review Bot 2011-07-05 04:23:41 PDT
Comment on attachment 99696 [details]
Fixed style

Attachment 99696 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8985356
Comment 7 Alexandru Chiculita 2011-07-05 04:24:23 PDT
Created attachment 99697 [details]
Patch
Comment 8 Gyuyoung Kim 2011-07-05 04:40:58 PDT
Comment on attachment 99697 [details]
Patch

Attachment 99697 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/8982738
Comment 9 Alexandru Chiculita 2011-07-05 04:43:59 PDT
Created attachment 99701 [details]
Patch
Comment 10 Hajime Morrita 2011-07-06 01:27:07 PDT
Comment on attachment 99701 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=99701&action=review

Hi Alexandru,
Thank you doing this. I try to give some feedback.

> Source/WebCore/rendering/RenderBoxModelObject.cpp:768
>          LayoutSize tileSize;

How about start with bringing these parameters together into a class, namely BackgroundImageGeometry, in a separate patch?
It would make this change clearer and smaller.

> Source/WebCore/rendering/RenderBoxModelObject.h:137
> +    bool calculateBackgroundImageGeometry(const FillLayer*, const IntRect& paintRect, IntRect& destRect, IntPoint& phase, IntSize& tileSize, IntSize& padding);

How about to define overloaded version which doesn't have |padding| parameter.
It looks many caller site just passes dummy value, which makes code complicated and
makes this change larger than necessary.
Comment 11 Alexandru Chiculita 2011-07-06 01:51:15 PDT
(In reply to comment #10)

> How about start with bringing these parameters together into a class, namely BackgroundImageGeometry, in a separate patch?

I will post a patch with that.

> How about to define overloaded version which doesn't have |padding| parameter.

If I do the first comment, this wouldn't be needed anymore.

Do you know if Skia can render spaces between pattern tiles? I only implemented it for CoreGraphics. The workaround would be to render the pattern in WebKit.
Comment 12 Alexandru Chiculita 2011-07-06 05:51:40 PDT
I've added https://bugs.webkit.org/show_bug.cgi?id=63987 and posted a patch to move the arguments into a class.
Comment 13 Hajime Morrita 2011-07-06 19:13:55 PDT
(In reply to comment #11)
> (In reply to comment #10)
> 
> > How about start with bringing these parameters together into a class, namely BackgroundImageGeometry, in a separate patch?
> 
> I will post a patch with that.
Thanks! I did have some feedback on Bug 63987.
Comment 14 Hajime Morrita 2011-07-06 20:53:06 PDT
> Do you know if Skia can render spaces between pattern tiles? I only implemented it for CoreGraphics. The workaround would be to render the pattern in WebKit.
I had a quick glance on skia and couldn't find tiling with margins.
If other non-CG backends also don't have the functionality, having it on WebCore would be
helpful for these backends.
Comment 15 Alexandru Chiculita 2011-07-20 00:44:49 PDT
Created attachment 101433 [details]
Patch
Comment 16 Alexandru Chiculita 2011-07-20 01:11:48 PDT
Created attachment 101435 [details]
Patch

Rebased the patch
Comment 17 Alexandru Chiculita 2011-07-20 01:28:14 PDT
The builds also failed on trunk. The fail is unrelated to this patch.

http://build.webkit.org/builders/Leopard%20Intel%20Release%20%28Build%29/builds/37086/steps/compile-webkit/logs/stdio

JavaScriptCore/runtime/ObjectPrototype.cpp: In function 'JSC::EncodedJSValue JSC::objectProtoFuncToString(JSC::ExecState*)':
JavaScriptCore/runtime/ObjectPrototype.cpp:198: error: invalid conversion from 'JSC::JSString*' to 'JSC::EncodedJSValue'
Comment 18 Alexandru Chiculita 2011-07-21 04:25:57 PDT
Created attachment 101572 [details]
Patch

Rebased to latest tree.
Comment 19 Alexandru Chiculita 2011-07-21 04:33:57 PDT
Created attachment 101573 [details]
Patch

Rebased again.
Comment 20 Hajime Morrita 2011-07-21 05:32:11 PDT
Comment on attachment 101573 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=101573&action=review

> Source/WebCore/platform/graphics/GraphicsContext.h:315
> +                                       const IntSize& tilePadding, CompositeOperator, bool useLowQualityScale);

"useLowQualityScale" is unfortunate. It would be great if you pass  InterpolationQuality directly.
That kind of change should have separate bug though.

> Source/WebCore/platform/graphics/Image.cpp:108
> +{

Is it possible to extend drawTiled() to support spacing instead of defining another yet similar method?
How about to add a portable path like this on drawTitled(), for call with non-zero padding?

> Source/WebCore/platform/graphics/Image.cpp:123
> +                    scaledTileSize.height() / intrinsicTileSize.height());

Is this division safe?

> Source/WebCore/rendering/RenderBoxModelObject.cpp:875
> +bool RenderBoxModelObject::calculateFillTilePadding(EFillRepeat repeatX, EFillRepeat repeatY, const IntSize& containingBox, BackgroundImageGeometry& geometry)

It looks this method is part of BackgroundImageGeometry?

> Source/WebCore/rendering/RenderBoxModelObject.cpp:985
> +    switch (backgroundRepeatX) {

Can this switch statement be a method of BackgroundImageGeometry?

> Source/WebCore/rendering/RenderBoxModelObject.cpp:998
> +    switch (backgroundRepeatY) {

Ditto.
Comment 21 Alexandru Chiculita 2011-07-26 00:07:41 PDT
Created attachment 101976 [details]
Patch
Comment 22 Alexandru Chiculita 2011-07-26 00:11:24 PDT
(In reply to comment #20)
> "useLowQualityScale" is unfortunate. It would be great if you pass  InterpolationQuality directly.
> That kind of change should have separate bug though.
I will add a bug for that.

> Is it possible to extend drawTiled() to support spacing instead of defining another yet similar method?
> How about to add a portable path like this on drawTitled(), for call with non-zero padding?
Did that in the last patch.

> Is this division safe?
The latest patch doesn't touch that area anymore. It should be safe because the image should have at least 1px by 1px size.

> It looks this method is part of BackgroundImageGeometry?
Yes, it also makes it nicer. Moved it to BackgroundImageGeometry.

> Can this switch statement be a method of BackgroundImageGeometry?
Yes, moved that too.

Also removed unused methods from BackgroundImageGeometry, because I can access the fields directly now.
Comment 23 Hajime Morrita 2011-07-26 01:22:51 PDT
CCing smfr@ as an expert on this area, I missed to CC him...
Comment 24 Hajime Morrita 2011-07-26 02:10:42 PDT
Comment on attachment 101976 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=101976&action=review

The code looks almost fine to me.
Could you add some more tests to cover edge cases like: 
- for RoundFill, the case for 
- Different repeat-x repeat-y combinations
- "background-position", which should be ignored (according to the spec.)
- A background area which is smaller than  the background image
- A background area which is as small as that the image cannot repeat twice.
- background-origin: border-box
Some of them looks OK to get them together.

> LayoutTests/fast/css/background-repeat-round.html:10
> +    }

Could you make it clear that the image scaling happens?

> Source/WebCore/platform/graphics/Image.cpp:151
> +

ceilf()?

> Source/WebCore/platform/graphics/Image.cpp:158
> +

Could you use FloatRect constructor to set quad values instead of setting it individually?

> Source/WebCore/platform/graphics/Image.cpp:174
> +                    visibleSrcRect.setHeight(clippedRect.height() / scale.height());

Use FloatRect constructor?

> Source/WebCore/rendering/RenderBoxModelObject.cpp:1005
> +        return;

Is it OK to return here?
Comment 25 Simon Fraser (smfr) 2011-07-26 07:48:59 PDT
Comment on attachment 101976 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=101976&action=review

>> LayoutTests/fast/css/background-repeat-round.html:10
>> +    }
> 
> Could you make it clear that the image scaling happens?

Please put spaces after the colons in this and the other tests.
Comment 26 Alexandru Chiculita 2011-07-26 08:26:50 PDT
Created attachment 102009 [details]
Patch

Added new tests. Included fixes for feedback.
Comment 27 Simon Fraser (smfr) 2011-07-26 09:20:29 PDT
Comment on attachment 102009 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=102009&action=review

> Source/WebCore/platform/graphics/Image.cpp:183
> +        for (int x = 0; x <= repeatX; ++x) {
> +            for (int y = 0; y <= repeatY; ++y) {
> +                FloatRect tileRect;
> +                tileRect.setX(destRect.x() + fmodf(fmodf(-srcPoint.x(), tileRealSize.width()) - tileRealSize.width(), tileRealSize.width()));
> +                tileRect.setY(destRect.y() + fmodf(fmodf(-srcPoint.y(), tileRealSize.height()) - tileRealSize.height(), tileRealSize.height()));
> +                tileRect.setSize(scaledTileSize);
> +
> +                tileRect.move(tileRealSize.width() * x, tileRealSize.height() * y);
> +
> +                FloatRect clippedRect = tileRect;
> +                clippedRect.intersect(destRect);
> +
> +                if (clippedRect.isEmpty())
> +                    continue;
> +
> +                if (usesSolidColor)
> +                    fillWithSolidColor(ctxt, clippedRect, solidColor(), styleColorSpace, op);
> +                else {
> +                    FloatRect visibleSrcRect((clippedRect.x() - tileRect.x()) / scale.width(),
> +                                             (clippedRect.y() - tileRect.y()) / scale.height(),
> +                                             clippedRect.width() / scale.width(),
> +                                             clippedRect.height() / scale.height());
> +                    draw(ctxt, clippedRect, visibleSrcRect, styleColorSpace, op);
> +                }
> +            }
> +        }

I'm concerned about the performance implications of this simple loop. Seems like you could fall into this slow tiling when you have a small background image with tiling over a large div (say a 1x1 px image with a 1px space over a 1000x1000 div).
Comment 28 WebKit Review Bot 2011-07-26 09:21:41 PDT
Comment on attachment 102009 [details]
Patch

Attachment 102009 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9253346

New failing tests:
fast/css/background-repeat-round-space.html
fast/css/background-repeat-round-size.html
fast/canvas/webgl/get-active-test.html
fast/css/background-repeat-round-no-repeat.html
fast/css/background-repeat-round-repeat.html
fast/css/background-repeat-round.html
fast/css/background-repeat-space.html
Comment 29 Alexandru Chiculita 2011-07-26 10:48:51 PDT
(In reply to comment #27)
> (From update of attachment 102009 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=102009&action=review

> I'm concerned about the performance implications of this simple loop. Seems like you could fall into this slow tiling when you have a small background image with tiling over a large div (say a 1x1 px image with a 1px space over a 1000x1000 div).

That slow path is there just to help graphics libraries that do not have native support to render patterns with padding (eg. Skia, Cairo). CoreGraphics can draw patterns with different spacing, so I will add the fast path in a different patch/bug. 

Another solution might be to compute an image in memory with the size of the tile+padding and draw that using the native library.
Comment 30 Hajime Morrita 2011-08-03 00:43:57 PDT
> That slow path is there just to help graphics libraries that do not have native support to render patterns with padding (eg. Skia, Cairo). CoreGraphics can draw patterns with different spacing, so I will add the fast path in a different patch/bug. 
> 
So could you 
- file a bug for make CoreGraphics the fast path
- add FIXME comment there with a link to the filed bug?
I will make it easy to track the intention.
Comment 31 Hajime Morrita 2011-08-03 00:45:23 PDT
And please mark new tests as failed on non-mac ports.
Comment 32 Dave Hyatt 2011-09-12 17:07:55 PDT
Note that border-image now has a space value that needs to be implemented as well. I don't know if this bug can cover that work also or if we should just file a new one.
Comment 33 Radar WebKit Bug Importer 2011-09-30 16:18:18 PDT
<rdar://problem/10218262>
Comment 34 Julien Chaffraix 2012-04-19 15:25:07 PDT
Comment on attachment 102009 [details]
Patch

Based on Simon's comment and the EWS failing, I think this will need another pass. I haven't looked closely at the code though.