Bug 28635 – [CSS3 Backgrounds and Borders] Add support for 2-keyword values for background-repeat

• Component: CSS
• Status: RESOLVED FIXED

<http://dev.w3.org/csswg/css3-background/#background-repeat> introduces a new way to specify the horizontal repeat behavior separately from the vertical repeat behavior:
    background-repeat: repeat no-repeat;
is equivalent to
    background-repeat: repeat-x;

While repeat-x and repeat-y are still supported as shorthands, combinations involving the new 'space' and 'round' values are also possible using the 2-keyword notation.

[Beth Dakin]

Created attachment 38762 [details]
Patch

[Darin Adler]

Comment on attachment 38762 [details]
Patch

> +            if (xRepeat == yRepeat)
> +                return CSSPrimitiveValue::create(xRepeat);
> +            else if (xRepeat == CSSValueRepeat && yRepeat == CSSValueNoRepeat)
> +                return CSSPrimitiveValue::createIdentifier(CSSValueRepeatX);
> +            else if (xRepeat == CSSValueNoRepeat && yRepeat == CSSValueRepeat)
> +                return CSSPrimitiveValue::createIdentifier(CSSValueRepeatY);

No need for else after return, and normally we omit it.

> +        case CSSPropertyWebkitMaskRepeat: {
> +            EFillRepeat xRepeat = style->maskRepeatX();
> +            EFillRepeat yRepeat = style->maskRepeatY();
> +            
> +            // For backwards compatibility, if both values are equal, just return one of them. And
> +            // if the two values are equivalent to repeat-x or repeat-y, just return the shorthand.
> +            if (xRepeat == yRepeat)
> +                return CSSPrimitiveValue::create(xRepeat);
> +            else if (xRepeat == CSSValueRepeat && yRepeat == CSSValueNoRepeat)
> +                return CSSPrimitiveValue::createIdentifier(CSSValueRepeatX);
> +            else if (xRepeat == CSSValueNoRepeat && yRepeat == CSSValueRepeat)
> +                return CSSPrimitiveValue::createIdentifier(CSSValueRepeatY);
> +
> +            RefPtr<CSSValueList> list = CSSValueList::createSpaceSeparated();
> +            list->append(CSSPrimitiveValue::create(xRepeat));
> +            list->append(CSSPrimitiveValue::create(yRepeat));
> +            return list.release();
> +        }

This should use a function that takes xRepeat and yRepeat arguments so mask-repeat can share almost all its code with background-repeat.

> +        case CSSPropertyBackgroundRepeat: {
> +            const int properties[2] = { CSSPropertyBackgroundRepeatX,
> +                                        CSSPropertyBackgroundRepeatY };
> +            return getLayeredShorthandValue(properties, 2);
> +        }

I think the property names would look better on a single line here.

> +        case CSSPropertyWebkitMaskRepeat: {
> +            const int properties[2] = { CSSPropertyWebkitMaskRepeatX,
> +                                        CSSPropertyWebkitMaskRepeatY };
> +            return getLayeredShorthandValue(properties, 2);
> +        }

Here too.

> +                    RefPtr<CSSValue> yValue;
> +                    if (values[j + 1]->isValueList())
> +                        yValue = static_cast<CSSValueList*>(values[j + 1].get())->itemWithoutBoundsCheck(i);
> +                    else
> +                        yValue = values[j + 1];

With values[j + 1] repeated three times I suspect it would read better if that was in a local variable.

> +                    int xId = static_cast<CSSPrimitiveValue*>(value.get())->getIdent();
> +                    int yId =  static_cast<CSSPrimitiveValue*>(yValue.get())->getIdent();

There’s an extra space here after the "=" sign.

All that code appending string is quite inefficient, but that's nothing new.

>          case NoRepeatFill:
>              m_value.ident = CSSValueNoRepeat;
> +            break;
>          case RoundFill:
>              m_value.ident = CSSValueRound;
> +            break;
>          case SpaceFill:
>              m_value.ident = CSSValueSpace;
>              break;

Are there tests that cover these two missing break statements?

> +    case CSSPropertyBackgroundRepeat:
> +        HANDLE_BACKGROUND_INHERIT_AND_INITIAL(repeatX, RepeatX);
> +        HANDLE_BACKGROUND_INHERIT_AND_INITIAL(repeatY, RepeatY);
> +        return;
> +    case CSSPropertyBackgroundRepeatX: {
> +        HANDLE_BACKGROUND_VALUE(repeatX, RepeatX, value)
> +        return;
> +    }
> +    case CSSPropertyBackgroundRepeatY: {
> +        HANDLE_BACKGROUND_VALUE(repeatY, RepeatY, value)
> +        return;
> +    }
> +    case CSSPropertyWebkitMaskRepeat:
> +        HANDLE_MASK_INHERIT_AND_INITIAL(repeatX, RepeatX);
> +        HANDLE_MASK_INHERIT_AND_INITIAL(repeatY, RepeatY);
> +        return;
> +    case CSSPropertyWebkitMaskRepeatX: {
> +        HANDLE_MASK_VALUE(repeatX, RepeatX, value)
> +        return;
> +    }
> +    case CSSPropertyWebkitMaskRepeatY: {
> +        HANDLE_MASK_VALUE(repeatY, RepeatY, value)
> +        return;
> +    }

Do the HANDLE_BACKGROUND_VALUE and HANDLE_MASK_VALUE ones really need braces? I'm surprised since the others don't have them.

> -background-image: url(about:blank); background-repeat: initial; background-attachment: initial; background-origin: initial; background-clip: initial; background-color: initial; background-position: 80% 50px;
> +background-image: url(about:blank); background-attachment: initial; background-origin: initial; background-clip: initial; background-color: initial; background-position: 80% 50px; background-repeat: initial initial;

The order here seems random. Should we define the order somehow? By changing the code to do things in a canonical order?

> -Removing background removes background-image, background-repeat, background-attachment, background-origin, background-clip, background-color, background-position.
> +Removing background removes background-image, background-attachment, background-origin, background-clip, background-color, background-position, background-repeat.

Ditto.

> -Removing -webkit-mask removes -webkit-mask-image, -webkit-mask-repeat, -webkit-mask-attachment, -webkit-mask-position-x, -webkit-mask-position-y, -webkit-mask-origin, -webkit-mask-clip.
> +Removing -webkit-mask removes -webkit-mask-image, -webkit-mask-repeat-x, -webkit-mask-repeat-y, -webkit-mask-attachment, -webkit-mask-position-x, -webkit-mask-position-y, -webkit-mask-origin, -webkit-mask-clip.

Ditto.

> Index: LayoutTests/platform/mac/fast/inspector/style-expected.txt
> ===================================================================
> --- LayoutTests/platform/mac/fast/inspector/style-expected.txt	(revision 47879)
> +++ LayoutTests/platform/mac/fast/inspector/style-expected.txt	(working copy)
> @@ -9,46 +9,49 @@ layer at (0,0) size 800x600
>        RenderBlock {DIV} at (24,42) size 736x28 [color=#FFFFFF] [bgcolor=#800080]
>          RenderText {#text} at (0,0) size 50x28
>            text run at (0,0) width 50: "Test"
> -      RenderBlock (anonymous) at (0,94) size 784x252
> +      RenderBlock (anonymous) at (0,94) size 784x270
>          RenderText {#text} at (0,0) size 388x18
>            text run at (0,0) width 388: "background-image: initial (original property was background)"
>          RenderBR {BR} at (388,14) size 0x0
> -        RenderText {#text} at (0,18) size 388x18
> -          text run at (0,18) width 388: "background-repeat: initial (original property was background)"
> -        RenderBR {BR} at (388,32) size 0x0
> -        RenderText {#text} at (0,36) size 418x18
> -          text run at (0,36) width 418: "background-attachment: initial (original property was background)"
> -        RenderBR {BR} at (418,50) size 0x0
> -        RenderText {#text} at (0,54) size 413x18
> -          text run at (0,54) width 413: "background-position-x: initial (original property was background)"
> -        RenderBR {BR} at (413,68) size 0x0
> +        RenderText {#text} at (0,18) size 401x18
> +          text run at (0,18) width 401: "background-repeat-x: initial (original property was background)"
> +        RenderBR {BR} at (401,32) size 0x0
> +        RenderText {#text} at (0,36) size 401x18
> +          text run at (0,36) width 401: "background-repeat-y: initial (original property was background)"
> +        RenderBR {BR} at (401,50) size 0x0
> +        RenderText {#text} at (0,54) size 418x18
> +          text run at (0,54) width 418: "background-attachment: initial (original property was background)"
> +        RenderBR {BR} at (418,68) size 0x0
>          RenderText {#text} at (0,72) size 413x18
> -          text run at (0,72) width 413: "background-position-y: initial (original property was background)"
> +          text run at (0,72) width 413: "background-position-x: initial (original property was background)"
>          RenderBR {BR} at (413,86) size 0x0
> -        RenderText {#text} at (0,90) size 387x18
> -          text run at (0,90) width 387: "background-origin: initial (original property was background)"
> -        RenderBR {BR} at (387,104) size 0x0
> -        RenderText {#text} at (0,108) size 373x18
> -          text run at (0,108) width 373: "background-clip: initial (original property was background)"
> -        RenderBR {BR} at (373,122) size 0x0
> -        RenderText {#text} at (0,126) size 387x18
> -          text run at (0,126) width 387: "background-color: purple (original property was background)"
> -        RenderBR {BR} at (387,140) size 0x0
> -        RenderText {#text} at (0,144) size 300x18
> -          text run at (0,144) width 300: "margin-top: 1em (original property was margin)"
> -        RenderBR {BR} at (300,158) size 0x0
> -        RenderText {#text} at (0,162) size 510x18
> -          text run at (0,162) width 510: "margin-right: 1em (original property was margin and property was implicitly set.)"
> -        RenderBR {BR} at (510,176) size 0x0
> -        RenderText {#text} at (0,180) size 525x18
> -          text run at (0,180) width 525: "margin-bottom: 1em (original property was margin and property was implicitly set.)"
> -        RenderBR {BR} at (525,194) size 0x0
> -        RenderText {#text} at (0,198) size 501x18
> -          text run at (0,198) width 501: "margin-left: 1em (original property was margin and property was implicitly set.)"
> -        RenderBR {BR} at (501,212) size 0x0
> -        RenderText {#text} at (0,216) size 75x18
> -          text run at (0,216) width 75: "color: white"
> -        RenderBR {BR} at (75,230) size 0x0
> -        RenderText {#text} at (0,234) size 362x18
> -          text run at (0,234) width 362: "font: normal normal normal 24px/normal 'Lucida Grande'"
> -        RenderBR {BR} at (362,248) size 0x0
> +        RenderText {#text} at (0,90) size 413x18
> +          text run at (0,90) width 413: "background-position-y: initial (original property was background)"
> +        RenderBR {BR} at (413,104) size 0x0
> +        RenderText {#text} at (0,108) size 387x18
> +          text run at (0,108) width 387: "background-origin: initial (original property was background)"
> +        RenderBR {BR} at (387,122) size 0x0
> +        RenderText {#text} at (0,126) size 373x18
> +          text run at (0,126) width 373: "background-clip: initial (original property was background)"
> +        RenderBR {BR} at (373,140) size 0x0
> +        RenderText {#text} at (0,144) size 387x18
> +          text run at (0,144) width 387: "background-color: purple (original property was background)"
> +        RenderBR {BR} at (387,158) size 0x0
> +        RenderText {#text} at (0,162) size 300x18
> +          text run at (0,162) width 300: "margin-top: 1em (original property was margin)"
> +        RenderBR {BR} at (300,176) size 0x0
> +        RenderText {#text} at (0,180) size 510x18
> +          text run at (0,180) width 510: "margin-right: 1em (original property was margin and property was implicitly set.)"
> +        RenderBR {BR} at (510,194) size 0x0
> +        RenderText {#text} at (0,198) size 525x18
> +          text run at (0,198) width 525: "margin-bottom: 1em (original property was margin and property was implicitly set.)"
> +        RenderBR {BR} at (525,212) size 0x0
> +        RenderText {#text} at (0,216) size 501x18
> +          text run at (0,216) width 501: "margin-left: 1em (original property was margin and property was implicitly set.)"
> +        RenderBR {BR} at (501,230) size 0x0
> +        RenderText {#text} at (0,234) size 75x18
> +          text run at (0,234) width 75: "color: white"
> +        RenderBR {BR} at (75,248) size 0x0
> +        RenderText {#text} at (0,252) size 362x18
> +          text run at (0,252) width 362: "font: normal normal normal 24px/normal 'Lucida Grande'"
> +        RenderBR {BR} at (362,266) size 0x0

Can this be a dumpAsText() test? The output seems to indicate it should be.

I'd like to see even more tests of the parser. I'm not sure all the edge cases are covered. Is ever branch of every if statement tested?

r=me as-is

[Beth Dakin]

Thanks for the review Darin!

(In reply to comment #2)
> (From update of attachment 38762 [details])
> > +            if (xRepeat == yRepeat)
> > +                return CSSPrimitiveValue::create(xRepeat);
> > +            else if (xRepeat == CSSValueRepeat && yRepeat == CSSValueNoRepeat)
> > +                return CSSPrimitiveValue::createIdentifier(CSSValueRepeatX);
> > +            else if (xRepeat == CSSValueNoRepeat && yRepeat == CSSValueRepeat)
> > +                return CSSPrimitiveValue::createIdentifier(CSSValueRepeatY);
> 
> No need for else after return, and normally we omit it.
> 

Fixed this.

> > +        case CSSPropertyWebkitMaskRepeat: {
> > +            EFillRepeat xRepeat = style->maskRepeatX();
> > +            EFillRepeat yRepeat = style->maskRepeatY();
> > +            
> > +            // For backwards compatibility, if both values are equal, just return one of them. And
> > +            // if the two values are equivalent to repeat-x or repeat-y, just return the shorthand.
> > +            if (xRepeat == yRepeat)
> > +                return CSSPrimitiveValue::create(xRepeat);
> > +            else if (xRepeat == CSSValueRepeat && yRepeat == CSSValueNoRepeat)
> > +                return CSSPrimitiveValue::createIdentifier(CSSValueRepeatX);
> > +            else if (xRepeat == CSSValueNoRepeat && yRepeat == CSSValueRepeat)
> > +                return CSSPrimitiveValue::createIdentifier(CSSValueRepeatY);
> > +
> > +            RefPtr<CSSValueList> list = CSSValueList::createSpaceSeparated();
> > +            list->append(CSSPrimitiveValue::create(xRepeat));
> > +            list->append(CSSPrimitiveValue::create(yRepeat));
> > +            return list.release();
> > +        }
> 
> This should use a function that takes xRepeat and yRepeat arguments so
> mask-repeat can share almost all its code with background-repeat.
> 

Fixed this too.


> > +        case CSSPropertyBackgroundRepeat: {
> > +            const int properties[2] = { CSSPropertyBackgroundRepeatX,
> > +                                        CSSPropertyBackgroundRepeatY };
> > +            return getLayeredShorthandValue(properties, 2);
> > +        }
> 
> I think the property names would look better on a single line here.

Fixed.

> 
> > +        case CSSPropertyWebkitMaskRepeat: {
> > +            const int properties[2] = { CSSPropertyWebkitMaskRepeatX,
> > +                                        CSSPropertyWebkitMaskRepeatY };
> > +            return getLayeredShorthandValue(properties, 2);
> > +        }
> 
> Here too.
> 

Fixed.

> > +                    RefPtr<CSSValue> yValue;
> > +                    if (values[j + 1]->isValueList())
> > +                        yValue = static_cast<CSSValueList*>(values[j + 1].get())->itemWithoutBoundsCheck(i);
> > +                    else
> > +                        yValue = values[j + 1];
> 
> With values[j + 1] repeated three times I suspect it would read better if that
> was in a local variable.
> 

Fixed.

> > +                    int xId = static_cast<CSSPrimitiveValue*>(value.get())->getIdent();
> > +                    int yId =  static_cast<CSSPrimitiveValue*>(yValue.get())->getIdent();
> 
> There’s an extra space here after the "=" sign.
> 

Fixed.

> All that code appending string is quite inefficient, but that's nothing new.
> 
> >          case NoRepeatFill:
> >              m_value.ident = CSSValueNoRepeat;
> > +            break;
> >          case RoundFill:
> >              m_value.ident = CSSValueRound;
> > +            break;
> >          case SpaceFill:
> >              m_value.ident = CSSValueSpace;
> >              break;
> 
> Are there tests that cover these two missing break statements?
> 

No. As far as I can tell, nothing bad would have happened in the current code since we wouldn't have fallen into any of the other cases. It seemed like a good idea to add the breaks though because we have them everywhere else.

> > +    case CSSPropertyBackgroundRepeat:
> > +        HANDLE_BACKGROUND_INHERIT_AND_INITIAL(repeatX, RepeatX);
> > +        HANDLE_BACKGROUND_INHERIT_AND_INITIAL(repeatY, RepeatY);
> > +        return;
> > +    case CSSPropertyBackgroundRepeatX: {
> > +        HANDLE_BACKGROUND_VALUE(repeatX, RepeatX, value)
> > +        return;
> > +    }
> > +    case CSSPropertyBackgroundRepeatY: {
> > +        HANDLE_BACKGROUND_VALUE(repeatY, RepeatY, value)
> > +        return;
> > +    }
> > +    case CSSPropertyWebkitMaskRepeat:
> > +        HANDLE_MASK_INHERIT_AND_INITIAL(repeatX, RepeatX);
> > +        HANDLE_MASK_INHERIT_AND_INITIAL(repeatY, RepeatY);
> > +        return;
> > +    case CSSPropertyWebkitMaskRepeatX: {
> > +        HANDLE_MASK_VALUE(repeatX, RepeatX, value)
> > +        return;
> > +    }
> > +    case CSSPropertyWebkitMaskRepeatY: {
> > +        HANDLE_MASK_VALUE(repeatY, RepeatY, value)
> > +        return;
> > +    }
> 
> Do the HANDLE_BACKGROUND_VALUE and HANDLE_MASK_VALUE ones really need braces?
> I'm surprised since the others don't have them.
> 

They don't! I got rid of them

> > -background-image: url(about:blank); background-repeat: initial; background-attachment: initial; background-origin: initial; background-clip: initial; background-color: initial; background-position: 80% 50px;
> > +background-image: url(about:blank); background-attachment: initial; background-origin: initial; background-clip: initial; background-color: initial; background-position: 80% 50px; background-repeat: initial initial;
> 
> The order here seems random. Should we define the order somehow? By changing
> the code to do things in a canonical order?
> 

I believe that the current order is determined by CSSMutableStyleDeclaration::cssText(), which determines the order simply by iterating through m_properties and concatenating the cssText(). background-repeat has moved to copy the behavior of background-position though because cssText is not sufficient and more work is required. 

> 
> > Index: LayoutTests/platform/mac/fast/inspector/style-expected.txt
> > ===================================================================
> > --- LayoutTests/platform/mac/fast/inspector/style-expected.txt	(revision 47879)
> > +++ LayoutTests/platform/mac/fast/inspector/style-expected.txt	(working copy)
> > @@ -9,46 +9,49 @@ layer at (0,0) size 800x600
> >        RenderBlock {DIV} at (24,42) size 736x28 [color=#FFFFFF] [bgcolor=#800080]
> >          RenderText {#text} at (0,0) size 50x28
> >            text run at (0,0) width 50: "Test"
> > -      RenderBlock (anonymous) at (0,94) size 784x252
etc...
> 
> Can this be a dumpAsText() test? The output seems to indicate it should be.
> 

Looks like it! I made it dumpAsText()

> I'd like to see even more tests of the parser. I'm not sure all the edge cases
> are covered. Is ever branch of every if statement tested?
> 

I agree that more thorough testing would be better. I will beef up the parser test a little.

> r=me as-is

Thanks!

[Beth Dakin]

Fixed with revision 47906.