coil-kt / coil

Image loading for Android and Compose Multiplatform.
https://coil-kt.github.io/coil/
Apache License 2.0
10.87k stars 670 forks source link

SVG view box should not be used for its aspect ratio #731

Closed zhanghai closed 1 year ago

zhanghai commented 3 years ago

Describe the bug

(Copying discussion from #688 becuase it's missing follow-up probably because it's a PR instead of an issue.)

I see, thanks for the clarification about using view box for fitting the target size. However, it still seems to me the original behavior is correct.

For example, if we have an SVG that is 100x200, but the view box is only 100x100, the image actually only has a 1x1 aspect ratio (everything outside of the view box isn't rendered).

This doesn't sound correct according to my understanding of SVG.

  1. Although the view box may be smaller, it's still perfectly valid for content to be drawn outside the view box and be shown in the final SVG, because:

    1. the viewBox attribute is essentially just a shorthand for a transformation matrix:

      The effect of the ‘viewBox’ attribute is that the user agent automatically supplies the appropriate transformation matrix to map the specified rectangle in user space to the bounds of a designated region (often, the viewport).

      https://www.w3.org/TR/SVG11/coords.html#ViewBoxAttribute

    2. the default value for preserveAspectRatio (when viewBox is present) is xMidYMid instead of none, so that uniform scaling is performed and the view box is centered, leaving additional space for drawing.

      • xMidYMid (the default) - Force uniform scaling. Align the midpoint X value of the element's ‘viewBox’ with the midpoint X value of the viewport. Align the midpoint Y value of the element's ‘viewBox’ with the midpoint Y value of the viewport.

      https://www.w3.org/TR/SVG11/coords.html#PreserveAspectRatioAttribute

      (One potential source of confusion is that for Android VectorDrawable XML files, there is no preserveAspectRatio equivalent and android:viewportWidth & android:viewportHeight behaves as if preserveAspectRatio is none. But Android VectorDrawable XML isn't SVG.)

      1. The SVG file in my previous comment is an example of this (It's the Material Design icon for "image", and obviously draws outside its view box), and I've confirmed in EOG and Inkscape that they treat the SVG file as if the view box is just a transformation matrix, and nothing else (size, aspect ratio, zoom) is affected by the view box.
      <svg xmlns="http://www.w3.org/2000/svg" width="24" height="24" viewBox="0 0 6 24" version="1.1">
          <path fill="#000000" d="M 10,5 V 19 H -4 V 5 H 10 M 10,3 H -4 c -1.1,0 -2,0.9 -2,2 v 14 c 0,1.1 0.9,2 2,2 h 14 c 1.1,0 2,-0.9 2,-2 V 5 C 12,3.9 11.1,3 10,3 Z m -4.86,8.86 -3,3.87 L 0,13.14 -3,17 H 9 Z" />
      </svg>
  2. Even if no content is drawn outside of the view box, it is still perfectly valid for an SVG file to utilize the view box for transformation, and it should be handled as if the transformation were applied at a root group instead of as the view box.

    (i.e. I didn't find anything in the specification that says the viewBox attribute should be the viewport of the user agent, and on the contrary, the specification seems to suggest the viewBox attribute is just another way to specify a transformation. The value for viewBox are the coordinates pre-tranform, but it's the post-tranform coordinates that should be rendered and respected.)

    Take this SVG file for an ellipse as an example:

        <svg xmlns="http://www.w3.org/2000/svg" width="48" height="24" viewBox="0 0 24 24" preserveAspectRatio="none" version="1.1">
            <circle cx="12" cy="12" fill="#000000" r="12" />
        </svg>

    It should be handled exactly like the following equivalent SVG file:

        <svg xmlns="http://www.w3.org/2000/svg" width="48" height="24" version="1.1">
            <g transform="scale(2, 1)">
                <circle cx="12" cy="12" fill="#000000" r="12" />
            </g>
        </svg>

    I've confirmed that both EOG and Inkscape treats the two SVG files in exactly the same way.

    This is actually also covered similarly in the specification's discussion of how the user agent may implement viewBox with transform:

    ... The effect is equivalent to having a viewport of size 300px by 200px and the following supplemental transformation in the document, as follows:

    <?xml version="1.0" standalone="no"?>
    <!DOCTYPE svg PUBLIC "-//W3C//DTD SVG 1.1//EN" 
      "http://www.w3.org/Graphics/SVG/1.1/DTD/svg11.dtd">
    <svg width="300px" height="200px" version="1.1"
         xmlns="http://www.w3.org/2000/svg">
      <g transform="scale(0.2)">
        <!-- Rest of document goes here -->
      </g>
    </svg>

    https://www.w3.org/TR/SVG11/coords.html#ViewBoxAttribute

  3. Even if some non-standard handling of viewBox for fitting the target size is desired for some reason, it should actually respect the value of preserveAspectRatio to be a correct implementation.

    The current implementation would return an incorrect result if preserveAspectRatio is set to none, in which case the aspect ratio from the width and height attributes should definitely be used instead of the one from viewBox.

    I still don't see why we need non-standard handling of viewBox for fitting the target size though.

Expected behavior

SVG view box should not be used for its aspect ratio.

To Reproduce

Read changelog or #688.

Logs/Screenshots

N/A

Version

1.2.0

colinrtwhite commented 1 year ago

Sorry for the long delay on this! I've avoided this issue for a while since I don't think I have enough understanding of the SVG spec to say what the correct behaviour should be here. It's possible Coil is implementing logic out of line with the spec, however overall I lean towards keeping the behaviour as is since there haven't been any follow up issue reports related to SVG rendering and this behaviour.

Additionally, I think changing the default behaviour at this point would be more detrimental to end users even if the behaviour is incorrect. Also it's possible to avoid the default behaviour by setting useViewBoundsAsIntrinsicSize = false.