1047973 - In SVG pattern with patternUnits="objectBoundingBox" and preserveAspectRatio="... meet" stretches to the size of a shape

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Aleksey]

User Agent: Mozilla/5.0 (Windows NT 6.1; rv:29.0) Gecko/20100101 Firefox/29.0 (Beta/Release)
Build ID: 20140506152807

Steps to reproduce:

1. Create an inline SVG element in HTML code.
2. Create <defs> and a <pattern> inside it, with 
 - id="pattern-1" - doesn't matter;
 - patternUnits="objectBoundingBox" - to use a host shape's coordinate system for sizing the pattern;
 - height="1" width="1" - to make pattern exactly fit a host shape;
 - preserveAspectRatio="... meet" - to make the pattern's shape fit a the host shape.
3. Add a shape inside the <pattern>
4. Give the pattern the appropriate viewBox - according to the size and position of the shape inside it.

5. Create a shape to use the <pattern> as a fill, give it some size and an attribute fill="url('#pattern-1')",
for example: <rect id="host" x="0" y="0" width="100" height="200" fill="url('#pattern-1')">


Actual results:

#pattern-1 stretches to the whole #host area.


Expected results:

#pattern-1 should have fit inside #host, just as background-size:contain; in CSS works. At least, in all other tested browsers (Chrome 29, Safari 5.1 Windows, Opera 12.15, Opera 16, IE 10) it worked the expected way.

The live example is here: http://codepen.io/bonflash/pen/iCfka The red anchor is not expected to stretch.

Comment 1

[Robert Longson [:longsonr]]

Attachment: testcase as attachment

Comment 2

[Robert Longson [:longsonr]]

Attachment: corrected testcase

Comment 3

[Robert Longson [:longsonr]]

IE11 and Opera 12 seem to display the testcase differently to each other.

Comment 4

[Robert Longson [:longsonr]]

Would be nice if you could create a simpler testcase e.g. no transforms on the pattern content items. Only rect shapes with simple integer coordinates etc.

Comment 5

[Robert Longson [:longsonr]]

Here's an example of a simple testcase: http://mxr.mozilla.org/mozilla-central/source/layout/reftests/svg/objectBoundingBox-and-pattern-01a.svg?raw=1

Comment 6

[Robert Longson [:longsonr]]

The pattern has height="1" width="1" patternUnits="objectBoundingBox" that means it's made into the width and height of the shape it occupies.

Comment 7

[Robert Longson [:longsonr]]

Except there's still the preserveAspectRatio...

Comment 8

[Daniel Holbert [:dholbert]]

Attachment: testcase 2

I think this demonstrates the preserveAspectRatio issue (the stretching in the first chunk of the first testcase).

The <pattern> has preserveAspectRatio="xMidYMid meet", but we render this as if it had preserveAspectRatio="none". (i.e. we stretch the purple circle horizontally, whereas Blink & Presto & emacs do not)

Comment 9

[Aleksey]

 Daniel, yes, that's right. According to https://developer.mozilla.org/en-US/docs/Web/SVG/Attribute/preserveAspectRatio :

meet (the default) - Scale the graphic such that:

    aspect ratio is **preserved**
    the entire viewBox is visible within the viewport
    the viewBox is scaled up as much as possible, while still meeting the other criteria

And thank you for a testcase!

IE 10, meanwhile, indeed does it in a wrong way.

Comment 10

[Amelia Bellamy-Royds]

I thought I'd add a comment that the problem with `preserveAspectRatio` and patterns is general, it applies to `slice` values as well as `meet`.

See for example, this fiddle: http://fiddle.jshell.net/xhh7a/1/
Which is explained in detail on this Stack Overflow post: http://stackoverflow.com/a/22886596/3128209

The third row should have undistorted images because of the viewBox and preserveAspectRatio values.

It works as intended in Chrome and IE.

Comment 11

[Robert Longson [:longsonr]]

Attachment: patch with reftest

Comment 12

[Robert Longson [:longsonr]]

Attachment: just the one bug fix this time

Comment 13

[Daniel Holbert [:dholbert]]

Comment on attachment 8588322 [details] [diff] [review]
just the one bug fix this time

Review of attachment 8588322 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/reftests/svg/viewBox-and-pattern-04.svg
@@ +1,5 @@
>  <!--
>       Any copyright is dedicated to the Public Domain.
>       http://creativecommons.org/publicdomain/zero/1.0/
>  -->
> +<svg xmlns="http://www.w3.org/2000/svg">

Nit: right now you're creating this testcase as an 'hg cp' of viewBox-and-pattern-03.svg, but the file is almost entirely different, so I don't think there's any benefit to tracking it as a copy.

Might be better to just remove it & re-add it as a new file, like so:
 hg forget layout/reftests/svg/viewBox-and-pattern-04.svg
 hg qref
 hg add layout/reftests/svg/viewBox-and-pattern-04.svg
 hg qref

::: layout/svg/nsSVGPatternFrame.cpp
@@ +642,3 @@
>    SVGSVGElement *ctx = nullptr;
>    nsIContent* targetContent = aTarget->GetContent();
> +  double scaleX, scaleY;

s/double/gfxFloat/, I think.

Comment 14

[Robert Longson [:longsonr]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1e197f7a92f4

Comment 15

[Robert Longson [:longsonr]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/731579510bef

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/731579510bef