609714 - Fix tests layout/reftests/backgrounds/background-size-no-intrinsic-{height,width}-image.html

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Daniel Holbert [:dholbert]]

The reftests
> layout/reftests/backgrounds/background-size-no-intrinsic-{height,width}-image{,-ref}.html
are still marked as "fails ==", even though we now support the feature they require (SVG-as-an-image -- which wasn't implemented when these tests were checked in, which is why they were marked "fails" when added).

However, I suspect the tests might be failing now because they make incorrect assumptions.  At least, our rendering matches both Opera 10.63 & Chromium 8.0.551.0 on each test & reference case.

I'm filing this bug on investigating & fixing the tests or the code (if necessary) so that they pass.

Mozilla/5.0 (X11; Linux x86_64; rv:2.0b8pre) Gecko/20101103 Firefox/4.0b8pre

Comment 1

[Jeff Walden [:Waldo]]

I looked at this again.  The relevant CSS3 description is:

"An ‘auto’ value for one dimension is resolved by using the image's intrinsic ratio and the size of the other dimension, or failing that, using the image's intrinsic size, or failing that, treating it as 100%.
If both values are ‘auto’ then the intrinsic width and/or height of the image should be used, if any. If the image has neither an intrinsic width nor an intrinsic height, its size is determined as for ‘contain’."

The image used in these is SVG whose content is a rect with width/height="100%", then applied against background-size: 32px auto, and vice versa.  Does that rec establish an intrinsic ratio of 1:1?  If it does, which is not necessarily obvious to me, then the tests would be wrong.  And if that's the case, is there any way to specify an image without an intrinsic height and/or width?

Comment 2

[Jeff Walden [:Waldo]]

bz tells me (or so I understand him!) that, without a viewbox in the SVG image, the one in these tests does *not* have an intrinsic ratio, so therefore the tests are correct.

Comment 3

[Jeff Walden [:Waldo]]

Attachment: Patch

Poking both the background-size person and the vector-image-background person for review, fight it out between the two of you as to who's the more appropriate reviewer -- or you can both review it if you want.  :-)

A brief spec quibble:

http://dev.w3.org/csswg/css3-background/#the-background-size

This is the spec language for an "auto" value for either, or both, background-size components:

> An ‘auto’ value for one dimension is resolved by using the image's intrinsic
> ratio and the size of the other dimension, or failing that, using the image's
> intrinsic size, or failing that, treating it as 100%.
>
> If both values are ‘auto’ then the intrinsic width and/or height of the image
> should be used, if any. If the image has neither an intrinsic width nor an
> intrinsic height, its size is determined as for ‘contain’.

I find the last sentence mildly confusing.  Referring to the language for 'contain':

> Scale the image, while preserving its intrinsic aspect ratio (if any), to
> the largest size such that both its width and its height can fit inside the
> background positioning area.

So for background-size: auto auto with an image with no intrinsic sizes (ergo no intrinsic ratio), it seems to mean "scale the image to the largest size such that both its width and its height can fit inside the background positioning area".  Is this any different from what would happen if you simply applied the instructions for a *single* dimension not having an intrinsic size, twice?  I don't see a difference.

To me the "If the image has neither...as for 'contain'." sentence is confusing and adds no new meaning.  Do you agree?  Seems worth taking to www-style if I'm not just misreading this somehow.

Comment 4

[Daniel Holbert [:dholbert]]

(In reply to comment #3)
> A brief spec quibble:
[...]
> an image with no intrinsic sizes (ergo no intrinsic ratio)

That "ergo" doesn't actually follow. An SVG image can *have* an intrinsic ratio *without having* a (pixel-valued) intrinsic size.

e.g. the author might leave height/width unspecified on the <svg> node (defaulting them to 100%) -- which means the image would have no definite [pixel-valued] intrinsic size.  But if there's a viewBox attribute on the root <svg> node, then we can still infer an intrinsic *ratio* from the viewBox, despite the lack of an intrinsic size.  See the second half of nsSVGOuterSVGFrame::GetIntrinsicRatio() for where that happens internally.

(and see e.g. http://mxr.mozilla.org/mozilla-central/source/layout/reftests/svg/as-image/limeInRed-noSVGDimensions-viewBox.svg?raw=1 for an example of such a file.)

> it seems to mean "scale the image to the largest size such
> that both its width and its height can fit inside the background positioning
> area

(It sounds like you're saying "just give the image a width & height that are exactly equal to the background area's dimensnions")

Without a viewBox, that's indeed what we'd do.  However, if there's a viewBox / intrinsic ratio, then it's not -- in that case, the
 "while preserving its intrinsic ratio"
part of the 'contain' description becomes important and will constrain our choice of height/width.  (For example, an SVG image with a square viewBox would get a square region to render into, even if it's being used as the background for a tall-and-skinny div.)

> To me the "If the image has neither...as for 'contain'." sentence is confusing
> and adds no new meaning.  Do you agree?

(No, I don't agree -- as noted above, I think the intrinsic ratio aspect of 'contain' is important to consider, and that adds meaning.)

Comment 5

[Daniel Holbert [:dholbert]]

Comment on attachment 500714 [details] [diff] [review]
Patch

>diff --git a/layout/base/nsCSSRendering.cpp b/layout/base/nsCSSRendering.cpp
>+   * @return the image size in appunits. CSS gradient images, and SVG images
>+   * can have dimensions without an intrinsic size, so we have to pass in the

Nit: Remove the comma after "images" there.

>+      if (usedSize.mWidthType == nsStyleBackground::Size::eAuto && !gotWidth) {
>+        usedSize.mWidth.mLength = 0;
>+        usedSize.mWidth.mPercent = 1.0f;
>+      }
[...]
>+      if (usedSize.mWidthType == nsStyleBackground::Size::eAuto) {
>+        if (usedSize.mHeightType == nsStyleBackground::Size::eAuto) {
>+          scaleX = scaleY = 1.0f;

So here, you're just giving the image a viewport equal to the full background size. (You're doing the "It sounds like you're saying" chunk in Comment 4)  However, since we do have an intrinsic ratio, we really should be setting our dimensions in such a way that we preserve that ratio.

In particular -- suppose you have e.g. an image like the following:
  ab
  cd
where a,b,c,d represent blocks of color.
Suppose also that we have a square viewBox that exactly contains the square region displayed above.

Now, if we use that as the background for a wide rectangular div, then I'd expect 'contain' to give us this output:
 (i)  --------------
      |ab          |
      |cd          |
      -------------- 
and I'd expect 'cover' to give us this output:
 (ii) --------------
      |aaaaaabbbbbb|
      |aaaaaabbbbbb|
      -------------- 
whereas your current patch would give us this horizontally-stretched output for both "cover" and "contain", I think:
 (iii)--------------
      |aaaaaabbbbbb|
      |ccccccdddddd|
      -------------- 

(SIDE NOTE: so SVG has its own internal mechanism for keeping itself from being distorted / cropped, when rendered into an arbitrarily-sized region -- the "preserveAspectRatio" attribute.  I was initially worried that preserveAspectRatio combined with 'contain' vs. 'cover' would become horrendously complex.  BUT, I think it's actually sensible.  If we fix this bug correctly, rendering the image into a region that matches its viewBox's intrinsic ratio, then the "preserveAspectRatio" attribute shouldn't actually have any effect at all.  That's because it only comes into play when we render to a region whose aspect ratio does NOT match the viewBox's.)

Comment 6

[Daniel Holbert [:dholbert]]

(For simplicity, let's further suppose that the SVG image in my example from comment 5 has preserveAspectRatio="none", so that it's possible for it to be stretched/distorted as shown in figure (iii).)

Comment 7

[Jeff Walden [:Waldo]]

Comment on attachment 500714 [details] [diff] [review]
Patch

Fun times.  :-)  Sounds like I need to write a few more tests before I update the patch to do what should be done here, because the two (three) tests here aren't doing much more than scratching the surface.  I'll post them for a review when I get to writing them -- hopefully very soon, ideally by the start of next week, for this to be properly and fully addressed this release.  Then I'll start work to update the patch itself.

Comment 8

[Jeff Walden [:Waldo]]

Attachment: Tests

Eat 'em up.  Or don't.  :-)  The only way I see to write this many tests for such a complicated algorithm is with an implementation, even a buggy one, and iterating on both to perfection.  I had hope to use an existing implementation to test, but Opera's seems buggy on a few of them I threw at it, and I don't know if anyone else has a complete implementation.  (Does IE9?  I don't have remote desktop set up to check via the desktop in the office, alas.)

I've put in a little effort toward writing the actual patch.  It's basically a multiply by 4 (dimensions or not) and a multiply by 2 (ratio or not) in complexity.  I'm not sure now what's the best way to organize the algorithm, but there are enough interdependencies that it's quite possible that multiply by eight really is the best way.  If so, size computation is going to be a nightmare.  I sent a query to www-style to see if anyone else has done this work and has an approach they're willing to share, will see if I get a response on that before making any more serious effort here.

http://lists.w3.org/Archives/Public/www-style/2011Jan/0205.html

Are the very few tests that test half-pixel rendering acceptable, or should I change those few tests to use slightly different dimensions such that the half-pixel dimension no longer occurs?  A couple spot-deviations in the tests shouldn't matter, I think, since the point is to check the math, not rigorously adhere to the template.

Comment 9

[Jeff Walden [:Waldo]]

Attachment: Moderately-working patch, much-fixed-up-and-correct tests

I haven't run against anything other than layout/reftests/backgrounds/, but on that directory it's down to 41 failures (all in the new tests).  I'll post the results from those runs after this.

Comment 10

[Jeff Walden [:Waldo]]

Attachment: Results for layout/reftests/backgrounds/ with that patch

Comment 11

[Jeff Walden [:Waldo]]

Oh, the new-test failures all seem to be in tests where the SVG 1) has a viewBox, and 2) has width/height attributes on the <svg> one or both as percentages (or omitted).  I haven't figured out yet whether that's my mistake or an existing one, except the extent of spread-outness over various background-sizes has me thinking it might (might!) not be me.

Comment 12

[Daniel Holbert [:dholbert]]

Taking one test-failure example:
> tall--auto--omitted-width-percent-height-viewbox.html

That testcase uses this SVG file...

> <svg xmlns="http://www.w3.org/2000/svg"
>      height="50%"
>      viewBox="0 0 4 32"
>      preserveAspectRatio="none">
> <rect width="100%" height="100%" fill="lime"/>

...as the background for a div with "width: 64px; height: 128px; background-size: auto".

From comparing this against the equivalent test with |height| unspecified on <svg>, it looks like you're assuming the height="50%" just chops off the bottom half of the rendered SVG content.

However, in fact, the height=50% has no effect here.  The |height| attribute on an <svg> element is merely a suggested size for its viewport, and it's ignored if an alternative height is provided by the host document (as is the case here, where the host is effectively asking for <img width="64px" height="128px">).  

For reference, see the first paragraph of bug 614649 comment 6, and the chunk of of spec referenced there.

Comment 13

[Daniel Holbert [:dholbert]]

d'oh -- though actually in the case of <img width="64px" height="128px"> w/ percent heights/widths on the <svg> element, we do apparently honor the percentages by clipping the painted region of SVG (to be smaller than the <img> element).  *That* is probably a bug.

Comment 14

[Jeff Walden [:Waldo]]

Attachment: Testcase for comment 13

Sort of circling back around to this now, really need to push through and get it done and off my plate...

(In reply to comment #13)
> d'oh -- though actually in the case of <img width="64px" height="128px"> w/
> percent heights/widths on the <svg> element, we do apparently honor the
> percentages by clipping the painted region of SVG (to be smaller than the <img>
> element).  *That* is probably a bug.

I don't see any clipping when I load this testcase with a March 3ish nightly.  Did something change?

Comment 15

[Daniel Holbert [:dholbert]]

> Did something change?

Yes -- bug 614649 might very well have changed a lot of the renderings here.    (It's summed up in bug 614649 comment 44).  We'll now synthesize a viewBox in many cases now -- basically, if there's no viewBox, we'll synthesize one at paint-time, in order to scale whichever dimension(s) have non-percent values on the root <svg> node.

(Having said that, the chunk you quoted from comment 13 seems to be about a case with percent height & width, so I'd initially expect that it shouldn't have been affected by bug 614649.  So if that rendering changed, I'm not immediately sure why.)

Comment 16

[Jeff Walden [:Waldo]]

Attachment: (Better?) Testcase for comment 13

Actually, it's probably that the SVG image has percent width *and* percent height, which probably always auto-scaled.  It's this case, where there's a non-percent width/height and a percent height/width, that's wrong.  So with this, then, the expected rendering is a tall black rectangle whose left half (not upper left quadrant) is lime?

If width="8px" height="50%" is supposed to be identical to width="8px" height="100%", that's going to change the expected results for a lot of these tests.  At this point the tests are quite well paged out of memory, so I should probably go through *all* of them and make the necessary updates for that, as I suspect I'd mess up any sort of automatic correction if I tried it.

Comment 17

[Daniel Holbert [:dholbert]]

(In reply to comment #16)
> Created attachment 519057 [details]
> (Better?) Testcase for comment 13

I think our current rendering of this testcase (an entirely-lime rect, 16x256, w/ black border) is correct.  Conceptually, the <img> element's height/width override the height/width on the root <svg> element - so it makes sense to me that the SVG attributes have no effect here.

If you remove the height or width on the <img>, *then* it'll ends up using the values provided in the SVG. (in this case, the 50% height has nothing to resolve against, so it turns into 150px -- but if you put a height on the body, it'll resolve to 50% of that.)

Comment 18

[Jeff Walden [:Waldo]]

Attachment: Unbitrotted, a bit sketchy/hackish in places, layout/reftests/backgrounds passes

Comment 19

[Jeff Walden [:Waldo]]

Attachment: Ready to go, tryserveration in progress

As I mentioned in-person, I'm not entirely happy about the way this abstraction's currently set up.  It seems to me the image size should be passed to the nsLayoutUtils::DrawImage call in all places, and that method shouldn't do the work of computing image size at all, because the image size and drawing dimensions go hand-in-hand as far as size negotiations go when rendering.  But that's a trickier bit of change than just converting background images, and it touches more stuff (list-style-image and something in XUL trees, as I recall), so it's probably better to do it in multiple steps.  In any case those concerns aren't nearly as important as making vector images work (correctly) as backgrounds.

The ComputeUnscaledSize/ComputeDrawnSize system is also a bit unsatisfactory, but given the latter alone is a 250-line algorithm, I was struggling to find some way to break it down even slightly.  (CDS could be broken into sub-methods, perhaps, since it's if(...) return...; if(...) return; ..., but I think that breaks some of the clarity in clearly handling all the cases, especially since some of the cases flow into each other.

As discussed, the diagonal testing is not exhaustive because 1) it's hard to do so and get nice round numbers for reference comparison and 2) because the numbers all flow the same place, so if one test passes that should be indicative of how a bevy of such tests would behave.

Tryservering is happening here:

http://tbpl.mozilla.org/?tree=Try&rev=bc30d782fe08

Have at it!

Comment 20

[Jeff Walden [:Waldo]]

Attachment: Very very slightly unbitrotted

Comment 21

[Jeff Walden [:Waldo]]

Try reveals that a couple tests seem to rely on previous behavior for gradient backgrounds, directly contrary to what CSS3 Images says about gradient dimension-ness:

layout/reftests/bugs/538909-1.html:
This assumes background-size: 100px creates a square.  Also, at least with my patch, a vertical rather than diagonal gradient is being created.  I'm not sure if that's me or not yet.

layout/reftests/image-element/gradient-html-07b.html:
Same song, second verse almost exactly.

I'll keep looking at those to figure out what should happen.  Maybe gradients should have an intrinsic ratio if there's truly concern about compatibility...

Comment 22

[Daniel Holbert [:dholbert]]

(roc or dbaron should probably review this, too, btw)

Comment 23

[Daniel Holbert [:dholbert]]

Comment on attachment 534582 [details] [diff] [review]
Very very slightly unbitrotted

Haven't made it all the way through yet, but here's what I've got so far:
>+  /*
>+   * Compute the "unscaled" size (insofar as the image has one) of the image in
>+   * mSize

"... in app units."


>+  void
>+  ComputeUnscaledSize(const nsSize& aBgPositioningArea,
>+                      PRBool& haveWidth, PRBool& haveHeight,
>+                      nsSize& aRatio);

Shouldn't haveWidth & haveHeight be aHaveWidth & aHaveHeight?

Also, the documentation should mention what |aRatio| is for.

Also, it's a bit awkward that this function stores* its resulting size in a new member variable, whereas ComputeDrawnSize returns its resulting size directly.
*(or doesn't store it, depending on haveWidth/haveHeight...)

I'd suggest the following change, to address that:
Given that these two functions are only called once, one-after-the-other, in ImageRenderer::ComputeSize, could we...
 - have ComputeUnscaledSize return its result as an nsSize (yay, consistency!)
 - then pass that as another param to ComputeDrawnSize (so ComputeDrawnSize doesn't touch mSize anymore)
 - After computeDrawnSize returns, we can set mSize to whatever combination of the above functions' results is appropriate, depending on haveWidth/haveHeight.

That would make it way easier to track where/if/how mSize is set, because it'd then be set in *one* definitive place. (rather than right now, where it could be partially set in ComputeUnscaledSize and partially set at the end of ComputeSize, or entirely set either of those).

>+  nsSize
>+  ComputeDrawnSize(const nsStyleBackground::Size& aLayerSize,
>+                   const nsSize& aBgPositioningArea,
>+                   PRBool haveWidth, PRBool haveHeight,

(as above, haveWidth & haveHeight want an "a" prefix)

This method is very similar in name to nsLayoutUtils::ComputeSizeForDrawing, which is a bit confusing, particularly given that the functions aren't directly related. :-/  I don't know if there's much we can do about that...

>+  nsSize                    mSize; // unscaled size of the image

Add to the comment: "in app units".  (In my initial reading, I assumed this was in native image pixels, in which case nsSize wouldn't be the right type.)

>+static nsSize
>+ComputeContainCoverSizeFromRatio(const nsSize& aBgPositioningArea,
>+                                 const nsSize& aRatio, FitType fitType)
>+{
>+  float scaleX = double(aBgPositioningArea.width) / aRatio.width;
>+  float scaleY = double(aBgPositioningArea.height) / aRatio.height;

Do these want to be floats or doubles? :)  Right now we make them one, then the other, for no good reason.

Also, this function could probably use the same "division by zero" / "image with nonsense ratio" assertions that you have at the beginning of another similar function in this patch.

>+    case COVER:
>+      if (scaleX < scaleY) {
>+        size.width = NSCoordSaturatingNonnegativeMultiply(aRatio.width, scaleY);
>+        size.height = aBgPositioningArea.height;
>+      } else {
>+        size.width = aBgPositioningArea.width;
>+        size.height = NSCoordSaturatingNonnegativeMultiply(aRatio.height, scaleX);
>+      }

It seems like a common real-world case here will be when "scaleX == scaleY" (when they're both 1.0, say).  In that case, we just want:
>   size = aBgPositioningArea;
It might be worth explicitly handling that as a special case (before the switch statement), to avoid unnecessary floating-point math and possible resulting imprecision.  (Or not... up to you. :))

Comment 24

[Jeff Walden [:Waldo]]

Update: the two reftest failures go away if I change the ratio assigned for gradients from (0, 0) to (1, 1).  I need to test other browsers and see what they do, but I have some difficulty believing they do the 1x1 thing.  But if they do, looks like maybe CSS3 Images should change, or something.

Comment 25

[Jeff Walden [:Waldo]]

(1, 1) is a special case that applies in those tests (or so it appears) because the background positioning area in those tests is square.  A non-repeating gradient whose size is only half-"auto" (i.e. "50px"/"50px auto" or "auto 50px") displays in all the browsers I can find, current-us included, as though the gradient were sized proportionate to the background positioning area while respecting the given dimension.  This idea of an image which "knows" the size of background positioning area isn't part of CSS3 Backgrounds or CSS3 Images.  I'll take it up with www-style tomorrow, I guess, and see if I can get people to agree on some particular behavior and how to make sense of it.

Comment 26

[Jeff Walden [:Waldo]]

Attachment: Address previous comments, fix existing tests, update algorithm avoiding full-background repaints

I talked over the problem with the two failures with Tab Atkins, and he's of the opinion that the rendering this patch implements is the correct one, so I've changed the tests.

The other big change here is that I updated the algorithm to determine when, for a frame size change, the background has to be repainted.  It might have been consistent with the old algorithm, but it definitely wasn't consistent with the new one.  Also nice: I was able (and basically had to, to implement the b-s algorithm properly within the optimization) to get rid of use of very vector-specific APIs in that code path, replacing them with generic ones to get the dimensions and ratio.

And a minor change: I added a gradient test indirectly suggested by Tab when discussing gradient sizing, in layout/reftests/backgrounds/gradient.  It'd be nice in separate bugs to move the gradient tests there, to make it easier to change this code, run a minimal set of tests, and generally detect errors quicker.

(In reply to comment #23)
> >+  void
> >+  ComputeUnscaledSize(const nsSize& aBgPositioningArea,
> >+                      PRBool& haveWidth, PRBool& haveHeight,
> >+                      nsSize& aRatio);

I changed this around a bit to not return via mSize but rather to return by two (count 'em) nscoord outparams.  That seems to parallel the have* parameters much more clearly.  I did the assign-to-mSize-only-once thing at the same time.

> This method is very similar in name to nsLayoutUtils::ComputeSizeForDrawing,
> which is a bit confusing, particularly given that the functions aren't
> directly related. :-/  I don't know if there's much we can do about that...

No ideas immediately spring to mind.

> Add to the comment: "in app units".  (In my initial reading, I assumed this
> was in native image pixels, in which case nsSize wouldn't be the right type.)

It's crazy nobody's yet structified appunits and css pixels so they aren't interconvertible.  :-\

> >+static nsSize
> >+ComputeContainCoverSizeFromRatio(const nsSize& aBgPositioningArea,
> >+                                 const nsSize& aRatio, FitType fitType)
> >+{
> >+  float scaleX = double(aBgPositioningArea.width) / aRatio.width;
> >+  float scaleY = double(aBgPositioningArea.height) / aRatio.height;
> 
> Do these want to be floats or doubles? :)  Right now we make them one, then
> the other, for no good reason.

As in the current code, this doubling-then-floating is to preserve extra precision, and the extra FP traffic should be negligible overall, especially compared to the overhead of actually painting.

> Also, this function could probably use the same "division by zero" / "image
> with nonsense ratio" assertions

Fair.

> It seems like a common real-world case here will be when "scaleX == scaleY"
> (when they're both 1.0, say).

This only applies to contain/cover situations, or to edge-ish cases where the spec algorithms fall through to as-if-by-contain, so I don't see it as important.  And in such cases I guess it'd only get hit if the image's proportions were exactly those of the BPA.  That doesn't seem particularly more real-worldish to me, but maybe I'm not thinking hard about it.

Comment 27

[Jeff Walden [:Waldo]]

Comment on attachment 534970 [details] [diff] [review]
Address previous comments, fix existing tests, update algorithm avoiding full-background repaints

(doubling up on review here per dholbert of a few weeks ago)

Comment 28

[Daniel Holbert [:dholbert]]

Comment on attachment 534970 [details] [diff] [review]
Address previous comments, fix existing tests, update algorithm avoiding full-background repaints

>diff --git a/layout/base/nsCSSRendering.cpp b/layout/base/nsCSSRendering.cpp
>+  nsSize
>+  ComputeDrawnSize(const nsStyleBackground::Size& aLayerSize,
>+                   const nsSize& aBgPositioningArea,
>+                   nscoord aUnscaledWidth, PRBool haveWidth,
>+                   nscoord aUnscaledHeight, PRBool haveHeight,
>+                   const nsSize& aRatio);

s/have/aHave/  (also in the impl of this function)

Also, it might be worth renaming "aRatio" to "aIntrinsicRatio", for consistency with other functions (e.g. nsLayoutUtils::ComputeSizeForDrawing, which your patch adds "aIntrinsicRatio" to).  At the least, you should document in the header comment that aRatio is the image's *intrinsic* ratio, and not some other type of ratio (to avoid confusion over the differently-named "ratio" params).

>+static nsSize
>+ComputeContainCoverSizeFromRatio(const nsSize& aBgPositioningArea,
>+                                 const nsSize& aRatio, FitType fitType)
>+{
>+  NS_ABORT_IF_FALSE(aRatio.height != 0 || aRatio.width == 0,
>+                    "division by zero");
[...]
>+  float scaleX = double(aBgPositioningArea.width) / aRatio.width;
>+  float scaleY = double(aBgPositioningArea.height) / aRatio.height;

Shouldn't that assertion check that *both* components are nonzero?  (since we divide by both of them)  This probably wants to be something like:
  NS_ABORT_IF_FALSE(aRatio.height && aRatio.width, "division by zero");

It looks like you're trying to allow for the (0,0) "no-ratio" case, but I think your code here will currently divide by zero in that case, which is bad.  The assertion in its curreent state also won't catch the (bad) case where aRatio.height is nonzero but aRatio.width is 0.

>+nsSize
>+ImageRenderer::ComputeDrawnSize(const nsStyleBackground::Size& aLayerSize,
>+                                const nsSize& aBgPositioningArea,
>+                                nscoord aUnscaledWidth, PRBool haveWidth,
>+                                nscoord aUnscaledHeight, PRBool haveHeight,
>+                                const nsSize& ratio)

s/ratio/aRatio/ (or aIntrinsicRatio, per above comment on the decl of this function)

>+  NS_ABORT_IF_FALSE(ratio.height != 0 || ratio.width == 0,
>+                    "division by zero");

Like in ComputeContainCoverSizeFromRatio, this assertion needs to be broadened to at least catch the nonzero-height-but-zero-width error case.

Perhaps shift the "haveRatio" decl up above this, and then change the assertion to:
  NS_ABORT_IF_FALSE(!haveRatio ||
                    (aRatio.height && aRatio.width), "division by zero");

>+  PRBool autoWidth = aLayerSize.mWidthType == nsStyleBackground::Size::eAuto;

Nit: perhaps "isAutoWidth"? (There are lots of widths & heights & sizes in play here, and it's nice to keep track of the fact that this particular variable is a flag (starting with "is") rather than an actual width)

>+  // Hardest case: only one auto.  Prepare to negotiate amongst intrinsic
[...]
>+  if (haveWidth && haveHeight) {
>+    // Use the provided dimension and compute the other from the ratio.
>+    NS_ABORT_IF_FALSE(haveRatio, "handled much earlier");

Nit: Maybe "handled the no-ratio case much earlier" (or "no-ratio have-both-dimensions case")

Also, is this assertion actually valid? (did you already handle that case?) From a quick glance at the code above it, I don't immediately see where it's handled...  (and there are cases allowing for haveRatio to be either true or false in the clauses above)

>+   * value for that dimension using the intrinsic ratio (if available).  If
>+   * there's an intrinsic ratio it will be returned; otherwise (0, 0) will be
>+   * returned.
[...]
>   static void ComputeSizeForDrawing(imgIContainer* aImage,

Nit: "(0, 0) will be returned" is a bit vague -- maybe replace with:
  "otherwise, the aIntrinsicRatio outparam will be set to (0, 0)".


>+++ b/layout/style/nsStyleStruct.cpp
>+  NS_ABORT_IF_FALSE(type == eStyleImageType_Image, "missed an enum value");
>+
>+  nsCOMPtr<imgIContainer> imgContainer;
>+  aImage.GetImageData()->GetImage(getter_AddRefs(imgContainer));
>+  if (imgContainer) {

I think we need a stronger (opt-build) check on "type == eStyleImageType_Image" here.  If in the future we add another enum value and neglect to update this code, we'll potentially be opening up a random-value-being-used-as-a-pointer bug here. (since GetImageData() just grabs a pointer out of a union)

Could you wrap this in "if (type == eStyleImageType_Image)", and then add use NS_NOTREACHED (or NS_ABORT_IF_FALSE(false)) for the "missed an enum value" case?

Or equivalently, add an early-return clause for the non-eStyleImageType_Image case.

Comment 29

[Jeff Walden [:Waldo]]

Working through the comment, but keep in mind that the situations where ComputeContainCoverSizeFromRatio is called are highly constrained.  While it's possible in theory that someone might pass in a parameter which triggers division by zero, I suspect those cases are precluded by the precise logic executed before the CCCSFR (gesundheit) call.  Images with zero width or height definitely trigger shortcuts in various places, and I think they might cover these concerns.

Or they might not, of course.  It shouldn't be hard to write some exhaustive tests for cases where one dimension or the other of the image is zero.  (Reference image will be simple!  ;-) )

Comment 30

[Jeff Walden [:Waldo]]

Attachment: Updated through comment 28

(In reply to comment #28)
> Also, it might be worth renaming "aRatio" to "aIntrinsicRatio", for
> consistency with other functions (e.g. nsLayoutUtils::ComputeSizeForDrawing,
> which your patch adds "aIntrinsicRatio" to).

I've become too used to JS's 99-character line limit, 80 feels confining now.  :-\  But this seems reasonable on its own merits, even accepting the closer-to-80 costs.

> >+static nsSize
> >+ComputeContainCoverSizeFromRatio(const nsSize& aBgPositioningArea,
> >+                                 const nsSize& aRatio, FitType fitType)
> >+{
> >+  NS_ABORT_IF_FALSE(aRatio.height != 0 || aRatio.width == 0,
> >+                    "division by zero");
> [...]
> >+  float scaleX = double(aBgPositioningArea.width) / aRatio.width;
> >+  float scaleY = double(aBgPositioningArea.height) / aRatio.height;
> 
> Shouldn't that assertion check that *both* components are nonzero?

Ah, yes -- added two assertions for this.  (Two because then on failure you get finer-grained information about the mode of failure, rather than having to guess which one was at fault.)

> It looks like you're trying to allow for the (0,0) "no-ratio" case, but I
> think your code here will currently divide by zero in that case, which is
> bad.  The assertion in its curreent state also won't catch the (bad) case
> where aRatio.height is nonzero but aRatio.width is 0.

Looking closer, I think you're on to something here.  One call to CCSFR always passes aIntrinsicRatio in a place where we have a ratio, so there's no danger there.  The other call passes aIntrinsicRatio in one place (safe), passes the image size in another (safe due to the early-return-if-empty-image at top), and passes scaled image dimensions in another (seemingly not safe, if scaling underflows).

But aIntrinsicRatio's not actually safe, because I didn't think about the possibility that the viewBox ratio might itself include a zero component!  I'd thought the implementation guaranteed that wouldn't happen, but it doesn't.  Added a check for having a ratio but having a zero component early on -- I think that should address everything in the method that uses the ratio and depends on it having non-zero components.

> Nit: perhaps "isAutoWidth"? (There are lots of widths & heights & sizes in
> play here, and it's nice to keep track of the fact that this particular
> variable is a flag (starting with "is") rather than an actual width)

Can't hurt.

> >+  // Hardest case: only one auto.  Prepare to negotiate amongst intrinsic
> [...]
> >+  if (haveWidth && haveHeight) {
> >+    // Use the provided dimension and compute the other from the ratio.
> >+    NS_ABORT_IF_FALSE(haveRatio, "handled much earlier");
> 
> Nit: Maybe "handled the no-ratio case much earlier" (or "no-ratio
> have-both-dimensions case")
> 
> Also, is this assertion actually valid?

It's valid because if the image has a fixed-size width and a fixed-size height, it must have an intrinsic ratio.  (In all the other cases this may not be the case, hence haveRatio isn't constrained.)  But that's not what the message says, is it?  I'll go with "images with width/height must have ratios" to fit the rationale behind it.

> Nit: "(0, 0) will be returned" is a bit vague -- maybe replace with:
>   "otherwise, the aIntrinsicRatio outparam will be set to (0, 0)".

Done, although we use "returned" to describe how we hand results to the caller for XPCOM methods, so it doesn't seem particularly vague to me.

> Could you wrap this in "if (type == eStyleImageType_Image)", and then add
> use NS_NOTREACHED (or NS_ABORT_IF_FALSE(false)) for the "missed an enum
> value" case?

Done.

Comment 31

[Daniel Holbert [:dholbert]]

Comment on attachment 541215 [details] [diff] [review]
Updated through comment 28

>+nsSize
>+ImageRenderer::ComputeDrawnSize(const nsStyleBackground::Size& aLayerSize,
[...]
>+  // If the image has an intrinsic ratio but either component of it is zero,
>+  // then the image would eventually scale to nothingness, so again we can bail.
>+  PRBool haveRatio = aIntrinsicRatio != nsSize(0, 0);
>+  if (haveRatio &&
>+      (aIntrinsicRatio.width == 0 || aIntrinsicRatio.height == 0)) {
>+  }

Looks like this wants to be an early-return, but right now it's just an empty clause.

Comment 32

[Jeff Walden [:Waldo]]

Attachment: Er, actually do the early-zero-ratio check

Guh.

Comment 33

[Daniel Holbert [:dholbert]]

Comment on attachment 541749 [details] [diff] [review]
Er, actually do the early-zero-ratio check

>diff --git a/layout/base/nsCSSRendering.cpp b/layout/base/nsCSSRendering.cpp
>+  void ComputeUnscaledDimensions(const nsSize& aBgPositioningArea,
>+                                 nscoord* aUnscaledWidth, bool& aHaveWidth,
>+                                 nscoord* aUnscaledHeight, bool& aHaveHeight,
>+                                 nsSize& aRatio);

Any reason aUnscaledWidth / aUnscaledHeight need to be pointers rather than references? (Note that the outparams here and in other related functions like  nsLayoutUtils::ComputeSizeForDrawing are references). 

Looks like they should probably be references (both for consistency & to be clear that we don't have to check for / worry about null pointer values).

>   nsLayoutUtils::SurfaceFromElementResult mImageElementSurface;
>   PRBool                    mIsReady;
>-  nsSize                    mSize;
>+  nsSize                    mSize; // unscaled size in app units of the image

Nit: The current patch's text could be interpreted to mean that "app units of the image" is a special type of unit.  Perhaps replace comment with:
                                    // unscaled size of the image, in app units

>+nsSize
>+ImageRenderer::ComputeDrawnSize(const nsStyleBackground::Size& aLayerSize,
[...]
>+  // The harder cases: contain/cover.
>+  if (aLayerSize.mWidthType == nsStyleBackground::Size::eContain ||
>+      aLayerSize.mWidthType == nsStyleBackground::Size::eCover) {
[...]
>+    // Easy case, or a harder case transformed: we have intrinsic dimensions.
>+    return ComputeContainCoverSizeFromRatio(aBgPositioningArea, size, fitType);

It looks like everything we do between the above chunks (in the "[...]") is for the purpose of computing "size", which really just needs to be a *ratio* (for passing to ComputeContainCoverSizeFromRatio).  So everything under "Impute a missing dimension from the available one [and aIntrinsicRatio]" is extra work that we don't actually need to do, because we can just directly use aIntrinsicRatio.

In fact, I think that the [...] and the ComputeContainCoverSizeFromRatio call quoted above can be simplified to the following, or something like it:
  if (haveRatio) {
    return ComputeContainCoverSizeFromRatio(aBgPositioningArea,
                                            aIntrinsicRatio, fitType);
  }
  if (aHaveHeight && aHaveWidth) {
    nsSize ratio = nsSize(aUnscaledWidth, aUnscaledHeight);
    return ComputeContainCoverSizeFromRatio(aBgPositioningArea,
                                            ratio, fitType);
  }
  // Missing at least one dimension & missing intrinsic ratio.
  // Just fill the background area exactly.
  return aBgPositioningArea;

Moreover, I'm not sure you even need the second clause in my suggested code above (for "aHaveHeight && aHaveWidth [but no ratio]").  Elsewhere you assert "images with width/height must have ratios", which makes sense, so maybe that second clause isn't needed?

>+  // Harder case: all-auto.
[...]
>+      // As above, use the ratio as dimensions.
>+      return ComputeContainCoverSizeFromRatio(aBgPositioningArea,
>+                                              aIntrinsicRatio, CONTAIN);

Assuming you agree with my previous comment, "as above" is no longer quite right here (my previous comment replaces the code that "as above" refers to).

Also, I don't think "use the ratio as dimensions" makes sense here. You're not using the ratio as dimensions.  You're using it as a ratio. :)

>+    if (haveRatio) {
>+      // Resolve missing dimensions using the intrinsic ratio first.

Drop the word "first".  (It's not "first", it's the only thing we do in this clause. Then we return. :))

>+    // Without a ratio we must fall back to the relevant dimension of the
>+    // area to determine the missing dimension.
>+    return aHaveWidth ? nsSize(aUnscaledWidth, aBgPositioningArea.height)
>+                     : nsSize(aBgPositioningArea.width, aUnscaledHeight);

Indentation is weird here -- looks like you were trying to align the "nsSize()" constructors, and then you added the "a" prefix to aHaveWidth and misaligned them.

>+    if (isAutoWidth) {
>+      size.height = aLayerSize.ResolveHeightLengthPercentage(aBgPositioningArea);
>+      size.width =
>+        NSCoordSaturatingMultiply(size.height,
>+                                  double(aIntrinsicRatio.width) /
>+                                  aIntrinsicRatio.height);

Nit: Elsewhere (in ComputeContainCoverSizeFromRatio) you use NSCoordSaturatingNonnegativeMultiply for a similar computation.  We probably want the "Nonnegative" version here, too, right? (and also in other similar computations where we compute one dimension from the other -- since we know that the ratio & dimensions are non-negative at all these places)

>+  if (!aHaveWidth && !aHaveHeight) {
>+    // Preserve an intrinsic ratio with respect to the CSS-specified dimension.
>+    if (haveRatio) {

I think the contents of this clause (16 lines) is exactly the same as the clause right above it (inside of "if (aHaveWidth && aHaveHeight)")

Can we share code better here somehow, either by using a helper-function or combining these clauses?  (see next comment, too)

>+  nsSize size;
>+  if (haveRatio) {
>+    if (isAutoWidth) {

The code inside this clause looks exactly the same as the code referenced above, too.  Share that code here, too?

>+ImageRenderer::ComputeSize(const nsStyleBackground::Size& aLayerSize,
[...]
>+  mSize.width = haveWidth ? unscaledWidth : drawnSize.width;
>+  mSize.height = haveHeight ? unscaledHeight : drawnSize.height;
>+  return drawnSize;

The old version of this method set mSize to the same thing that it returned -- whereas now, mSize and the return value don't necessarily match.

Could you add a comment to briefly explain why? (Perhaps in the header documentation, which currently doesn't mention that we touch mSize at all.)

>+++ b/layout/reftests/backgrounds/gradient/scaled-color-stop-position-ref.html
>@@ -0,0 +1,27 @@
>+<!--
>+     Any copyright is dedicated to the Public Domain.
>+     http://creativecommons.org/licenses/publicdomain/
>+-->

The URL you actually want to in all these tests is:
  http://creativecommons.org/publicdomain/zero/1.0/
See bug 665009 for details.

>+++ b/layout/style/nsStyleStruct.cpp
>+nsStyleBackground::Size::DependsOnFrameSize(const nsStyleImage& aImage) const
>+{
[...]
>+      // If the image doesn't have a width or height, it'll only render with a
>+      // fixed size if it has an intrinsic ratio.
>+      if (!hasWidth && !hasHeight) {
>+        return imageRatio != nsSize(0, 0) &&
>+               (mWidthType == eLengthPercentage ||
>+                mHeightType == eLengthPercentage);
>+      }

// ...and if background-size has a fixed length in either dimension. (right? that's what the eLengthPercentage checks are for?)

>+      // If the image has a ratio, the image renders at fixed size if the size
>+      // isn't fully auto.
>+      if (imageRatio != nsSize(0, 0)) {
>+        return mWidthType != mHeightType;
>+      }

So it's not immediately obvious, but I think this "!=" check is really checking the same thing as the "== eLengthPercentage || etc" check in the previous clause.  Perhaps one of those checks should be changed to match the other, for consistency? (either using "!=" in both spots, or looking for eLengthPercentage in both spots)

Justification: Per your ABORT_IF_FALSE a little higher up, we know that m{Height|Width}Type are either both auto, or else one of them's eLengthPercentage and the other is auto.  Given that restriction, the "== eLengthPercentage" checks in the clause above this are equivalent to checking mWidthType != mHeightType.

r=dholbert with the above addressed.  Sorry to have taken so long on this.

Comment 34

[Jeff Walden [:Waldo]]

Attachment: Updated to comments, plus more bugfixes

Good comments.  I noticed a couple other tweaks to make, and together I think they could use another run-by just to be safe.  Some more non-negative multiplications were pretty easy.  However, I noticed while addressing comments that a couple cases in DependsOnFrameSize had inverted logic, claiming rendering didn't depend when it should have.  And I noticed a couple more places to common up logic in ComputeDrawnSize.  dholbert, mind taking a look at those changes?  (DOFS is particularly worth examining because historically I've had no luck writing tests that failed for bad background-damage reasons and which were fixed when those damage problems were resolved.)

Comment 35

[Daniel Holbert [:dholbert]]

Comment on attachment 542957 [details] [diff] [review]
Updated to comments, plus more bugfixes

(In reply to comment #34)
> dholbert, mind taking a look at those changes?

Just one issue that I noticed (aside from the unrelated-test-file-removal :)) -  I think you might've missed this review comment:
> >+ImageRenderer::ComputeSize(const nsStyleBackground::Size& aLayerSize,
> [...]
> >+  mSize.width = haveWidth ? unscaledWidth : drawnSize.width;
> >+  mSize.height = haveHeight ? unscaledHeight : drawnSize.height;
> >+  return drawnSize;
> 
> The old version of this method set mSize to the same thing that it returned
> -- whereas now, mSize and the return value don't necessarily match.
> 
> Could you add a comment to briefly explain why? (Perhaps in the header
> documentation, which currently doesn't mention that we touch mSize at all.)

r+ with that addressed.

Comment 36

[Jeff Walden [:Waldo]]

Attachment: On to the second reviewer!

Comment 37

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 543259 [details] [diff] [review]
On to the second reviewer!

nsStyleStruct.cpp:

+      // If the image has an intrinsic ratio, rendering will depend on frame
+      // size when background-size is all auto.
+      if (imageRatio != nsSize(0, 0)) {
+        return mWidthType == mHeightType;
+      }

I think you should instead be checking imageRatio.x != 0.0 && imageRatio.y != 0.0 , although I might be missing something?

nsCSSRendering.cpp:

+  switch (fitType) {
+    case COVER:
+      if (scaleX < scaleY) {
+        size.width = NSCoordSaturatingNonnegativeMultiply(aRatio.width, scaleY);
+        size.height = aBgPositioningArea.height;
+      } else {
+        size.width = aBgPositioningArea.width;
+        size.height = NSCoordSaturatingNonnegativeMultiply(aRatio.height, scaleX);
+      }
+      break;
+    case CONTAIN:
+      if (scaleX < scaleY) {
+        size.width = aBgPositioningArea.width;
+        size.height = NSCoordSaturatingNonnegativeMultiply(aRatio.height, scaleX);
+      } else {
+        size.width = NSCoordSaturatingNonnegativeMultiply(aRatio.width, scaleY);
+        size.height = aBgPositioningArea.height;
+      }
+      break;
+  }

I'd actually suggest simplifying this using the test
  if ((scaleX < scaleY) == (fitType == CONTAIN))
which will let you avoid the duplication here.

+  // If the image dimensions and size are complementary, use the complement.
+  nsSize size;
+  if (isAutoWidth && aHaveWidth) {
+    size.width = aUnscaledWidth;
+    size.height = aLayerSize.ResolveHeightLengthPercentage(aBgPositioningArea);
+  } else if (!isAutoWidth && aHaveHeight) {
+    size.width = aLayerSize.ResolveWidthLengthPercentage(aBgPositioningArea);
+    size.height = aUnscaledHeight;
+  } else {
+    // Otherwise use the one specified dimension, and treat the other as 100%.
+    if (isAutoWidth) {
+      size.width = aBgPositioningArea.width;
+      size.height = aLayerSize.ResolveHeightLengthPercentage(aBgPositioningArea);
+    } else {
+      size.width = aLayerSize.ResolveWidthLengthPercentage(aBgPositioningArea);
+      size.height = aBgPositioningArea.height;
+    }
+  }

This might be clearer with the pattern:

  nsSize size;
  if (isAutoWidth) {
    size.width = aHaveWidth ? aUnscaledWidth : aBgPositioningArea.width;
    size.height = aLayerSize.ResolveHeightLengthPercentage(aBgPositioningArea);
  } else {
    // similar
  }

>+/*
>+ * The size returned by this method differs from the value of mSize, which this
>+ * method also computes, in that mSize is the image's "preferred" size for this
>+ * particular rendering, while the size returned here is the actual rendered
>+ * size after accounting for background-size.  The preferred size is most often
>+ * the image's intrinsic dimensions.  But for images with incomplete intrinsic
>+ * dimensions, the preferred size varies, depending on the background
>+ * positioning area, the specified background-size, and the intrinsic ratio and
>+ * dimensions of the image (if it has them).
>+ */

Could you explain a bit better why this distinction is needed?

nsLayoutUtils.cpp:

Does the semantic change to nsLayoutUtils::ComputeSizeForDrawing (not
filling in intrinsic sizes from the intrinsic ratio when there's a ratio
and one size) have an effect on nsLayoutUtils::DrawImage?  It seems like
that could break something.  Have you tested SVG images with a viewBox
and an intrinsic size in one dimension, in an img element... including
making sure they zoom correctly?

(Though roc or dholbert might have a better idea what might go wrong there.)


(Also, I actually prefer pointers rather than references for out params,
since it makes it much clearer at the caller end.  But I'm not going to
make you change it again...)



r=dbaron, though I'm a little suspicious of the nsLayoutUtils.cpp issue.

Comment 38

[Daniel Holbert [:dholbert]]

(In reply to David Baron [:dbaron] from comment #37)
> nsLayoutUtils.cpp:
> 
> Does the semantic change to nsLayoutUtils::ComputeSizeForDrawing (not
> filling in intrinsic sizes from the intrinsic ratio when there's a ratio
> and one size) have an effect on nsLayoutUtils::DrawImage?  It seems like
> that could break something.  Have you tested SVG images with a viewBox
> and an intrinsic size in one dimension, in an img element... including
> making sure they zoom correctly?
> 
> (Though roc or dholbert might have a better idea what might go wrong there.)

Hmm, good point -- I think this might be problematic in the case that dbaron brought up (w/ viewBox and a single dimension), depending on the preserveAspectRatio value on the root <svg> node.

For example, suppose we have preserveAspectRatio="none" (which tells the SVG to zoom its viewBox by different amounts in each dimension, to exactly fit a particular viewport).  Then, I believe current m-c would use the ratio & single dimension to calculate the "implied" other dimension, and we'd end up preserving the ratio in the final rendering, as if the other dimension had been specified.  However, with the attached patch, we'd instead stretch the image to occupy the full size of |aFill| along the missing dimension, I believe.

Comment 39

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #38)
> For example, suppose we have preserveAspectRatio="none" (which tells the SVG
> to zoom its viewBox by different amounts in each dimension, to exactly fit a
> particular viewport).

(er, s/tells the SVG/allows the SVG/)

Comment 40

[Jeff Walden [:Waldo]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/89c17ca55124

For the nsStyleStruct.cpp, since it's an optimization for images with an aspect ratio with a zero component (which will render as empty), it doesn't really matter which check is used, I think.

I fixed the nsLayoutUtils issue but didn't add a test for it.  It turns out DrawImage is only used for border-image now, and between the syntax there being changed Real Soon Now and my not knowing either old or new syntax, it seemed more trouble than it was worth.  I got dbaron to look at the changes as a double-check, and that should be good enough for now.  Longer-term, someone should make sure every place where an image is drawn in this stuff does the proper object-sizing stuff, and tests for border-image with vector images should happen then.

I'll write up a blog post about this sometime soon, probably within the next week or so, hopefully.

Comment 41

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/89c17ca55124

Comment 42

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Tests pushed to CSS test suite repo too:
https://hg.csswg.org/test/rev/ad613745367f

Comment 43

[Jeff Walden [:Waldo]]

I updated https://developer.mozilla.org/en/CSS/background-size for the semantics here, and I specifically called out the change in gradient rendering when a background-size is specified.  I also added a note that it's best to only completely specify the size for a gradient (that is, don't include an auto component in such a size), because rendering will be inconsistent until browsers all implement the specs here.

As the peanut gallery in #devmo notes, this could use some diagrams/examples for vector images missing various bits of dimensions and proportion.  I can write some examples, and probably will in order to blog about this.  I might not be the best person to draw up diagrams, as that sort of graphical work isn't really my shtick.  We'll see how far I get -- if I end up doing all that I'll consider this documented (but still in need of notes in "new in Firefox 8" pages), otherwise I'll leave it for someone to clean up the trailing bits.

Comment 44

[Eric Shepherd [:sheppy]]

(In reply to Jeff Walden (remove +bmo to email) from comment #43)
> As the peanut gallery in #devmo notes, this could use some diagrams/examples
> for vector images missing various bits of dimensions and proportion.  I can
> write some examples, and probably will in order to blog about this.  I might
> not be the best person to draw up diagrams, as that sort of graphical work
> isn't really my shtick.  We'll see how far I get -- if I end up doing all
> that I'll consider this documented (but still in need of notes in "new in
> Firefox 8" pages), otherwise I'll leave it for someone to clean up the
> trailing bits.

If you write some examples, I can use that to give me a clue as to making the diagrams you mention needing. In the meantime, I've added a "NeedsDiagrams" tag to the background-size page.

Comment 45

[Jeff Walden [:Waldo]]

I finally finished that blog post:

http://whereswalden.com/2011/10/21/properly-resizing-vector-image-backgrounds/

Comment 46

[Eric Shepherd [:sheppy]]

I've converted that blog post into an article on MDN. Hopefully I didn't screw anything up in the transition; a quick review would be appreciated.

This is linked to from the background-size page and from Firefox 8 for developers.

https://developer.mozilla.org/en/CSS/Scaling_of_SVG_backgrounds