SVG graphic in background-image does not preserve aspect ratio when height is not specified on SVG

RESOLVED FIXED in Firefox 54

Status

()

Core
SVG
RESOLVED FIXED
11 months ago
11 months ago

People

(Reporter: birtles, Assigned: mantaroh)

Tracking

Trunk
mozilla54
Points:
---

Firefox Tracking Flags

(firefox54 fixed)

Details

(Whiteboard: [webcompat], URL)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Reporter)

Description

11 months ago
Created attachment 8835274 [details]
Test case

I'm really not sure what is going on here, but for the test URL you can see that the Mario in the middle appears squashed. In Chrome and Edge, however, he appears normal.

I've attached a reduced test case that shows the same problem with an arrow graphic.

Note that the SVG graphic specifies a width but not a height. If I add a height it appears as expected (not squashed). It seems the aspect ratio could be inferred from the viewBox. I'm not sure if we're supposed to do that or not, it's possible we're even doing the correct thing, but at least Chrome and Edge do. It's also likely this is a dupe of an existing bug but I'm filing this for now until I find that bug.
(Reporter)

Comment 1

11 months ago
Mantaroh, here is another SVG-related bug that would be great to fix if you have time. This sort of interop bug is the sort thing that frustrates authors a lot. I suspect this cross a number of spec boundaries, however, and will take some time to work out the correct sizing behavior.

Updated

11 months ago
Whiteboard: [webcompat]
(Reporter)

Comment 2

11 months ago
Safari appears to matches the behavior of Blink and Edge for this test case.
Display of images where attributes are missing is pretty much unspecced. All UAs try to guess the user's actual intent. The accuracy of this guessing varies.
(Reporter)

Comment 4

11 months ago
CSS images 3 does talk about an intrinsic dimensions in a way that suggests what is expected but without actually defining how this works for SVG:

  "The term intrinsic dimensions refers to the set of the intrinsic height,
  intrinsic width, and intrinsic aspect ratio (the ratio between the width and
  height), each of which may or may not exist for a given object. These
  intrinsic dimensions represent a preferred or natural size of the object
  itself; that is, they are not a function of the context in which the object is
  used. CSS does not define how the intrinsic dimensions are found in general.

  "Raster images are an example of an object with all three intrinsic
  dimensions.  SVG images designed to scale might have only an intrinsic aspect
  ratio; SVG images can also be created with only an intrinsic width or height.
  CSS gradients, defined in this specification, are an example of an object with
  no intrinsic dimensions at all. Another example of this is embedded documents,
  such as the <iframe> element in HTML. An object cannot have only two intrinsic
  dimensions, as any two automatically define the third."
  (https://drafts.csswg.org/css-images-3/#intrinsic-dimensions)

So in this case we have an intrinsic width and an intrinsic aspect ratio. As the spec text if we have two intrinsic dimensions we can calculate the third. So presumably we should treat this image as having an intrinsic height calculated from the aspect ratio and width.

That said, I'm not sure if that's actually where we're coming unstuck. It may be we calculate correct intrinsic height but when we size the graphic to the box we do it differently. And I agree it doesn't actually spell out the mapping to SVG for these properties.
(Assignee)

Comment 5

11 months ago
In the gecko, we will calculate and set image size when drawing the image.[1]

[1] https://dxr.mozilla.org/mozilla-central/rev/698de2db1b16a5ef3c6a39f0f72885e69aee4022/layout/painting/nsCSSRendering.cpp#4048-4052

This size is based on the value from imgIContainer object. (in this case, VectorImage which is subclass)
If we don't set the hight value only, we will use the default size.[2]

[2] https://dxr.mozilla.org/mozilla-central/rev/698de2db1b16a5ef3c6a39f0f72885e69aee4022/layout/painting/nsCSSRendering.cpp#5585-5594

I think that we will need to calculate the width or height value from two intrinsic dimensions.

Rough implement: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8fc1f60966cfb05c71e21197ff1af5e1d57189ba
Assignee: nobody → mantaroh
Comment hidden (mozreview-request)

Comment 9

11 months ago
mozreview-review
Comment on attachment 8840299 [details]
Bug 1338042 - Compute a missing width/height from the aspect ratio and the other dimension.

https://reviewboard.mozilla.org/r/114740/#review116286

r+ with review comments addressed

::: layout/painting/nsCSSRendering.cpp:5428
(Diff revision 1)
>        if (haveHeight) {
>          result.SetHeight(nsPresContext::CSSPixelsToAppUnits(imageIntSize.height));
>        }
> +
> +      // If we can calculate width or height from two intrinsic factors,
> +      // we will compute third intrinsic dimension.

The comment is perhaps better written as

// If we know the aspect ratio and one of the dimensions, 
// we can compute the other missing width or height.

::: layout/reftests/svg/reftest.list:478
(Diff revision 1)
>  == mask-use-element-01.svg pass.svg
>  == clip-use-element-01.svg pass.svg
>  == clip-use-element-02.svg pass.svg
>  == filter-use-element-01.svg pass.svg
> +
> +== background-svg-without-height.html background-ref.html

reftests are both generally alphabetically ordered in this file (except for this mess at the end that needs fixing), and they are also numbered in case we want to create multiple tests for the same issue.
Attachment #8840299 - Flags: review?(longsonr) → review+

Comment 10

11 months ago
mozreview-review
Comment on attachment 8840299 [details]
Bug 1338042 - Compute a missing width/height from the aspect ratio and the other dimension.

https://reviewboard.mozilla.org/r/114740/#review116292

also the checkin comment is misspelled it should be Compute a missing width/height from the aspect ratio and the other dimension.
Comment hidden (mozreview-request)
(Assignee)

Comment 12

11 months ago
mozreview-review-reply
Comment on attachment 8840299 [details]
Bug 1338042 - Compute a missing width/height from the aspect ratio and the other dimension.

https://reviewboard.mozilla.org/r/114740/#review116292

Thanks, Robert.

I addressed comments and tests orders.

Comment 13

11 months ago
Pushed by mantaroh@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/f95acc1bb74e
Compute a missing width/height from the aspect ratio and the other dimension. r=longsonr+218550

Comment 14

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f95acc1bb74e
Status: ASSIGNED → RESOLVED
Last Resolved: 11 months ago
status-firefox54: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.