Closed Bug 619500 Opened 14 years ago Closed 8 years ago

SVG as border-image does not scale to element

Categories

(Core :: SVG, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 + fixed
firefox49 --- fixed

People

(Reporter: jykng, Assigned: u459114)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete, Whiteboard: [parity-Opera][parity-webkit])

Attachments

(12 files, 7 obsolete files)

407 bytes, image/svg+xml
Details
430 bytes, image/svg+xml
Details
430 bytes, image/svg+xml
Details
432 bytes, image/svg+xml
Details
58 bytes, text/x-review-board-request
seth
: review+
Details
58 bytes, text/x-review-board-request
dholbert
: review+
Details
58 bytes, text/x-review-board-request
dholbert
: review+
Details
58 bytes, text/x-review-board-request
dholbert
: review+
Details
8.22 KB, image/png
Details
14.85 KB, text/html
Details
3.69 KB, patch
Details | Diff | Splinter Review
25.61 KB, patch
Details | Diff | Splinter Review
User-Agent:       Mozilla/5.0 (Windows NT 5.1; rv:2.0b7) Gecko/20100101 Firefox/4.0b7
Build Identifier: Mozilla/5.0 (Windows NT 5.1; rv:2.0b7) Gecko/20100101 Firefox/4.0b7

When using an SVG image as the url of a border-image, it doesn't scale to the size of the element it's being used on.  

I've tried different case here on an element that has border-width:30px and stretch for both the horizontal and vertical.  I've tried with slices at 50% 50% 50% 50% and 25% 25% 25% 25%.  Using 4 different SVG images of the same things which is 4 coloured circles grouped together, each taking up a quadrant with a radius of 25% the size of the image.  One without height and width given to the svg area.  One that is smaller at 10 pixels, one where the size matches more closely at 60 pixels, and one that's larger than the border-width at 300 pixels.

While this test also does show how the other browsers are flawed, what I'm seeing for the element with the border image in FF4.0, is that it looks completely different for the images of different sizes.  And stretch on a SVG image behaves more like repeat.

Reproducible: Always



Expected Results:  
I'd expect the elements with different sized svg images as border-images to look relatively the same.
Could you attach individual files rather than a zip please?
Attached image SVG no dimensions
Attached image SVG 10 pixels
Attached image SVG 60 pixels
Attached image SVG 300 pixels
Attached file fix links (obsolete) —
Attachment #497914 - Attachment is obsolete: true
Attachment #497924 - Attachment mime type: text/plain → image/svg+xml
Attachment #497924 - Attachment mime type: image/svg+xml → text/html
OS: Windows XP → All
Component: General → SVG
Product: Firefox → Core
QA Contact: general → general
Version: unspecified → Trunk
Status: UNCONFIRMED → NEW
Ever confirmed: true
Hardware: x86 → All
Whiteboard: [parity-Opera][parity-webkit]
There are a number of things causing the brokenness here.  I just talked to roc about this, and he suggested (and I agree) that the best way to fix this is to make our border-image code use gfxDrawables.  Then, we'd allow any gfxDrawable to be used as a border-image.  This will require tweaks in both border-image code and gfxDrawable code, so this should wait until bug 600207 is fixed, since that bug will also change how gfxDrawables work.
Depends on: 600207
Is this why Australis looks bad on HD displays as described on http://www.reddit.com/r/firefox/comments/218dv6/this_new_rounded_tab_interface_looks_crappy_on_hd/ ?
Flags: needinfo?(dholbert)
It's possible, but I doubt it; this bug is specifically about border-image, and the Australis tabs don't use border-image, as far as I know.

I'm told by Australis devs that they use an SVG background-image and SVG clip-paths. References to both are here:  http://mxr.mozilla.org/mozilla-central/source/browser/themes/shared/tabs.inc.css#161
So I don't think border-image issues would impact australis tabs.

If you can reproduce the fuzziness shown in that screenshot, please file a new bug with more information about the hardware that you're using.
Flags: needinfo?(dholbert)
Actually, comment 11 is likely bug 946987, which is in the process of being fixed.
https://twitter.com/kizmarh/status/195977985986273280 seems related, though :seth thinks that what we do there is specific to the SVG not having a viewBox attribute.
Attachment #8395790 - Attachment is obsolete: true
Here’s yet another testcase: http://result.dabblet.com/gist/cb9df5ed96786a6c34d2
Working perfectly in Blink/WebKit.
If you replace viewBox="0 0 4 4" with width="4" height="4" it works fine in Gecko as well.
preserveAspectRatio="none" and removing any dimensions all together let the svg strech to any size.

Logic described here seems to apply - https://developer.mozilla.org/en-US/docs/Web/CSS/Scaling_of_SVG_backgrounds
Bug 1110496 covers a more recent regression that made things worse.
See Also: → 1110496
Something wrong in DrawBorderImage. I am working on it
Assignee: nobody → cku
Two problems:
1. border-style
In firefox, border-style need to be set as solid to let border-image appear; In chrome, border-style can be skipped. According to the spec, the default value of border-style is none, so border image should not appear in this case. I think the behavior of FF is correct.

2. viewport
Rendering result of a border image is not correct if viewport size is not given.(as comment 16)
FF can render svg-as-border image correctly only if
a. there is viewport size attributes, width and height, in SVG.
b. and, no viewBox attribute in SVG
I am working on fixing #2
Let's disregard part (1) for the purposes of this bug -- it's not SVG-image-specific, and as you note, Firefox is correct there.  There's a Chrome issue filed on it, too: https://code.google.com/p/chromium/issues/detail?id=559258
Depends on: 1151016
Attached WIP. It basically works correctly
1. svg no viewport has viewbox - done
2. svg no viewport no viewbox - still need to fix bug 1151016 before getting correct result.
BTW, If you want to have a try on these patches, you still can't see any border image in Attachment 497904 [details].
That's because of part 1 of comment 21. You have to manually set border-style as solid.
Blocks: 1110496
Attachment #8713077 - Attachment is obsolete: true
Attachment #8713079 - Attachment is obsolete: true
Attachment #8714219 - Flags: review?(dholbert)
Attachment #8714220 - Flags: review?(dholbert)
Attachment #8714221 - Flags: review?(dholbert)
Attachment #8714222 - Flags: review?(dholbert)
Hi Daniel, would you mind to have a review on these patches?
Sure! Though, it'll be a few days before I get to them -- I'm behind on my review queue at the moment, and I've got 3 patch-stacks to get through before I look at this one.
Add two more test cases in the 3rd patch
I'm planning on reviewing this today -- sorry for the wait here.
Attachment #8714219 - Flags: review?(dholbert)
Comment on attachment 8714219 [details]
MozReview Request: Bug 619500: Part 1. Default sizing for specified size of SVG images which have no constraints; r=dholbert r=seth

https://reviewboard.mozilla.org/r/32971/#review30771

Seth should probably do the final review on this, if he's available, since he wrote ClippedImage & ImageOps and hence knows that API & the surrounding code better than I do.

However, I did find a number of things to fix here -- could you fix up the following, and then request review from seth? (Or, if he can't review it for whatever reason, I'm happy to do the final review too.)

::: image/ClippedImage.h:67
(Diff revision 2)
> -  ClippedImage(Image* aImage, nsIntRect aClip);
> +  ClippedImage(Image* aImage, nsIntRect aClip, const Maybe<nsSize>& aImageSize = Nothing());

Let's drop the "= Nothing()" default value for this argument here.

I don't think we need it - I'm pretty sure all places that create a ClippedImage should be passing through an |aImageSize| arg. (See also my comment for ImageOps::Clip, which covers one callsite that doesn't pass this arg right now, but should.)

::: image/ClippedImage.h:91
(Diff revision 2)
> -
> +  Maybe<nsIntSize> mImageSize;    // The given size of a vector image, which

A few nits about the mImageSize declaration & documentation here:

(1) The first line ("The given size of a vector image") implies that ClippedImage will always have a vector image, and that's not true. Maybe reword as: "If we're clipping a VectorImage, this is the size of [etc.]"

(2) I'm not clear what "given size" refers to here. Is this supposed to be the size of the viewport that the SVG Image should consider itself as being rendered into? (If so, please reword along those lines.)

(3) This variable's name is probably too vague right now. "mImageSize" sounds like it could be the size of our ClippedImage (but it's not), and it also sounds like it could be the size of our internal image (but it's not really that either).

Maybe rename to "mVectorImageSize" or "mSVGViewportSize" or something along those lines?

::: image/ClippedImage.cpp:146
(Diff revision 2)
> +  MOZ_ASSERT(mImageSize.isNothing() ||

s/mImageSize/aImageSize/ in this assertion.

(mImageSize will always be Nothing at this point; you haven't initialized it yet.)

::: image/ClippedImage.cpp:147
(Diff revision 2)
> +             aImage->GetType() == imgIContainer::TYPE_VECTOR,

I'm not clear where this invariant (that aImageSize is only passed for vector-flavored images) is actually enforced in the client code.

If it's possible to tweak the client code to make it clearer that we're honoring this invariant (with comments or checks or assertions), that might be good to do.

::: image/ClippedImage.cpp:177
(Diff revision 2)
> +                                                           mImageSize->height)));

For both of the nsIntRect constructions here:

Since you've already got a nsIntSize, please use the nsIntRect convenience-constructor that takes an nsIntSize.

I think `nsIntRect(nsIntPoint(0,0), *mImageSize)` should do it.  (No need to query for the width and height off of mImageSize.)

::: image/ClippedImage.cpp:438
(Diff revision 2)
> +    innerSize.height = mImageSize->height;

Can't this just be `innerSize = *mImageSize`?  No need to assign width/height independently.

::: image/ClippedImage.cpp:445
(Diff revision 2)
> +    size.Scale(scaleX, scaleY);

The 7 lines before this point (around "Map the clip and size") seem to be code-duplication with the other "if" clause here.

Can we avoid this code-duplication?

Something like this (untested) (not sure if mozreview will format nicely; sorry if it doesn't):
  bool needScale = false;
  if (mImageSize.isSome()...) \{
    innerSize = \*mImageSize;
    needScale = true;
  \} else if (NS_SUCCEEDED(InnerImage()->GetWidth(&innerSize.width)) && ...) \{
    needScale = true;
  \} else \{
    MOZ_ASSERT(false, ...);
  \}
  if (needScale) \{
    \[code that was duplicated goes here\]
  \}

::: image/ClippedImage.cpp:517
(Diff revision 2)
> +    return mClip.Size() * finalScale;

As above, this seems like a lot of copypasted code. Could we share code better with the other case here?

::: image/ImageOps.h:49
(Diff revision 2)
> +                                              const Maybe<nsSize>& aImageSize = Nothing());

Please make this same change (adding an aImageSize arg) to the other Clip() function here, too (the one that takes an Image pointer).

(That version of the function actually doesn't have any callers, and doesn't seem to have ever had any callers -- I believe it exists for completeness & because it's trivial, and it's meant to be consistent with this imgIContainer version.)

Also: please document what this parameter means (and consider renaming it, for the same reasons described above for ClippedImage).  Be sure to document any invariants that we're requiring for it. (for example, I think we only allow this argument to be specified if the image is a VectorImage, right?)

::: layout/base/nsCSSRendering.cpp:3716
(Diff revision 2)
> +                                          );

Could you split this logic out into a local variable, to reduce the complexity of this DrawBorderImageComponent call?

Also, I'm not clear on what the connection is between the condition here (intrinsicSize.CanComputeConcreteSize()) and the thing that we pass if the condition is true (imageSize)...

Also: note also that, several layers beneath this, we're asserting that this arg must only be non-"Nothing()" if our image has type VectorImage. Do we know that that's the case here? (we don't seem to be explicitly checking it here, but maybe we're guaranteed that it's a vectorimage at this point for some reason...?)

::: layout/base/nsCSSRendering.cpp:5261
(Diff revision 2)
> +                                          const Maybe<nsSize>& aImageSize)

Please update the documentation above this function-definition (which has some text about each of the args), to explain this new arg.

::: layout/base/nsCSSRendering.cpp:5261
(Diff revision 2)
> +                                          const Maybe<nsSize>& aImageSize)

Please update the documentation above this function-definition (which has some text about each of the args), to explain this new arg.
Comment on attachment 8714220 [details]
MozReview Request: Bug 619500: Part 2. When drawing an SVG image as a CSS border-image, use preverveAspectRatio="none"; r=dholbert

https://reviewboard.mozilla.org/r/32973/#review30929

Commit message: "Draw boader image, with viewBox, by preverveAspectRatio=None."
 - Typo: s/boader/border/
 - The commit message doesn't really make sense or explain what's changing. I think (?) you're wanting to say something like:
     If an SVG border-image has a viewBox,
     draw it with preverveAspectRatio="none".
   Please reword to say something like that, to more clearly explain what's changing.

::: image/VectorImage.cpp:832
(Diff revision 2)
> +  if (aSVGContext.isSome() && aFlags & FLAG_ALWAYS_STRETCH &&

Please wrap "aFlags & FLAG_ALWAYS_STRETCH" in parens; otherwise it's easy to get lost, with the "&&" on either side of it.

Also: could you add a comment to explain what you're doing in this clause, & why? It's not clear to me.

::: image/VectorImage.cpp:834
(Diff revision 2)
> +    Maybe<SVGPreserveAspectRatio> aspectRatio = Some(SVGPreserveAspectRatio(SVG_PRESERVEASPECTRATIO_NONE, SVG_MEETORSLICE_UNKNOWN));

This line is much too long; it needs rewrapping.

::: image/VectorImage.cpp:835
(Diff revision 2)
> +    svgContext = Some(SVGImageContext(aSVGContext.ref().GetViewportSize(), aspectRatio));

Use -> instead of .ref().

This line is also too long (like the one above it) -- rewrap, please.

::: image/imgIContainer.idl:195
(Diff revision 2)
> -
> +  const unsigned long FLAG_ALWAYS_STRETCH                  = 0x200;

Please document what this flag means, in the comment above these flag declarations here. (We have documentation for the other flags there.)

Also: please preserve the blank line between these flags and the comment after them. (Right now you're adding this new flag *on* that formerly-blank line, but you should be inserting the new flag *before* the blank line.)

::: layout/base/nsCSSRendering.cpp:5274
(Diff revision 2)
> +    uint32_t flag = ConvertImageRendererToDrawFlags(mFlags) | imgIContainer::FLAG_ALWAYS_STRETCH;

Two things:
 (1) s/flag/drawFlags/  (it should be plural, and also we have multiple flavors of flags in play here, and you want to specify which flavor this is. I'm assuming "draw" is a reasonable description of the flavor, given the name of the ConvertImageRendererToDrawFlags function.)

 (2) Please add a comment here explaining why we're including the extra flag (FLAG_ALWAYS_STRETCH) -- i.e. what we're hoping to achieve by adding it.  (Once you've documented the flag in imgIContainer.idl -- addressing another one of my review comments -- that'll help here. But still worth giving a hint here as well -- in particular, to explain why we use the flag here but not elsewhere.)
Attachment #8714220 - Flags: review?(dholbert)
Attachment #8714221 - Flags: review?(dholbert) → review+
Comment on attachment 8714221 [details]
MozReview Request: Bug 619500: Part 3. svg-as-borderimage test cases; r=dholbert

https://reviewboard.mozilla.org/r/32975/#review30931

r=me on the testcases, with nits addressed:

::: layout/reftests/border-image/svg-as-border-image-1-ref.html:4
(Diff revision 2)
> +<title>test of svg-as-border-image</title>

s/test/reference/ in the title element here.  (because this is the reference case)

Since the 'title' appears in Firefox's titlebar, this makes it easier to tell which is which -- testcase vs. reference case -- when comparing the test to the reference in adjacent tabs.

::: layout/reftests/border-image/svg-as-border-image-1a.html:10
(Diff revision 2)
> +  border: 15px solid;transparent;

Invalid syntax here -- "solid;transparent;"

I think you want to just drop the "transparent;" from the end of the line here?

This applies to all of the testcases here (though not the reference case).

::: layout/reftests/border-image/svg-as-border-image-1a.html:11
(Diff revision 2)
> +  border-image: 25% url('data:image/svg+xml,<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 40 40" ><rect x="0" y="0" width="10" height="10" fill="red"/><rect x="30" y="30" width="10" height="10" fill="green"/></svg>');

Please replace "red" with some other color. (maybe "pink")

In some test suites that we use/overlap with, there's a convention that "any red means the test has failed":
https://www.w3.org/Style/CSS/Test/guidelines.html#red
 
So, it's best to avoid using red as a normal part of your test, except to indicate failure.

(This applies to all of the testcases & the reference case, of course. Search-and-replace in the patch file might be a quick & easy way to fix this.)

::: layout/reftests/border-image/svg-as-border-image-1b.html:18
(Diff revision 2)
> +</html>

Nit: add a newline at the end of this file.  (The 1-ref and 1a files have one;  the 1b and 1c files do not.

(This is a general Mozilla coding style thing, for consistency & to avoid patches that end up having hardcoded text "no newline at end of file" in them -- see https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#General_C.2FC.2B.2B_Practices )

::: layout/reftests/border-image/svg-as-border-image-1c.html:1
(Diff revision 2)
> +

Nit: drop blank line from the beginning of this file

::: layout/reftests/border-image/svg-as-border-image-1c.html:19
(Diff revision 2)
> +</html>

As above, add a newline at the end of this file.
Comment on attachment 8714222 [details]
MozReview Request: Bug 619500: Part 4. Remove one unused data member in SVGDrawingParameters; r=dholbert

https://reviewboard.mozilla.org/r/32977/#review30939

r=me on part 4, though perhaps s/redundant/unused/ in the commit message.

(The word "redundant" implies (to me at least) that this variable *is* used, and that its usages can simply be removed & replaced with other variables that have the same information. But really, this param is just completely unused right now.)

(Side note: looks like the last usage of this variable was removed here: http://hg.mozilla.org/mozilla-central/diff/3bfa3ac98ba1/image/src/VectorImage.cpp#l1.106 )
Attachment #8714222 - Flags: review?(dholbert) → review+
One general note -- consider adding "part 1", "part 2", etc. to your commit messages here. (Not everyone does that, but I think it's a good best-practice -- otherwise, someone who's looking at one of your changesets in isolation -- e.g. your fourth one with commit message...
> Bug 619500: Remove one redundant data member in SVGDrawingParameters.
...would get the mistaken impression that this changeset was the only thing that was being done in this bug.)
Daniel, thank for your detailed review, I will go back and address issues here after fix bug 1245499
https://reviewboard.mozilla.org/r/32971/#review30771

> Let's drop the "= Nothing()" default value for this argument here.
> 
> I don't think we need it - I'm pretty sure all places that create a ClippedImage should be passing through an |aImageSize| arg. (See also my comment for ImageOps::Clip, which covers one callsite that doesn't pass this arg right now, but should.)

If that's the case, should we just replace Maybe<nsSize>& argument type by nsSize&?
https://reviewboard.mozilla.org/r/32971/#review30771

> If that's the case, should we just replace Maybe<nsSize>& argument type by nsSize&?

In my original design, a client will pass this parameter only if the image to be clipped has no intrinsic size, so that the client need to give a determined size to ClippedImage. In border-image case, determined size comes from "default size algorithm".
https://reviewboard.mozilla.org/r/32971/#review30771

> Could you split this logic out into a local variable, to reduce the complexity of this DrawBorderImageComponent call?
> 
> Also, I'm not clear on what the connection is between the condition here (intrinsicSize.CanComputeConcreteSize()) and the thing that we pass if the condition is true (imageSize)...
> 
> Also: note also that, several layers beneath this, we're asserting that this arg must only be non-"Nothing()" if our image has type VectorImage. Do we know that that's the case here? (we don't seem to be explicitly checking it here, but maybe we're guaranteed that it's a vectorimage at this point for some reason...?)

1. intrinsicSize.CanComputeConcreteSize() return true means the border image has intrisic size(raster image/ vector image with viewport size/ vector image with viewBox)
2. intrinsicSize.CanComputeConcreteSize() return false means the border image has no-intrisic size(a vector image which has no viewport and viewbox)

Case #1, we don't need to pass image size, ClippedImage can get it from that border-image directly.
Case #2, ClippedImage can not read size information from that border image. In this case, we need to tell ClippedImage how big we want the border-image is by default size algorithm(https://www.w3.org/TR/css3-images/#default-sizing-algorithm) in nsImageRenderer::ComputeConcreteSize. And that's the reason why gecko always rendering svg-border-image without viewport attribue incorrect.
https://reviewboard.mozilla.org/r/32971/#review30771

> Please make this same change (adding an aImageSize arg) to the other Clip() function here, too (the one that takes an Image pointer).
> 
> (That version of the function actually doesn't have any callers, and doesn't seem to have ever had any callers -- I believe it exists for completeness & because it's trivial, and it's meant to be consistent with this imgIContainer version.)
> 
> Also: please document what this parameter means (and consider renaming it, for the same reasons described above for ClippedImage).  Be sure to document any invariants that we're requiring for it. (for example, I think we only allow this argument to be specified if the image is a VectorImage, right?)

The aImageSize parameter tells ClippedImage: hey ClippedImage, since there is no size information in the inner image that I pass to you. So I give you the size of it by aImageSize. If I pass Nothing(), that means the inner image has intrinsic size, just read it directly.
https://reviewboard.mozilla.org/r/32971/#review30771

> I'm not clear where this invariant (that aImageSize is only passed for vector-flavored images) is actually enforced in the client code.
> 
> If it's possible to tweak the client code to make it clearer that we're honoring this invariant (with comments or checks or assertions), that might be good to do.

A client should pass None() if aImage is a raster image.
A client can pass None() or Some() value if aImage is a vector image.

Considerating a vector image may not have viewport/viewbox defined, we need aImageSize to tell ClippedImage the size of it.
For a raster image, it should always has an intrinsic size(unless it's broken) and does not need to pass aImageSize at all.
The assetion here is try to tell the client this logic
Comment on attachment 8714219 [details]
MozReview Request: Bug 619500: Part 1. Default sizing for specified size of SVG images which have no constraints; r=dholbert r=seth

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32971/diff/2-3/
Attachment #8714219 - Attachment description: MozReview Request: Bug 619500: Default sizing for specified size of SVG images which have no constraints. → MozReview Request: Bug 619500: Part 1. Default sizing for specified size of SVG images which have no constraints.
Attachment #8714219 - Flags: review?(dholbert)
Comment on attachment 8714220 [details]
MozReview Request: Bug 619500: Part 2. When drawing an SVG image as a CSS border-image, use preverveAspectRatio="none"; r=dholbert

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32973/diff/2-3/
Attachment #8714220 - Attachment description: MozReview Request: Bug 619500: Draw boader image, with viewBox, by preverveAspectRatio=None. → MozReview Request: Bug 619500: Part 2. Draw boader image, with viewBox, by preverveAspectRatio=None.
Attachment #8714220 - Flags: review?(dholbert)
Attachment #8714222 - Attachment description: MozReview Request: Bug 619500: Remove one redundant data member in SVGDrawingParameters. → MozReview Request: Bug 619500: Part 4. Remove one unused data member in SVGDrawingParameters.
Attachment #8714220 - Attachment description: MozReview Request: Bug 619500: Part 2. Draw boader image, with viewBox, by preverveAspectRatio=None. → MozReview Request: Bug 619500: Part 2. If an SVG border-image has a viewBox, draw it with preverveAspectRatio="none";
(In reply to C.J. Ku[:cjku] from comment #48)
> > Also, I'm not clear on what the connection is between the condition here (intrinsicSize.CanComputeConcreteSize()) and the thing that we pass if the condition is true (imageSize)...
> > 
> > Also: note also that, several layers beneath this, we're asserting that this arg must only be non-"Nothing()" if our image has type VectorImage. Do we know that that's the case here? (we don't seem to be explicitly checking it here, but maybe we're guaranteed that it's a vectorimage at this point for some reason...?)
> 
> 1. intrinsicSize.CanComputeConcreteSize() return true means the border image
> has intrisic size(raster image/ vector image with viewport size/ vector
> image with viewBox)
> 2. intrinsicSize.CanComputeConcreteSize() return false means the border
> image has no-intrisic size(a vector image which has no viewport and viewbox)
> 
> Case #1, we don't need to pass image size, ClippedImage [etc]

Thank you for explaining this here -- please explain it in the code as well, so that this is clearer there.
Comment on attachment 8714219 [details]
MozReview Request: Bug 619500: Part 1. Default sizing for specified size of SVG images which have no constraints; r=dholbert r=seth

https://reviewboard.mozilla.org/r/32971/#review33521

Per beginning of comment 40, Seth should really do the final review on this, since he knows ClippedImage better and has this image-viewport/sizing stuff fresher in his mind than I do.  I'm not reviewing the ClippedImage code in too much detail because I'm assuming he'll be better able to do that.

Hence, I'm not ticking "Ship it" -- but here are my second-round review comments (and I anticipate this should be relatively straightforward for seth to review with these addressed):

::: image/ClippedImage.cpp:146
(Diff revisions 2 - 4)
> -  MOZ_ASSERT(mImageSize.isNothing() ||
> +  MOZ_ASSERT(mSVGViewportSize.isNothing() ||

As noted in comment 40, this assertion will trivially be satisfied right now, and that's bad.

This assertion should *really* be checking about the argument (aSVGViewportSize), *not* the member variable (which you haven't actually initialized yet at this point -- mImageSize only gets a value right after this assertion, which means this isNothing() check will always be true; hence the assertion is trivially satisfied).

::: image/ClippedImage.cpp:175
(Diff revisions 2 - 4)
> -                                        mImageSize->height));
> +                                        mSVGViewportSize->height));

You fixed one of the nsIntRects that I mentioned in comment 40 here, but not the other one (the one who's end I'm quoting here, passed to the Intersect function).

It actually looks like you're creating the same nsIntRect twice in a row here.  So, probably better to just create it once, as a local variable, and pass that local variable to both functions -- e.g.:
```
  nsIntRect svgViewportRect(nsIntPoint(0,0),
                            *mSVGViewportSize);
  mClip = mClip.Intersect(svgViewportRect);
    mShouldClip.emplace(!mClip.IsEqualInterior(svgViewportRect);
```

::: image/ClippedImage.cpp:509
(Diff revisions 2 - 4)
> -    // To avoid ugly sampling artifacts, ClippedImage needs the image size to
> +    needScale = true;

Looks like your updated patch removes this comment and the one right after it ("To avoid ugly" & "First, we select").

Is there a reason for that? At first glance, it seems these comments still apply -- the code they document is sticking around.  So I think these comments should still be preserved (alongside the "nsIntSize scale" declaration).

::: image/ImageOps.h:50
(Diff revisions 2 - 4)
> -                                              const Maybe<nsSize>& aImageSize = Nothing());
> +                                              const Maybe<nsSize>& aImageSize);

(1) Please update the documentation (just above this) to mention & explain this new parameter.

(2) This parameter "aImageSize" only has one use -- it gets passed as the arg that's now called "aSVGViewportSize" for ClippedImage.

So maybe it should be named aSVGViewportSize here, too?  (particualrly given that we know we're going to assert several layers below this, if this arg is ever provided for a non-SVG image).

::: layout/base/nsCSSRendering.h:234
(Diff revisions 2 - 4)
> +   * aImageSize the image size evaluated by default sizing algorithm.

Please add a bit more detail here. In particular -- aImageSize is a Maybe<> - explain the significance of a Some() vs. a Nothing() value here.

::: layout/base/nsCSSRendering.cpp:3705
(Diff revisions 2 - 4)
> +                                                  Nothing() : Some(imageSize);

Three nits:

 (1) The indentation on this line (before "Nothing()") seems arbitrary here.  I believe it should only be indented 2 spaces more than the previous line. (aligned with the "y" of "Maybe" on previous line)
 
 (2) Per comment 59, please explain the significance of what you're doing here -- why we're choosing Nothing() vs. Some(imageSize) depending on the condition.

 (3) Since we depend on it several layers below this, it's probably worth adding an assertion here that viewportSize must be Nothing() unless we're TYPE_VECTOR (with a brief assertion message explaining why we know that this will be the case here).
Attachment #8714219 - Flags: review?(dholbert)
Attachment #8714220 - Flags: review?(dholbert) → review+
Comment on attachment 8714220 [details]
MozReview Request: Bug 619500: Part 2. When drawing an SVG image as a CSS border-image, use preverveAspectRatio="none"; r=dholbert

https://reviewboard.mozilla.org/r/32973/#review33537

I'll sign off on part 2 -- r=me with review comments addressed. (I don't think this part needs a second look from Seth)

Firstly, the commit message:
> Part 2. If an SVG border-image has a viewBox, draw it with preverveAspectRatio="none";

I don't actually think we should care if the image has a viewBox.  By definition, preserveAspectRatio only has an effect on drawing if there's a viewBox.

SO: I think we should just drop the mention of viewBox from the commit message, and drop the check from the patch, unless we actually need it for some reason.

So, I think the commit message should be something more like:
 "When drawing an SVG image as a CSS border-image, use preverveAspectRatio="none".

::: image/VectorImage.cpp:831
(Diff revisions 2 - 4)
>    Maybe<SVGImageContext> svgContext = aSVGContext;

This assignment (and the copying that it involves) will have been useless, if we enter the "if" clause below this.

To avoid this waste, please just declare this without assigning it, and then set it to aSVGContext in an "else" clause for the if-condition below this.

So we should end up with:
```
  Maybe<SVGImageContext> svgContext;
  if (...conditions...) {
    [code to set aspectRatio];
    svgContext = Some(...);
  } else {
    svgContext = aSVGContext;
  }
```
with no unnecessary copying.

::: image/VectorImage.cpp:833
(Diff revisions 2 - 4)
> +  // svgPreserveAspectRatio attibute of this image as none, and always stretch

Two comment fixes for this line:
(1) s/svgPreserveAspectRatio/SVG preserveAspectRatio/
(making "SVG" a separate word from "preserveAspectRatio attribute".  The attribute is called "preserveAspectRatio", not "svgPreserveAspectRatio".)

(2) s/as none/with none/  (so you're saying "overwrite...with" instead of "overwrite...as")

::: image/VectorImage.cpp:834
(Diff revisions 2 - 4)
> +  // this image to viewport non-uniformly.

This comment now explains the FLAG_ALWAYS_STRETCH check, but it doesn't explain the rest of this condition. (aSVGContext & HasViewBoxRect).

I *think* we care about aSVGContext because we know that stretching will only usefully happen if we know our SVG viewport size, maybe...?  So maybe add an afterthought to the comment like "(and we can only do this if the caller passed in the SVG viewport, via aSVGContext)"

And as for HasViewBoxRect, I don't actually think we need to check that here. I think it is true that our preserveAspectRatio value will only matter if there's a viewBox -- but there's no harm in tweaking the override here even when there's no viewBox.

So, consider dropping the HasViewBoxRect check here. (or, if we really do need it, please explain it in your code-comment here.)

::: image/imgIContainer.idl:197
(Diff revisions 2 - 4)
>     * This flag is for vector image only. A raster image should ignore this flag

Firstly: this comment should merge with the larger comment above the list of all the flags. See how the flags above this are documented and match that style, please.

Secondly: You're missing period at the end of this line. (You start a new sentence on the next line.)

::: image/imgIContainer.idl:198
(Diff revisions 2 - 4)
>     * While drawing a vecotr image with this flag, do not force uniform scaling

Typo: s/vecotr/vector/

::: image/imgIContainer.idl:199
(Diff revisions 2 - 4)
>     * even if preserveAspectRatio attribute of it is defined as force uniform

Replace this phrase...
"even if preserveAspectRatio attribute of it is defined as force uniform one"
...with this phrase:
"even if its root `<svg>` node has a preserveAspectRatio attribute that would otherwise require uniform scaling"

::: image/imgIContainer.idl:202
(Diff revisions 2 - 4)
>     * box exactly matches the viewport rectangle.

Replace...
 "bounding box"
...with:
 "viewBox (if specified or implied by height/width attrs)"

(The term "bounding box" is incorrect; that sounds to me like the box in which the SVG content draws anything, and that may be smaller or larger than the viewBox.)

::: image/imgIContainer.idl:204
(Diff revisions 2 - 4)
>    const unsigned long FLAG_ALWAYS_STRETCH                  = 0x200;

I wonder if this flag should really be named "FLAG_FORCE_PRESERVEASPECTRATIO_NONE"? Since that's what it does, in practice.

(FLAG_ALWAYS_STRETCH is more concise, but it's easier to misread/misunderstand, and it doesn't sound like it'd be SVG-specific from the name).
Attachment #8714219 - Flags: review?(dholbert)
https://reviewboard.mozilla.org/r/32973/#review34221

Two comment nits from skimming the updated "part 2" briefly -- please fix before landing:

::: image/imgIContainer.idl:188
(Diff revision 6)
> +   * , do not force uniform scaling even if its root <svg> node has a

Nit: Don't begin a line with a comma. Bump the previous word ("flag") down to this line, so that this starts with "flag," instead of ","

::: layout/base/nsCSSRendering.cpp:5285
(Diff revision 6)
> +    // FLAG_FORCE_PRESERVEASPECTRATIO_NONE flag here, tell mImage to ignore

s/tell/to tell/
(or "which tells")
https://reviewboard.mozilla.org/r/32973/#review34223

one more nit (for the line directly after the "tell mImage" line that I commented on above):

::: layout/base/nsCSSRendering.cpp:5286
(Diff revision 6)
> +    // preserveAspectRatio attribute, always do non-uniform stretch.

s/always/and always/
Attachment #8714219 - Flags: review?(dholbert)
Comment on attachment 8714219 [details]
MozReview Request: Bug 619500: Part 1. Default sizing for specified size of SVG images which have no constraints; r=dholbert r=seth

https://reviewboard.mozilla.org/r/32971/#review35247

Looks OK. Lots of style nits, but the actual design looks good.

::: image/ClippedImage.cpp:146
(Diff revision 7)
> +  MOZ_ASSERT(aSVGViewportSize.isNothing() ||

Just use MOZ_ASSERT_IF(aSVGViewportSize, aImage->GetType() == imgIContainer::TYPE_VECTOR).

::: image/ClippedImage.cpp:150
(Diff revision 7)
> +    mSVGViewportSize.emplace(aSVGViewportSize->ToNearestPixels(

Don't use |emplace()| here. Just use |mSVGViewportSize = Some(...)|. Also, instead of |if (aSVGViewportSize.isSome())|, just write |if (aSVGViewportSize)|.

::: image/ClippedImage.cpp:173
(Diff revision 7)
> +    } else if (mSVGViewportSize.isSome() && !mSVGViewportSize->IsEmpty()) {

Replace |mSVGViewportSize.isSome()| with just |mSVGViewportSize|.

::: image/ClippedImage.cpp:441
(Diff revision 7)
> +  if (mSVGViewportSize.isSome() && !mSVGViewportSize->IsEmpty()) {

Get rid of |isSome()| here too.

::: image/ClippedImage.cpp:448
(Diff revision 7)
> +    MOZ_ASSERT(false,

MOZ_ASSERT_UNREACHABLE

::: image/ClippedImage.cpp:506
(Diff revision 7)
> -  if (NS_SUCCEEDED(InnerImage()->GetWidth(&imgWidth)) &&
> +  bool    needScale = false;

Don't align |needScale| here.

::: image/ClippedImage.cpp:507
(Diff revision 7)
> +  if (mSVGViewportSize.isSome() && !mSVGViewportSize->IsEmpty()) {

Remove |isSome()|.

::: image/ImageOps.h:50
(Diff revision 7)
> +                                      const Maybe<nsSize>& aSVGViewportSize);

Maybe just make |aSVGViewportSize| default to |Nothing()| so you don't have to update half the callsites.

::: image/ImageOps.h:53
(Diff revision 7)
> +                                              const Maybe<nsSize>& aSVGViewportSize);

Same here.

::: layout/base/nsCSSRendering.cpp:3724
(Diff revision 7)
> +                                          );

Put |);| on the previous line.

::: layout/base/nsCSSRendering.cpp:5311
(Diff revision 7)
> +    MOZ_ASSERT(aSVGViewportSize.isNothing() ||

MOZ_ASSERT_IF
Attachment #8714219 - Flags: review?(seth) → review+
Attachment #8714219 - Flags: review?(dholbert)
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d03f2103664bbc4b024fe73213883f32afe8535
Bug 619500: Part 1. Default sizing for specified size of SVG images which have no constraints; r=dholbert r=seth

https://hg.mozilla.org/integration/mozilla-inbound/rev/055cc694bc2635b4c2cfcc94945cee1b8a6d1f77
Bug 619500: Part 2. When drawing an SVG image as a CSS border-image, use preverveAspectRatio="none"; r=dholbert

https://hg.mozilla.org/integration/mozilla-inbound/rev/ada8d43117654c9164a437f484ea7c9db901225b
Bug 619500: Part 3. svg-as-borderimage test cases; r=dholbert

https://hg.mozilla.org/integration/mozilla-inbound/rev/bc277a82a03e68ef27dbade4b9e5efcfa5b4e911
Bug 619500: Part 4. Remove one unused data member in SVGDrawingParameters; r=dholbert
Attached file test case
Testing this on Firefox 48.0a1 (2016-04-14) the results of the test case look like expected for the 50% slices now. I.e. you see a differently colored circle on each corner.

Though the ones using 25% slices look broken. It looks like the stretching is done preserving the aspect ratio of each of the 1/4 parts, which is wrong.
This gets even worse when zooming the page.
The expected display would stretch the two horizontal edge slices only horizontally and the vertical edge slices only vertically.

I have added a PNG file as reference border image.

Sebastian
Attachment #497904 - Attachment is obsolete: true
Attachment #497924 - Attachment is obsolete: true
Hi Sebastian, please file a new bug blocking this one: https://bugzilla.mozilla.org/enter_bug.cgi?format=__default__&product=Core&cloned_bug_id=619500

Once a bug is resolved, all new issues need to go in new bugs to make them easier to track.
Indeed, a followup bug would be great for tracking that.  Mind filing? (This bug is already quite long -- both temporally and in number-of-comments -- and it's had code land to fix at least one large part of the issue.  So, I suspect this isn't the most productive place for further investigation/patching.)

(Big thanks for noticing the remaining issue -- and I do agree with you, the "size 300 SVG 25% slices" pieces of your testcase does definitely seem broken, as compared to the PNG of the same size and as compared to Chrome.)
(In reply to Sebastian Zartner [:sebo] from comment #85)
> Created attachment 8741548 [details]
> test case
> 
> Testing this on Firefox 48.0a1 (2016-04-14) the results of the test case
> look like expected for the 50% slices now. I.e. you see a differently
> colored circle on each corner.
> 
> Though the ones using 25% slices look broken. It looks like the stretching
> is done preserving the aspect ratio of each of the 1/4 parts, which is wrong.
> This gets even worse when zooming the page.
> The expected display would stretch the two horizontal edge slices only
> horizontally and the vertical edge slices only vertically.
> 
> I have added a PNG file as reference border image.
> 
> Sebastian

I think it's relative to
https://bugzilla.mozilla.org/show_bug.cgi?id=1151016#c5
No longer blocks: 1264809
Depends on: 1264809
Added a compatibility footnote about this at https://developer.mozilla.org/en-US/docs/Web/CSS/border-image.

Sebastian
Depends on: 1269528
Depends on: 1285320
[Tracking Requested - why for this release]:
This bug introduced a regression on bug 1269528(seen on FF48 & FF49) which had been fixed by bug 1264809(fixed on FF50). Patches in bug 1264809 will be uplifted to aurora so that we are able to provide better SVG-as-border-image support from FF49.

For regression in FF48, per discussion with CJ, Kevin, RM:Gerry, we will back out the patch in FF48.
CJ, please help to prepare the patch and uplift to beta. Thanks.
Flags: needinfo?(cku)
Per comment #90, set flag to affected for 48.
Flags: needinfo?(cku)
Comment on attachment 8770806 [details] [diff] [review]
Disable svg-as-borderimage in FF48

Approval Request Comment
[Feature/regressing bug #]:619500
[User impact if declined]: This patch temporary switch off svg border image viewbox support to prevent regression on bk.com(Bug 1269528).
[Describe test coverage new/current, TreeHerder]: manual test + try
[Risks and why]: low risk.
[String/UUID change made/needed]: none
Attachment #8770806 - Flags: approval-mozilla-beta?
How confident are you that this reverts us to the behavior that ships in 47?
Flags: needinfo?(cku)
I am very sure the behavior of 47 and 48 will be the same after apply this patch(verified by test cases). But if you worry about it, I can simply revert all check-in code.
Flags: needinfo?(cku)
Attachment #8770806 - Flags: approval-mozilla-beta?
Attachment #8770854 - Attachment is obsolete: true
Comment on attachment 8770856 [details] [diff] [review]
Revert svg-as-borderimage with viewbox support in gecko 48

Approval Request Comment
[Feature/regressing bug #]: bug 619500
[User impact if declined]: This patch revert svg-as-borderimage viewbox support to prevent regression on bk.com(Bug 1269528).
[Describe test coverage new/current, TreeHerder]: manual test + try
[Risks and why]: low risk.
[String/UUID change made/needed]: none
Attachment #8770856 - Flags: approval-mozilla-beta?
Comment on attachment 8770856 [details] [diff] [review]
Revert svg-as-borderimage with viewbox support in gecko 48

Further backout for beta to revert to previous svg behavior.
Attachment #8770856 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
I've updated the compatibility note at https://developer.mozilla.org/en-US/docs/Web/CSS/border-image and also added it to https://developer.mozilla.org/en-US/docs/Web/CSS/border-image-slice now in regard of this and the related bugs.

C.J. can you please check if the information is correct and complete?

Sebastian
Flags: needinfo?(cku)
LGTM. 
1. example in codepen & jsfiddle is not work.
2. You mentioned 4-value syntax of CSS in border-image-slice. It may useful to link with
https://developer.mozilla.org/en-US/docs/Web/CSS/Shorthand_properties#Tricky_edge_cases
where explain 1-to-4-value syntax by figures.
Flags: needinfo?(cku)
(In reply to C.J. Ku[:cjku](UTC+8) from comment #105)
> LGTM. 
> 1. example in codepen & jsfiddle is not work.

Fixed that by using absolute URLs. Thanks for the hint!

> 2. You mentioned 4-value syntax of CSS in border-image-slice. It may useful
> to link with
> https://developer.mozilla.org/en-US/docs/Web/CSS/
> Shorthand_properties#Tricky_edge_cases
> where explain 1-to-4-value syntax by figures.

It's not 100% correct, because border-image-slice is currently a longhand property, but it may help to understand the syntax, so I've added a 'See also' section with the link.

Sebastian
C.J., please post a followup patch to document what FLAG_FORCE_UNIFORM_SCALING means in imgIContainer.idl and (sorry Daniel! =) ask dholbert to review it. I'm sorry to be a stickler about this, but I really don't want flags with no documentation in the tree.
Flags: needinfo?(cku)
It should be a follow up patch of bug 1264809 Part2. Left a comment in that bug already.
Flags: needinfo?(cku)
Thank you C.J.!
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: