Bug 27570 – [CSS3 Backgrounds and Borders] Add support for the "space" value for background-repeat

• Status: NEW
• Component: CSS

[Dave Hyatt]

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.

[Lea Verou]

Also the round keyword.
Currently they are not even dropped, bu treated like no-repeat which is really lame.

[Alexandru Chiculita]

Created attachment 99695 [details]
Patch

[Alexandru Chiculita]

The patch should fix the following tests:
http://samples.msdn.microsoft.com/ietestcenter/css3/bordersbackgrounds/background-repeat-space-padding-box.htm
http://samples.msdn.microsoft.com/ietestcenter/css3/bordersbackgrounds/background_repeat_space_border_box.htm
http://samples.msdn.microsoft.com/ietestcenter/css3/bordersbackgrounds/background_repeat_space_content_box.htm

[WebKit Review Bot]

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.

[Alexandru Chiculita]

Created attachment 99696 [details]
Fixed style

[WebKit Review Bot]

Comment on attachment 99696 [details]
Fixed style

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

[Alexandru Chiculita]

Created attachment 99697 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 99697 [details]
Patch

Attachment 99697 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/8982738

[Alexandru Chiculita]

Created attachment 99701 [details]
Patch

[Hajime Morrita]

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.

[Alexandru Chiculita]

(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.

[Alexandru Chiculita]

I've added https://bugs.webkit.org/show_bug.cgi?id=63987 and posted a patch to move the arguments into a class.

[Hajime Morrita]

(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.

[Hajime Morrita]

> 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.

[Alexandru Chiculita]

Created attachment 101433 [details]
Patch

[Alexandru Chiculita]

Created attachment 101435 [details]
Patch

Rebased the patch

[Alexandru Chiculita]

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'

[Alexandru Chiculita]

Created attachment 101572 [details]
Patch

Rebased to latest tree.

[Alexandru Chiculita]

Created attachment 101573 [details]
Patch

Rebased again.

[Hajime Morrita]

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.

[Alexandru Chiculita]

Created attachment 101976 [details]
Patch

[Alexandru Chiculita]

(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.

[Hajime Morrita]

CCing smfr@ as an expert on this area, I missed to CC him...

[Hajime Morrita]

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?

[Simon Fraser (smfr)]

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.

[Alexandru Chiculita]

Created attachment 102009 [details]
Patch

Added new tests. Included fixes for feedback.

[Simon Fraser (smfr)]

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).

[WebKit Review Bot]

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

[Alexandru Chiculita]

(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.

[Hajime Morrita]

> 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.

[Hajime Morrita]

And please mark new tests as failed on non-mac ports.

[Dave Hyatt]

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.

[Radar WebKit Bug Importer]

<rdar://problem/10218262>

[Julien Chaffraix]

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.

[Ahmad Saleem]

Just wanted to update that Safari is still facing lot of test in WPT:

https://wpt.fyi/results/css/css-backgrounds?label=master&label=experimental&aligned&q=background-repeat-space

I think this patch has not been landed or it has not been fixed. Just wanted to share an update. Thanks!