Bug 154478 - Repeated background images have the wrong position when using bottom/right-relative background-position
Summary: Repeated background images have the wrong position when using bottom/right-re...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: Safari 9
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Simon Fraser (smfr)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-02-19 16:13 PST by Simon Fraser (smfr)
Modified: 2016-02-22 17:28 PST (History)
4 users (show)

See Also:


Attachments
Testcase (614 bytes, text/html)
2016-02-19 16:13 PST, Simon Fraser (smfr)
no flags Details
Patch (51.42 KB, patch)
2016-02-19 22:43 PST, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Patch (51.85 KB, patch)
2016-02-19 22:44 PST, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Patch (53.68 KB, patch)
2016-02-19 23:24 PST, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Patch (54.02 KB, patch)
2016-02-20 11:25 PST, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Patch (24.37 KB, patch)
2016-02-22 12:45 PST, Simon Fraser (smfr)
hyatt: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2016-02-19 16:13:08 PST
Created attachment 271816 [details]
Testcase

We get the image position wrong when repeating, with background-position: bottom 10px right 20px;
Comment 1 Simon Fraser (smfr) 2016-02-19 22:43:44 PST
Created attachment 271850 [details]
Patch
Comment 2 Simon Fraser (smfr) 2016-02-19 22:44:31 PST
Created attachment 271851 [details]
Patch
Comment 3 Simon Fraser (smfr) 2016-02-19 23:24:45 PST
Created attachment 271853 [details]
Patch
Comment 4 Simon Fraser (smfr) 2016-02-20 11:25:32 PST
Created attachment 271863 [details]
Patch
Comment 5 Darin Adler 2016-02-21 16:52:57 PST
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.
Comment 6 Simon Fraser (smfr) 2016-02-21 21:54:17 PST
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.
Comment 7 Simon Fraser (smfr) 2016-02-22 12:45:19 PST
Created attachment 271943 [details]
Patch
Comment 8 Dave Hyatt 2016-02-22 12:47:48 PST
Comment on attachment 271943 [details]
Patch

r=me
Comment 9 Simon Fraser (smfr) 2016-02-22 14:27:28 PST
https://webkit.org/b/154478
Comment 10 Said Abou-Hallawa 2016-02-22 15:45:54 PST
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
Comment 11 Simon Fraser (smfr) 2016-02-22 17:28:57 PST
(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).