[CSS Background] Position property should be ignored when using repeat: space

[Andrei Parvu]

As the spec [1] says, background/mask-position property should be ignored when using repeat: space, if there are at least two copies of the image on a direction. If there is not enough space for two copies, only one should be drawn, and background/mask-position should determine its position.

[1] - http://www.w3.org/TR/css3-background/#the-background-repeat

[Andrei Parvu]

Created attachment 210338 [details]
Patch

[Dirk Schulze]

Comment on attachment 210338 [details]
Patch

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

> Source/WebCore/ChangeLog:7
> +        The background/mask-position should be ignored when using repeat: space, if there are
> +        at least two copies of the image in one direction.

The last part of the sentence is confusing. I needed to read the spec again to understand what you mean. Maybe just quite the spec :)

> Source/WebCore/rendering/RenderBoxModelObject.cpp:1358
> +            backgroundRepeatX = NoRepeatFill;

Doesn't that affect computed style? The spec is just speaking about visual differences.

[Andrei Parvu]

> 
> > Source/WebCore/rendering/RenderBoxModelObject.cpp:1358
> > +            backgroundRepeatX = NoRepeatFill;
> 
> Doesn't that affect computed style? The spec is just speaking about visual differences.

backgroundRepeatX is a local variable, it doesn't affect he computed style.

[Andrei Parvu]

Created attachment 210370 [details]
Patch

[Andrei Parvu]

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

Modified the description in the change log.

[Dirk Schulze]

Comment on attachment 210370 [details]
Patch

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

> Source/WebCore/rendering/RenderBoxModelObject.cpp:1353
> +        if (space) {

You are not checking for positive as suggested in the ChangeLog, but for zero. Negtative values would still enter the code path.

> Source/WebCore/rendering/RenderBoxModelObject.cpp:1358
> +            geometry.setSpaceSize(FloatSize(space, 0));
> +            geometry.setPhaseX(actualWidth ? actualWidth - roundToInt(computedXPosition + left) % actualWidth : 0);
> +        } else
> +            backgroundRepeatX = NoRepeatFill;

To be honest, I do not understand the code path good enough. I would have assumed that you check how many tiles fit into the paint area. If it is less than two... disable spacing and use background-position instead. Maybe the code path does it, but I don't see it. Can you elaborate more please?

[Andrei Parvu]

Created attachment 210432 [details]
Patch

[Andrei Parvu]

(In reply to comment #6)
> (From update of attachment 210370 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=210370&action=review
> 
> > Source/WebCore/rendering/RenderBoxModelObject.cpp:1353
> > +        if (space) {
> 
> You are not checking for positive as suggested in the ChangeLog, but for zero. Negtative values would still enter the code path.
> 
> > Source/WebCore/rendering/RenderBoxModelObject.cpp:1358
> > +            geometry.setSpaceSize(FloatSize(space, 0));
> > +            geometry.setPhaseX(actualWidth ? actualWidth - roundToInt(computedXPosition + left) % actualWidth : 0);
> > +        } else
> > +            backgroundRepeatX = NoRepeatFill;
> 
> To be honest, I do not understand the code path good enough. I would have assumed that you check how many tiles fit into the paint area. If it is less than two... disable spacing and use background-position instead. Maybe the code path does it, but I don't see it. Can you elaborate more please?

getSpace() now returns -1 if the number of tiles is less than two; in this case the repeat of the image is set as NoRepeatFill. If space is greater or equal to zero, then the value of background/mask-position is ignored when computing 'computedX/YPosition'.

[Dirk Schulze]

Comment on attachment 210432 [details]
Patch

r=me

[Andrei Parvu]

(In reply to comment #9)
> (From update of attachment 210432 [details])
> r=me

Thanks for the review. Could you also commit the patch?

[WebKit Commit Bot]

Comment on attachment 210432 [details]
Patch

Clearing flags on attachment: 210432

Committed r156097: <http://trac.webkit.org/changeset/156097>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.