Repeated background images have the wrong position when using bottom/right-relative background-position

• Component: CSS
• Status: RESOLVED FIXED

[Simon Fraser (smfr)]

Created attachment 271816 [details]
Testcase

We get the image position wrong when repeating, with background-position: bottom 10px right 20px;

[Simon Fraser (smfr)]

Created attachment 271850 [details]
Patch

[Simon Fraser (smfr)]

Created attachment 271851 [details]
Patch

[Simon Fraser (smfr)]

Created attachment 271853 [details]
Patch

[Simon Fraser (smfr)]

Created attachment 271863 [details]
Patch

[Darin Adler]

Comment on attachment 271863 [details]
Patch

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

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:706
> +static Ref<CSSValueList> createPositionListForLayer(const FillLayer* layer, const RenderStyle& style)

This should take a reference rather than a pointer since it won’t work if the layer is null.

> Source/WebCore/css/CSSPrimitiveValueMappings.h:905
> +template<> inline CSSPrimitiveValue::CSSPrimitiveValue(Edge e)

Would be nicer to name this “edge” rather than “e”.

> Source/WebCore/css/CSSToStyleMap.cpp:255
> +    layer.setXPosition(EdgeRelativeLength(length, edge, pair));

Need to name the type? How about this instead:

    layer.setXPosition({ length, edge, pair });

> Source/WebCore/css/CSSToStyleMap.cpp:287
> +    layer.setYPosition(EdgeRelativeLength(length, edge, pair));

Ditto.

> Source/WebCore/rendering/style/EdgeRelativeLength.h:61
> +    Length m_length;
> +    unsigned m_edge : 2;
> +    bool m_edgeSpecified : 1;

I suppose this is a lot more efficient than the more straightforward version? This:

    struct EdgeRelativeLength {
        Length length;
        Optional<Edge> edge;
    };

It sure would be clearer to just use a struct instead if we can.

[Simon Fraser (smfr)]

I'm considering an alternative solution where "bottom 10px" is represented as a calc expression like "calc(100% - 10px)", which is what other browsers do. This lets it be interpolated (if we allow calc to interpolate), and avoids the need to store a separate edge flag.

[Simon Fraser (smfr)]

Created attachment 271943 [details]
Patch

[Dave Hyatt]

Comment on attachment 271943 [details]
Patch

r=me

[Simon Fraser (smfr)]

https://webkit.org/b/154478

[Said Abou-Hallawa]

Comment on attachment 271943 [details]
Patch

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

> Source/WebCore/rendering/RenderBoxModelObject.cpp:1047
> +static LayoutUnit resolveEdgeRelativeLength(const Length& length, Edge edge, LayoutUnit availableSpace, const LayoutSize& areaSize, const LayoutSize& tileSize)

I think "resolve" is misleading here.  My understanding about this function is it transforms a length from the other side of the coordinates system to the origin of the coordinates system. How about normalizeEdgeRelativeLength() or edgeRelativeLengthToAbsolutePosition()?

> Source/WebCore/rendering/RenderBoxModelObject.cpp:1055
> +        return areaSize.height() - tileSize.height() - result;

Why do we involve the tileSize in these calculations? Isn't edge at the border of the areaSize?

> Source/WebCore/rendering/style/FillLayer.h:206
> +    unsigned m_backgroundYOrigin : 2; // Edge

Can't we define these variables like this?

Edge m_backgroundXOrigin : 2;
Edge m_backgroundYOrigin : 2;

So we can avoid casting everywhere. I think clang is fine with these definitions but I am not sure about the other ports.

> Source/WebCore/rendering/style/RenderStyleConstants.h:234
> +enum class Edge { Top, Right, Bottom, Left };

Why do we have different enums for the same thing?
BackgroundEdgeOrigin (now Edge)
BoxSide
PhysicalBoxSide

[Simon Fraser (smfr)]

(In reply to comment #10)
> Comment on attachment 271943 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=271943&action=review
> 
> > Source/WebCore/rendering/RenderBoxModelObject.cpp:1047
> > +static LayoutUnit resolveEdgeRelativeLength(const Length& length, Edge edge, LayoutUnit availableSpace, const LayoutSize& areaSize, const LayoutSize& tileSize)
> 
> I think "resolve" is misleading here.  My understanding about this function
> is it transforms a length from the other side of the coordinates system to
> the origin of the coordinates system. How about
> normalizeEdgeRelativeLength() or edgeRelativeLengthToAbsolutePosition()?

I'm OK with "normalize" but I don't think it's better enough to change.

> > Source/WebCore/rendering/RenderBoxModelObject.cpp:1055
> > +        return areaSize.height() - tileSize.height() - result;
> 
> Why do we involve the tileSize in these calculations? Isn't edge at the
> border of the areaSize?

Because percentages are relative to (boxSize - tileSize).

> > Source/WebCore/rendering/style/FillLayer.h:206
> > +    unsigned m_backgroundYOrigin : 2; // Edge
> 
> Can't we define these variables like this?
> 
> Edge m_backgroundXOrigin : 2;
> Edge m_backgroundYOrigin : 2;

No, the Windows compiler doesn't allow enums in bitfields.

> > Source/WebCore/rendering/style/RenderStyleConstants.h:234
> > +enum class Edge { Top, Right, Bottom, Left };
> 
> Why do we have different enums for the same thing?
> BackgroundEdgeOrigin (now Edge)
> BoxSide
> PhysicalBoxSide

Yes, we could change all these (as long as there aren't implicit orderings).