Bug 119324 – [CSS Masking] -webkit-mask-repeat: space does not work

• Component: CSS
• Status: RESOLVED FIXED

[Andrei Parvu]

Created attachment 207811 [details]
Example of -webkit-mask-repeat: space usage which doesn't work.

When using -webkit-mask-repeat with a value of 'space' the image isn't spaced so that it fist a whole number of times in the background, but isn't repeated at all.

Looking through the code, I noticed that everything different from -webkit-mask-repeat: repeat is treated like -webkit-mask-repeat: no-repeat, so the space repeat style is not implemented.

[Andrei Parvu]

Created attachment 208098 [details]
Patch

[Build Bot]

Comment on attachment 208098 [details]
Patch

Attachment 208098 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1334921

New failing tests:
fast/gradients/gradient-on-pseudoelement-crash.html

[Build Bot]

Created attachment 208102 [details]
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-08  Port: mac-mountainlion  Platform: Mac OS X 10.8.4

[Andrei Parvu]

I don't think the failing test has anything to do with my patch, it also fails when running it on a branch which doesn't have the repeat space changes.

[Dirk Schulze]

(In reply to comment #4)
> I don't think the failing test has anything to do with my patch, it also fails when running it on a branch which doesn't have the repeat space changes.

The background code can be fragile. Especially gradients code is likely to be caused by changes to background. Can you elaborate and make sure that it is not your fault?

[Dirk Schulze]

Comment on attachment 208098 [details]
Patch

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

r- because of some change request. I didn't check the change in ModelObject yet.

> Source/WebCore/ChangeLog:6
> +        Reviewed by NOBODY (OOPS!).

You need to explain what you do, why you do it and why this is the right thing to archive the right behavior.

> Source/WebCore/ChangeLog:24
> +        (WebCore::RenderBoxModelObject::paintFillLayerExtended):
> +        (WebCore::RenderBoxModelObject::calculateBackgroundImageGeometry):

Smaller inline comments can help to understand what you do.

> Source/WebCore/platform/graphics/cg/ImageCG.cpp:136
> +        // On Leopard and newer, this code now only runs for partially decoded images whose buffers do not yet match the overall size of the image.
> +        static const CGPatternCallbacks patternCallbacks = { 0, drawPatternCallback, 0 };
> +        CGAffineTransform matrix = CGAffineTransformMake(narrowPrecisionToCGFloat(patternTransform.a()), 0, 0, narrowPrecisionToCGFloat(patternTransform.d()), adjustedX, adjustedY);
> +        matrix = CGAffineTransformConcat(matrix, CGContextGetCTM(context));
> +        // The top of a partially-decoded image is drawn at the bottom of the tile. Map it to the top.
> +        matrix = CGAffineTransformTranslate(matrix, 0, size().height() - h);
> +        RetainPtr<CGPatternRef> pattern = adoptCF(CGPatternCreate(subImage.get(), CGRectMake(0, 0, tileRect.width(), tileRect.height()),
> +            matrix, tileRect.width() + spaceSize().width(), tileRect.height() + spaceSize().height(),
> +            kCGPatternTilingConstantSpacing, true, &patternCallbacks));
> +        if (!pattern)
> +            return;
> +

It would be great if you can use the Pattern class already in WebKit to do that, so that it works in all ports, not just Mac. You may need to add some more code to Pattern to make that possible. Can you check this first please? Resetting review flag for now.

> LayoutTests/css3/background/background-repeat-border-expected.html:16
> +            var urls = Array(), size = Array(), position = Array(), repeat = Array(),
> +                origin = Array(), clip = Array();

Does that all need to be JS? Can't you just write a simple test case to make sure that it works?

> LayoutTests/css3/background/background-repeat-border-expected.html:18
> +            function addMasks() {

If you are testing background, then mask might be misleading.

[Andrei Parvu]

> > Source/WebCore/platform/graphics/cg/ImageCG.cpp:136
> > +        // On Leopard and newer, this code now only runs for partially decoded images whose buffers do not yet match the overall size of the image.
> > +        static const CGPatternCallbacks patternCallbacks = { 0, drawPatternCallback, 0 };
> > +        CGAffineTransform matrix = CGAffineTransformMake(narrowPrecisionToCGFloat(patternTransform.a()), 0, 0, narrowPrecisionToCGFloat(patternTransform.d()), adjustedX, adjustedY);
> > +        matrix = CGAffineTransformConcat(matrix, CGContextGetCTM(context));
> > +        // The top of a partially-decoded image is drawn at the bottom of the tile. Map it to the top.
> > +        matrix = CGAffineTransformTranslate(matrix, 0, size().height() - h);
> > +        RetainPtr<CGPatternRef> pattern = adoptCF(CGPatternCreate(subImage.get(), CGRectMake(0, 0, tileRect.width(), tileRect.height()),
> > +            matrix, tileRect.width() + spaceSize().width(), tileRect.height() + spaceSize().height(),
> > +            kCGPatternTilingConstantSpacing, true, &patternCallbacks));
> > +        if (!pattern)
> > +            return;
> > +
> 
> It would be great if you can use the Pattern class already in WebKit to do that, so that it works in all ports, not just Mac. You may need to add some more code to Pattern to make that possible. Can you check this first please? Resetting review flag for now.

That code was already there (except the adding of 'spaceSize'). I will try using the Pattern class but it would require some changes (you have to pass a CGImageRef to it which you currently cannot).

> > LayoutTests/css3/background/background-repeat-border-expected.html:16
> > +            var urls = Array(), size = Array(), position = Array(), repeat = Array(),
> > +                origin = Array(), clip = Array();
> 
> Does that all need to be JS? Can't you just write a simple test case to make sure that it works?

I wanted some tests with masks which are being repeated a bigger number of times - adding 12 masks manually seemed ugly :). Though I can get rid of the repeat, origin and clip arrays, apparently I only need to specifiy them once.

[Andrei Parvu]

Created attachment 208122 [details]
Patch

[Andrei Parvu]

Tried to change ImageCG::drawPattern to use the Pattern class, although I don't know how useful it will be - didn't figure out a better way to do it.

[Andrei Parvu]

Created attachment 208396 [details]
Patch

[Andrei Parvu]

(In reply to comment #10)
> Created an attachment (id=208396) [details]
> Patch

Added some more comments to the change logs.

[Dirk Schulze]

Comment on attachment 208396 [details]
Patch

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

Great patch! I think we run into a misunderstanding on the use of Pattern. It would be great if this could be done outside of the platform dependent code, so that it works on all platforms.

> Source/WebCore/platform/graphics/Pattern.h:63
> +    static PassRefPtr<Pattern> create();

Hm, you create a Pattern without initial data? Don't you alraedy have access to the Image at that point?

> Source/WebCore/platform/graphics/cg/ImageCG.cpp:126
> +        matrix = CGAffineTransformTranslate(matrix, 0, size().height() - h);
> +        RefPtr<Pattern> patternPtr = Pattern::create();
> +        RetainPtr<CGPatternRef> pattern = adoptCF(patternPtr->createPlatformPattern(matrix, subImage.get(), tileRect,

I thought more of using Pattern in the platform independent path. Here is does not make a lot of sense to wrap the Pattern class around the PlatformPattern. This can not be used by other ports either. I thought more in rendering code, where we initially call GraphicsContext background drawing routines.

> Source/WebCore/platform/graphics/cg/ImageCG.cpp:143
> +        CGRect r = CGContextGetClipBoundingBox(context);
> +        CGContextFillRect(context, r);

Did you initialize the r for testing? You don't need to assign a new variable here.

> Source/WebCore/platform/graphics/cg/PatternCG.cpp:90
> +CGPatternRef Pattern::createPlatformPattern(const AffineTransform& matrix, PlatformImagePtr patternImage, FloatRect tileRect, float xStep, float yStep) const

You may want to pass a FloatSize instead of xStep and yStep.

> Source/WebCore/rendering/RenderBoxModelObject.cpp:1323
> +        geometry.setSpaceSize(FloatSize(0, 0));

just use FloatSize()

> Source/WebCore/rendering/RenderBoxModelObject.cpp:1367
> +    } else if (backgroundRepeatY == SpaceFill) {
> +        if (fillTileSize.height() > 0) {

can you combine that a one condition in the else if ?

[Dirk Schulze]

Comment on attachment 208396 [details]
Patch

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

We had longer discussions how the Pattern class can be refactored to represent the spacing. We realize that there are some tradeoffs to make like storing more member variables in the frequently used Pattern class. On the other hand there are no benefits for the other ports that still need to modify the image buffers. CG provides spacing in the internal pattern logic, the other ports don't.

Because of that, I think it doesn't make sense to modify Pattern after all and just do the necessary changes in ImageCG.

Some nips:

> Source/WebCore/ChangeLog:11
> +        [CSS Masking] -webkit-mask-repeat: space does not work
> +        Added the space option to repeat property using the createPlatformPattern function.
> +        The space mask-repeat repeats the mask as often as will fit within the background positioning
> +        area without being clipped and then spaces the masks so that they fill the area.
> +        Changed the Pattern class so that createPlatformPattern can be called from the ImageCG use-case.
> +        https://bugs.webkit.org/show_bug.cgi?id=119324
> +
> +        Reviewed by NOBODY (OOPS!).
> +

The format of the changelog is still a bit off.

s/repeat property/background-repeat and -webkit-mask-repeat properties/

The space mask-repeat...

With the property value 'space', the background or mask image gets repeated as often as it fits within the background positioning area. The repeated images are spaced equally to fill the unused area.

> Source/WebCore/ChangeLog:19
> +        * WebCore.order: added the symbol for the new Pattern::create method

Sentences please.

> Source/WebCore/ChangeLog:22
> +        * platform/graphics/Image.h: add space property to Image class

Ditto.

> Source/WebCore/ChangeLog:25
> +        * platform/graphics/Pattern.cpp: added a new createPlatformPattern method to be called from ImageCG

Ditto.

> Source/WebCore/ChangeLog:29
> +        * platform/graphics/cg/ImageCG.cpp: added the space values to createPlatformPattern

Ditto.

> Source/WebCore/ChangeLog:36
> +        (WebCore::RenderBoxModelObject::paintFillLayerExtended): computed the space values on x and y axis
> +        (WebCore::RenderBoxModelObject::calculateBackgroundImageGeometry): passed the space values to the Image class
> +        * rendering/RenderBoxModelObject.h: add space property to BackgroundImageGeometry class

Ditto.

s/computed/Compute/
s/passed/Pass/

> Source/WebCore/rendering/RenderBoxModelObject.cpp:1344
> +            int nrTiles = positioningAreaSize.width() / fillTileSize.width();

Please rename nrTiles. Not sure, maybe to numberOfTiles.

> Source/WebCore/rendering/RenderBoxModelObject.cpp:1368
> +            int nrTiles = positioningAreaSize.height() / fillTileSize.height();

Ditto.

> Source/WebCore/rendering/RenderBoxModelObject.cpp:1377
> +            int actualHeight = geometry.tileSize().height();
> +            if (nrTiles > 1) {
> +                int space = roundToInt((float)(positioningAreaSize.height() - nrTiles * fillTileSize.height()) / (nrTiles - 1));
> +                geometry.setSpaceSize(FloatSize(geometry.spaceSize().width(), space));
> +
> +                actualHeight += space;
> +            } else
> +                geometry.setSpaceSize(FloatSize(geometry.spaceSize().width(), 0));

That may should go into an inline function and reused by both: width and height.

[Andrei Parvu]

Created attachment 209964 [details]
Patch

[Andrei Parvu]

(In reply to comment #14)
> Created an attachment (id=209964) [details]
> Patch

Also passed the space values to ImageBuffer and SVGImage classes so that it can work with gradients and svg images - also changed two tests to use these two as masks.

[Dirk Schulze]

Comment on attachment 209964 [details]
Patch

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

r=me

> Source/WebCore/platform/graphics/ImageBuffer.h:135
> +        {
> +            m_space = space;
> +        }

one liners are usually done without wrapping.

[WebKit Commit Bot]

Comment on attachment 209964 [details]
Patch

Clearing flags on attachment: 209964

Committed r154875: <http://trac.webkit.org/changeset/154875>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.