1264809 - SVG as border-image incorrectly stretches on the edges

Status: VERIFIED FIXED

(Core :: SVG, defect)

Description

[Sebastian Zartner [:sebo]]

+++ This bug was initially created as a clone of Bug 619500 comment 85 +++

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

Comment 1

[Sebastian Zartner [:sebo]]

Attachment: SVG with no dimensions

Comment 2

[Sebastian Zartner [:sebo]]

Attachment: SVG 10 pixels

Comment 3

[Sebastian Zartner [:sebo]]

Attachment: SVG 60 pixels

Comment 4

[Sebastian Zartner [:sebo]]

Attachment: SVG 300 pixels

Comment 5

[Sebastian Zartner [:sebo]]

Attachment: PNG 300 pixels

Comment 6

[Sebastian Zartner [:sebo]]

Attachment: test case

Comment 7

[u459114]

Kevin, please check https://bugzilla.mozilla.org/show_bug.cgi?id=1151016#c5

Comment 8

[Kevin Chen[:kechen] (UTC + 8)]

Attachment: Prevent reseting preserveAspectRatio for SVG with valid viewport size or aspect-ratio

The reason that border-image broken is due to the reset of preserveAspectRatio  for SVG with valid viewport size or aspect-ratio when drawing the image[1], this patch can prevent this reset.
But I am still studying about the spec to make sure if this behavior is correct.

[1] https://dxr.mozilla.org/mozilla-central/source/image/VectorImage.cpp#840

Comment 9

[Kevin Chen[:kechen] (UTC + 8)]

Attachment: PNG image with 300/150 w/h

Comment 10

[Kevin Chen[:kechen] (UTC + 8)]

Attachment: SVG 300 pixels with 300/150 viewport size

Comment 11

[Kevin Chen[:kechen] (UTC + 8)]

Hi Sebastian Zartner,

Though the current border-image with 25% seems broken, IMHO, it might be the correct behavior, the reason is described in following paragraph :

According the link of steps of drawing border-image in [1], taking the top and bottom edge for example, it will take two steps to scale the SVG image to target border's size.

Step 1 :
"The two images for the top and bottom edges are made as tall as the top and bottom border image area parts, respectively, and their width is scaled proportionally."

Step 2 :
"If the first keyword is ‘stretch’, the top, middle and bottom images are further scaled to be as wide as the middle part of the border image area. The height is not changed any further."

For the word "scale" in description above, I am not sure what it actually means, but if it means "change of viewport size"; Then, in the above SVG image(like attachment 8741555 [details]), it uses percentage as the value of circle's radius which will change dynamically while scaling according to SVG's spec[2] (the function will be "qrt((actual-width)**2 + (actual-height)**2))/sqrt(2)" ).

In that case, when resizing the SVG's subimage to fit the target border edge, we might expect it displays like attachment 8756720 [details], however it will actually displays like attachment 8756721 [details], and shows the broken border.

Is my description correct? Or are there anything I miss? Can you give me some advices?

Thanks!


[1] https://www.w3.org/TR/css3-background/#border-image-process
[2] https://www.w3.org/TR/SVG/coords.html#Units

Comment 12

[Sebastian Zartner [:sebo]]

(In reply to Kevin Chen[:kechen] from comment #11)
> Hi Sebastian Zartner,
> 
> Though the current border-image with 25% seems broken, IMHO, it might be the
> correct behavior, the reason is described in following paragraph :
> 
> According the link of steps of drawing border-image in [1], taking the top
> and bottom edge for example, it will take two steps to scale the SVG image
> to target border's size.

I had to reread the spec. a few times, but here is how I interpret the steps:

> Step 1 :
> "The two images for the top and bottom edges are made as tall as the top and
> bottom border image area parts, respectively, and their width is scaled
> proportionally."

This scales the width and height of the border image part proportionally until its height fills the border image area.

Applied to our 10px SVG with the 25% slices and the 30px border this means for the top border image area: Scale the 5px x 2.5px top border image part up to 60px x 30px (proportionally).

> Step 2 :
> "If the first keyword is ‘stretch’, the top, middle and bottom images are
> further scaled to be as wide as the middle part of the border image area.
> The height is not changed any further."

Applied to our example this means to resize the width of the 60px x 30px border image part, so that it fills the width of the top border image area.

> For the word "scale" in description above, I am not sure what it actually
> means, but if it means "change of viewport size"; Then, in the above SVG
> image(like attachment 8741555 [details]), it uses percentage as the value of
> circle's radius which will change dynamically while scaling according to
> SVG's spec[2] (the function will be "qrt((actual-width)**2 +
> (actual-height)**2))/sqrt(2)" ).

To my understanding 'scale' means resizing the image dimensions. There is no mention of the image's viewport and other image formats also don't have a viewport.

Sebastian

Comment 13

[Kevin Chen[:kechen] (UTC + 8)]

Attachment: [WIP] Prevent reseting preserveAspectRatio for SVG without viewBox

This patch can fix the problem, but it is more like a walk-around solution.

When using SVG image, which has viewBox attribute, as border-image, the preserveAspectRatio of SVG will be set to none for non-uniform scaling [1]; however, the reset of preserveAspectRatio somehow effect the rendering result of those border-images which use SVG without viewBox as image source.

I will continue inspecting the reason why the preserveAspectRatio effects the rendering result even if SVG source doesn't have viewBox attribute.


[1] https://dxr.mozilla.org/mozilla-central/source/image/VectorImage.cpp#840

Comment 14

[Kevin Chen[:kechen] (UTC + 8)]

Attachment: [WIP] Prevent reseting preserveAspectRatio for SVG without viewBox

Hi Daniel Holbert & CJ,
Can you give me some advices to this patch ?

When using SVG image as border-image, there are four possible settings in SVG image :
1. Has both viewport size and viewBox.
2. Has viewBox, but no viewport size.
3. Has viewport size, but no viewBox.
4. No viewport size and viewBox.

In bug 619500, we fixed the situation 1 & 2, it sets the preserveAspectRatio to none, so that SVG image can scale non-uniformly to fit border-image area [1].

This change should only effect the behavior of the SVG images who has viewBox attribute; however, the reset of preserveAspectRatio will trigger SetImageOverridePreserveAspectRatio which sets the flag  mIsPaintingSVGImageElement to true [2] and because we have changed viewport size in [3] to get proper size for rendering which cause system to recalculate a new viewBox value with modified viewport size and scale the SVG image improperly which cause situation 3 broken.

So, the solution to this problem is to avoid the reset of preserveAspectRatio if SVG source doesn't have viewBox attribute.


And for situation 4, when we scale the viewport size in [3], we kind of break its width height ratio and make it resize improperly, so my solution here is to select bigger ratio of height and width to scale the viewport size, kind of like what "slice" in preserveAspectRatio do to keep the ratio of viewport size.

Does the solution looks okay to you ?
Thank you.


[1] https://dxr.mozilla.org/mozilla-central/source/image/VectorImage.cpp?from=VectorImage.cpp#840
[2] https://dxr.mozilla.org/mozilla-central/source/dom/svg/SVGSVGElement.cpp#1100
[3] https://dxr.mozilla.org/mozilla-central/source/layout/base/nsLayoutUtils.cpp#6482

Comment 15

[u459114]

Comment on attachment 8758583 [details] [diff] [review]
[WIP] Prevent reseting preserveAspectRatio for SVG without viewBox

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

LGTM, thanks, Kevin

Comment 16

[Daniel Holbert [:dholbert]]

(In reply to Kevin Chen[:kechen] from comment #14)
> however, the reset of preserveAspectRatio will trigger
> SetImageOverridePreserveAspectRatio which sets the flag 
> mIsPaintingSVGImageElement to true [2]
[which disables our code that synthesizes a viewBox from width/height attributes]
> So, the solution to this problem is to avoid the reset of
> preserveAspectRatio if SVG source doesn't have viewBox attribute.

I'd prefer that we solve this by *preventing ourselves* from ever incorrectly setting mIsPaintingSVGImageElement, when we override the preserveAspectRatio attribute here.  That seems to be the root of the problem.

(That bool is specifically about whether we're being painted for the SVG "<image>" element, in a SVG host document.  If we're being painted for one of those elements, we need to *not* synthesize a viewBox -- that's why that bool exists.  But otherwise, for HTML/CSS contexts, we absolutely should allow ourselves to synthesize a viewbox.)

So I think we need to decouple SetImageOverridePreserveAspectRatio() from "mIsPaintingSVGImageElement", and add a new private setter (called maybe "SetIsPaintingForSVGImageElement()"), and add a new bool to SVGImageContext (alongside the override preserveAspectRatio value) which controls whether AutoSVGRenderingState will call this new method.

Does that make sense?

Comment 17

[Daniel Holbert [:dholbert]]

(In reply to Kevin Chen[:kechen] from comment #14)
> And for situation 4, when we scale the viewport size in [3], we kind of
> break its width height ratio and make it resize improperly, so my solution
> here is to select bigger ratio of height and width to scale the viewport
> size, kind of like what "slice" in preserveAspectRatio do to keep the ratio
> of viewport size.

(For this part: I'm not as familiar with what's going on in this section of ClippedImage, and I'd need to do a bit more poking around to be sure that what you're doing there makes sense. My first reaction is that I think it might be simpler/cleaner to adjust |scale| up-front, rather than to adjust |finalScale|; but maybe it doesn't matter much.)

Comment 18

[Daniel Holbert [:dholbert]]

Comment on attachment 8758583 [details] [diff] [review]
[WIP] Prevent reseting preserveAspectRatio for SVG without viewBox

(feedback- since I think we should go about fixing this differently, per comment 16)

Comment 19

[Kevin Chen[:kechen] (UTC + 8)]

Attachment: (Part 1) Decouple SetImageOverridePreserveAspectRatio from mIsPaintingSVGImageElement setting

I separate this patch into two parts, since situation 3 & 4 mentioned in comment 14 are caused by different reasons.

In this patch, I decouple SetImageOverridePreserveAspectRatio() from mIsPaintingSVGImageElement which solves the problem listed in situation 3.

Comment 20

[Kevin Chen[:kechen] (UTC + 8)]

Attachment: [WIP] (Part 2) Preserve the scale ratio if SVG dosen't have viewport size

In this patch, I preserve the width/height ratio of SVG source which has no viewport size when calculating the optimal rendering size.

This can fix the problem listed in situation 4, however, there might be a better way to solve this. Still trying it.

Comment 21

[Kevin Chen[:kechen] (UTC + 8)]

Attachment: (Part 2) Preserve scale ratio if SVG dosen't have viewport size and viewBox.

Hi Daniel,

When the border image do the image snapping algorithm[1] separately, it will scale the SVG source's viewport size to get the best resolution[2]. However, for those SVG sources who don't have viewport size or viewBox value, this adjustment will break the ratio and squeeze the twist. Therefore, I add a flag "FLAG_SCALE_WITH_FIXED_RATIO" to prevent the source from non-fix ratio scaling.

Can you help me to review this patch ? Thank you.

[1] https://wiki.mozilla.org/Gecko:Image_Snapping_and_Rendering
[2] https://dxr.mozilla.org/mozilla-central/source/layout/base/nsLayoutUtils.cpp#6553

Comment 22

[Kevin Chen[:kechen] (UTC + 8)]

Comment on attachment 8759607 [details] [diff] [review]
(Part 1) Decouple SetImageOverridePreserveAspectRatio from mIsPaintingSVGImageElement setting

Hi Daniel,

I modify the patch according to comment 16, can you help me to review this patch ?
Thank you.

Comment 23

[Kevin Chen[:kechen] (UTC + 8)]

Attachment: (Part 3) Add reftest.

Hi Daniel,

I add two reftests, for situation 3 & 4 which is mentioned in comment 14.
Can you help me to review this patch? Thank you.

Comment 24

[Daniel Holbert [:dholbert]]

Comment on attachment 8759607 [details] [diff] [review]
(Part 1) Decouple SetImageOverridePreserveAspectRatio from mIsPaintingSVGImageElement setting

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

Right now, there's a newline in the middle of the commit message -- please remove that newline and be sure that the full "main" commit message is on a single line inside the patch-file.

(Many tools, e.g. hg.mozilla.org pushlog, only display the first line of the commit message, i.e. up to the first newline character.  So, an arbitrary linebreak will inadvertantly truncate the message that's shown in those tools.)

::: image/VectorImage.cpp
@@ +842,5 @@
>        Some(SVGPreserveAspectRatio(SVG_PRESERVEASPECTRATIO_NONE,
>                                    SVG_MEETORSLICE_UNKNOWN));
>      svgContext =
>        Some(SVGImageContext(aSVGContext->GetViewportSize(),
> +                           aspectRatio, 1.0, false));

As noted below, I think your new arg to SVGImageContext() should *default* to false, which means you can revert this change here, I think.

::: layout/svg/SVGImageContext.h
@@ +26,5 @@
>  
>    SVGImageContext(CSSIntSize aViewportSize,
>                    Maybe<SVGPreserveAspectRatio> aPreserveAspectRatio,
> +                  gfxFloat aOpacity = 1.0,
> +                  bool aIsPaintingSVGImageElement = true)

Two notes here:
 (1) Please add a line of documentation for "aIsPaintingSVGImageElement" somewhere. (It's extremely easy to misunderstand what this variable would mean, due to "SVG Image" being an ambiguous term -- and every SVGImageContext being associated with an "SVG Image".)

e.g. maybe above the constructor here, add a comment saying:
  // Note: 'aIsPaintingSVGImageElement' should be used to indicate whether
  // the SVG image in question is being painted for an SVG <image> element.


(2) I don't think aIsPaintingSVGImageElement should default to "true". Painting for an SVG <image> element is a rare scenario, so most callers will want this variable to be set to false.

I think the only place where we should set it to true (i.e. the only place we're actually painting an SVG <image> element) is in the SVGImageContext instance that gets created in nsSVGImageFrame.cpp, here:
http://mxr.mozilla.org/mozilla-central/source/layout/svg/nsSVGImageFrame.cpp?rev=0f0586c0b68d&force=1#400

Comment 25

[Daniel Holbert [:dholbert]]

Comment on attachment 8760535 [details] [diff] [review]
(Part 2) Preserve scale ratio if SVG dosen't have viewport size and viewBox.

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

This patch has a newline in the middle of its commit message -- please fix that, as noted above for part 1.

::: image/ClippedImage.cpp
@@ +505,5 @@
>    }
>  
>    int32_t imgWidth, imgHeight;
> +  enum ScaleFlag {DONT_SCALE, NEED_SCALE, SCALE_WITH_RATIO};
> +  ScaleFlag needScale = DONT_SCALE;

I tend to think this enum stuff is more confusing than it's worth. (In part because the names here are a bit confusing -- e.g. "needScale" still sounds like a bool, and needScale/NEED_SCALE have the same name and yet the latter is just one possible value that the former can take.)

Seems to me it'd be simpler to revert all of the enum stuff here, and just control the new uniform-scaling behavior with a new bool called e.g. "mustScaleUniformly".

So, I think this should go back to just:
  bool needScale = false;
...and you should add after that:
  bool mustScaleUniformly = false;

And all other usages of "needScale" should be reverted.

@@ +510,5 @@
>    if (mSVGViewportSize && !mSVGViewportSize->IsEmpty()) {
>      imgWidth = mSVGViewportSize->width;
>      imgHeight = mSVGViewportSize->height;
> +    needScale = (aFlags & imgIContainer::FLAG_SCALE_WITH_FIXED_RATIO)
> +      ? SCALE_WITH_RATIO : NEED_SCALE;

And this line should reverting back to:
  needScale = true;
...and you'll want to insert after that:
  mustScaleUniformly = (aFlags & imgIContainer::FLAG_SCALE_WITH_FIXED_RATIO);

@@ +525,5 @@
>      nsIntSize scale(ceil(aDest.width / mClip.width),
>                      ceil(aDest.height / mClip.height));
>  
> +    if (needScale == SCALE_WITH_RATIO) {
> +      scale.width = scale.height = max(scale.height, scale.width);

And this would just be "if (mustScaleUniformly)"

::: image/imgIContainer.idl
@@ +206,5 @@
>    const unsigned long FLAG_HIGH_QUALITY_SCALING            = 0x40;
>    const unsigned long FLAG_WANT_DATA_SURFACE               = 0x80;
>    const unsigned long FLAG_BYPASS_SURFACE_CACHE            = 0x100;
>    const unsigned long FLAG_FORCE_PRESERVEASPECTRATIO_NONE  = 0x200;
> +  const unsigned long FLAG_SCALE_WITH_FIXED_RATIO          = 0x400;

I'd suggest we name this...
 FLAG_FORCE_UNIFORM_SCALING

That seems clearer to me; "uniform scaling" is pretty unambiguous, I think. (and it happens to match the variable-naming I'm suggesting in ClippedImage.cpp)

(The "ratio" in "SCALE_WITH_FIXED_RATIO" is ambiguous -- e.g. it could be misread as saying that the ratio *between our scale factors* might have a fixed ratio, if perhaps the caller wanted scaleX to always be twice as large as scaleY, for some reason.  Anyway, that's not what you want this flag to mean, so that sort of ambiguity is worth avoiding.)

::: layout/base/nsCSSRendering.cpp
@@ +3866,5 @@
>        // In this condition, we pass imageSize(a resolved size comes from
>        // default sizing algorithm) to renderer as the viewport size.
> +      Maybe<nsSize> svgViewportSize =
> +        (intrinsicSize.CanComputeConcreteSize()) ?
> +          Nothing() : Some(imageSize);

Probably revert this change, unless it's doing something substantial that I'm missing.

(It doesn't seem to be actually changing the code -- it's just adding a linebreak and some extra parens, I think.  IMO, the old version is easier to read.

@@ +5486,5 @@
>      // FLAG_FORCE_PRESERVEASPECTRATIO_NONE flag here, to tell mImage to ignore
>      // preserveAspectRatio attribute, and always do non-uniform stretch.
>      uint32_t drawFlags = ConvertImageRendererToDrawFlags(mFlags) |
>                             imgIContainer::FLAG_FORCE_PRESERVEASPECTRATIO_NONE;
> +    // For those SVG image sources which don't have fix ratio (i.e without

Typos:
  s/fix ratio/fixed ratio/
  s/i.e/i.e./  (Add a period after "e")

@@ +5487,5 @@
>      // preserveAspectRatio attribute, and always do non-uniform stretch.
>      uint32_t drawFlags = ConvertImageRendererToDrawFlags(mFlags) |
>                             imgIContainer::FLAG_FORCE_PRESERVEASPECTRATIO_NONE;
> +    // For those SVG image sources which don't have fix ratio (i.e without
> +    // viewport size and viewbox), we should scale the source in a fixed ratio

s/in a fixed ratio/uniformly/

@@ +5488,5 @@
>      uint32_t drawFlags = ConvertImageRendererToDrawFlags(mFlags) |
>                             imgIContainer::FLAG_FORCE_PRESERVEASPECTRATIO_NONE;
> +    // For those SVG image sources which don't have fix ratio (i.e without
> +    // viewport size and viewbox), we should scale the source in a fixed ratio
> +    // after the viewport size is desided by "Default Sizing Algorithm".

Typo:
 s/desided/decided/

@@ +5489,5 @@
>                             imgIContainer::FLAG_FORCE_PRESERVEASPECTRATIO_NONE;
> +    // For those SVG image sources which don't have fix ratio (i.e without
> +    // viewport size and viewbox), we should scale the source in a fixed ratio
> +    // after the viewport size is desided by "Default Sizing Algorithm".
> +    if (!ComputeIntrinsicSize().CanPreserveRatio()) {

As noted below, I think this should perhaps just be "HasAspectRatio()" (I don't think we need to add CanPreserveRatio()).

::: layout/base/nsCSSRendering.h
@@ +50,5 @@
>      return mHasWidth + mHasHeight + HasRatio() >= 2;
>    }
> +  bool CanPreserveRatio() const
> +  {
> +    return (mHasWidth & mHasHeight) | HasRatio();

You're using bitwise operators here... I think you really meant to use logical operators? (&& and ||, instead of & and |)

Also, more importantly -- do we actually need this function?  I'm pretty sure "HasRatio()" on its own should be sufficient.  (Could you give that a try? If that fails, I'd be curious to see why).

I think you're trying to cover the "<svg> has |width| and |height| specified, but doesn't have viewBox" scenario here.  BUT, HasRatio() already effectively covers that scenario, because it ends up being backed[1] by nsSVGOuterSVGFrame::GetIntrinsicRatio(), which already has custom code to handle width & height being specified. (and it successfully generates an aspect ratio from them)

[1] (HasRatio returns mRatio, which is set via nsLayoutUtils::ComputeSizeForDrawing, which invokes VectorImage::GetIntrinsicRatio, which calls GetIntrinsicRatio() on the outer frame, and the outer frame for an SVG document will be of type nsSVGOuterSVGFrame ).

Comment 26

[Daniel Holbert [:dholbert]]

Comment on attachment 8760675 [details] [diff] [review]
(Part 3) Add reftest.

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

::: layout/reftests/border-image/reftest.list
@@ +87,5 @@
>  == svg-as-border-image-1a.html svg-as-border-image-1-ref.html
>  == svg-as-border-image-1b.html svg-as-border-image-1-ref.html
>  == svg-as-border-image-1c.html svg-as-border-image-1-ref.html
> +fuzzy(75,500) == svg-as-border-image-1d.html svg-as-border-image-1-ref-2.html
> +== svg-as-border-image-1e.html svg-as-border-image-1-ref-3.html

Why do these files have "-1-ref-2" / "-1-ref-3" suffixes? Are they reference cases, or testcases? (I think they're testcases, in which case they should be "-1f" and "-1g" perhaps?)

If it's not clear, our naming convention for reftests is to name the files like so:
 foo-1.html foo-1-ref.html
 foo-2.html foo-2-ref.html

...and if there are multiple files that match a single reference case (as is the case in the preexisting tests here), we'll have:
 foo-1a.html foo-1-ref.html
 foo-1b.html foo-1-ref.html
 foo-1c.html foo-1-ref.html
 foo-2.html  foo-2-ref.html

Subsequent tests should only have "foo-1" in their name if they're actually testcases that match "foo-1-ref.html". Otherwise, they want to be numbered "2" or "3" or whatever.

So: perhaps you want to be adding these new files with names like...
== svg-as-border-image-2.html svg-as-border-image-2-ref.html
== svg-as-border-image-3.html svg-as-border-image-3-ref.html

? (I'm not sure)

Also: "fuzzy(75,500)" is a pretty large amount of fuzziness/mismatch for your first new test here. Do you know why your test needs that? Is it actually passing?

Comment 27

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #25)
> Seems to me it'd be simpler to revert all of the enum stuff here, and just
> control the new uniform-scaling behavior with a new bool called e.g.
> "mustScaleUniformly".
[...]
> > +  const unsigned long FLAG_SCALE_WITH_FIXED_RATIO          = 0x400;
> 
> I'd suggest we name this...
>  FLAG_FORCE_UNIFORM_SCALING

Er -- actually, I should have suggested "bool forceUniformScaling" instead of "bool mustScaleUniformly", for better consistency with my suggested replacement FLAG_ name (FLAG_FORCE_UNIFORM_SCALING).  Seems worth having that naming be consistent.

Comment 28

[Kevin Chen[:kechen] (UTC + 8)]

Attachment: (Part 1) Decouple SetImageOverridePreserveAspectRatio from mIsPaintingSVGImageElement setting

Hi Daniel,

I modified this patch according to comment 24, can you help me to review it ?

Thank you.

Comment 29

[Kevin Chen[:kechen] (UTC + 8)]

Attachment: (Part 2) Preserve scale ratio if SVG dosen't have viewport size and viewBox.

Hi Daniel,

I modified this patch according to comment 25 and comment 27, can you help me to review it ?

Thank you.

Comment 30

[Kevin Chen[:kechen] (UTC + 8)]

Attachment: (Part 3) Add reftest.

Hi Daniel,

I modified this patch according to comment 26 and found out that test svg-as-border-image-1b.html is covered by the test svg-as-border-image-2.html in this patch, should I also remove it ?

[1] https://dxr.mozilla.org/mozilla-central/source/layout/reftests/border-image/svg-as-border-image-1b.html

Comment 31

[Kevin Chen[:kechen] (UTC + 8)]

Try for these patches, an error occurs at Linux x64 debug tc-VP[tier 2], but that is not caused by these patches.

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

Comment 32

[Daniel Holbert [:dholbert]]

Comment on attachment 8765726 [details] [diff] [review]
(Part 1) Decouple SetImageOverridePreserveAspectRatio from mIsPaintingSVGImageElement setting

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

r=me with nits addressed.

Nit #1: As I noted in comment 24, your commit message still has a newline in the middle of it. Please look at the text of the patch file -- you'll see that it starts out like this:
> From: Kevin Chen <[e-mail]>
> Date: Fri, 3 Jun 2016 18:45:42 +0800
> Subject: Bug 1264809 - (Part 1) Decouple SetImageOverridePreserveAspectRatio
>  from mIsPaintingSVGImageElement setting.

Nit #2: Notice the hardcoded newline in the middle of the commit message there. Please drop it (and add "r=dholbert" at the end).  If a tool is auto-generating that newline for you when you populate your commit message, please reconfigure whatever tool you're using to edit commit messages so that it stops doing that.

::: layout/svg/SVGImageContext.h
@@ +48,5 @@
>      return mGlobalOpacity;
>    }
>  
> +  bool GetIsPaintingForSVGImageElement() const {
> +    return mIsPaintingSVGImageElement;

Please drop the "Get" from the function name here -- it's unnecessary.  This accessor can just be called "IsPaintingForSVGImageElement()".

(This makes it slightly asymmetric since we've got a SetIsPaintingForSVGImageElement(), but that's OK.  My recollection is that this is common practice for naming getters & setters for true/false things in Gecko.)

Comment 33

[Daniel Holbert [:dholbert]]

(Sorry, during some last-minute comment editing there, I misplaced my "Nit #2" label. I meant to associate that label with the "drop the Get from the function name" thing at the end, but I accidentally typed it into the middle of my first nit. Feel free to just ignore my mis-inserted "Nit #1" and "Nit #2" labels, and just refer to the text of my bug-comment. :))

Comment 34

[Daniel Holbert [:dholbert]]

Your updated "part2" still has a newline in the commit message, too. Please get rid of this newline there as well.

Also: there's a hardcoded date-stamp in the patch header for each of your patches here, which happens to be about a month out of date -- e.g. "part 2" has this in the second line of the patch file:
 "Date: Fri, 3 Jun 2016 19:11:26 +0800"

Please either remove those datestamps from your patch files when you repost the final versions here, or (if your tools insist on including a datestamp) please refresh the patches to use a more recent date. (Otherwise, this stale datestamp will stick around as the "official" datestamp of your commit on hg.mozilla.org, and it can cause confusion if hg-archeologists are trying to figure out when your patch landed when browsing hg history in the future.)

Comment 35

[Daniel Holbert [:dholbert]]

Comment on attachment 8765727 [details] [diff] [review]
(Part 2) Preserve scale ratio if SVG dosen't have viewport size and viewBox.

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

r=me with commit message / datestamp issues fixed (comment 34) and the following nits addressed:

::: layout/base/nsCSSRendering.cpp
@@ +5523,5 @@
>      // preserveAspectRatio attribute, and always do non-uniform stretch.
>      uint32_t drawFlags = ConvertImageRendererToDrawFlags(mFlags) |
>                             imgIContainer::FLAG_FORCE_PRESERVEASPECTRATIO_NONE;
> +    // For those SVG image sources which don't have fixed ratio (i.e. without
> +    // viewport size and viewbox), we should scale the source uniformly after

Two tweaks here:
 (a) s/ratio/aspect ratio/ for clarity (sorry, I should have said this up in comment 25)

 (b) Please use the correct capitalization for "viewBox" in the comment here (capitalize the letter "B" in the middle of the word.)

Comment 36

[Daniel Holbert [:dholbert]]

(In reply to Kevin Chen[:kechen] from comment #30)
> Created attachment 8765729 [details] [diff] [review]
> (Part 3) Add reftest.
> I modified this patch according to comment 26 and found out that test
> svg-as-border-image-1b.html is covered by the test
> svg-as-border-image-2.html in this patch, should I also remove it ?

Thanks for noticing -- I'd say we should just keep the old test around, even if your new test is similar.  Since the older test passes currently (and your new test does not), that makes me think they're testing scenarios that might be subtly-different enough to be worth testing independently.

(It may also be somewhat useful for completeness, to be able to compare it against the other svg-as-border-image-1a and svg-as-border-image-1c flavors and see what we're testing there.)

Comment 37

[Daniel Holbert [:dholbert]]

Comment on attachment 8765729 [details] [diff] [review]
(Part 3) Add reftest.

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

r=me on the test patch, with the month-old datestamp removed or made more current as noted above, and with the following nits:

::: layout/reftests/border-image/svg-as-border-image-2-ref.html
@@ +1,4 @@
> +<!DOCTYPE html>
> +<html lang="en-US">
> +<head>
> +<title>test of svg-as-border-image</title>

Nit: s/test of/reference for/ in the <title> here.  (since this file a reference case, not a testcase)

::: layout/reftests/border-image/svg-as-border-image-2.html
@@ +7,5 @@
> +  width: 10px;
> +  height: 10px;
> +  background-color: blue;
> +  border: 10px solid;
> +  border-image: url('data:image/svg+xml,<svg xmlns="http://www.w3.org/2000/svg" ><circle cx="50%" cy="50%" r="50%" fill="red"/></svg>') 5% stretch;

Nit: drop the stray space character just below the first ">" on this line. (after the xmlns attribute)

::: layout/reftests/border-image/svg-as-border-image-3-ref.html
@@ +1,4 @@
> +<!DOCTYPE html>
> +<html lang="en-US">
> +<head>
> +<title>test of svg-as-border-image</title>

As above, s/test of/reference for/

Comment 39

[Kevin Chen[:kechen] (UTC + 8)]

Attachment: (Part 1) Decouple SetImageOverridePreserveAspectRatio from mIsPaintingSVGImageElement setting

Carry r+ from comment 32

Try result shows in comment 31

Comment 40

[Kevin Chen[:kechen] (UTC + 8)]

Attachment: (Part 2) Preserve scale ratio if SVG dosen't have viewport size and viewBox.

Carry r+ from comment 35

Try result shows in comment 31

Comment 41

[Kevin Chen[:kechen] (UTC + 8)]

Attachment: (Part 3) Add reftest.

Carry r+ from comment 37

Try result shows in comment 31

Comment 42

[Carsten Book [:Tomcat]]

has problems to apply:

renamed 1264809 -> 0002-Bug-1264809-Part-2-Preserve-scale-ratio-if-SVG-dosen.patch
applying 0002-Bug-1264809-Part-2-Preserve-scale-ratio-if-SVG-dosen.patch
patching file image/ClippedImage.cpp
Hunk #1 FAILED at 0
Hunk #2 succeeded at 24 with fuzz 2 (offset 0 lines).
1 out of 3 hunks FAILED -- saving rejects to file image/ClippedImage.cpp.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh 0002-Bug-1264809-Part-2-Preserve-scale-ratio-if-SVG-dosen.patch

Comment 43

[Kevin Chen[:kechen] (UTC + 8)]

Attachment: (Part 2) Preserve scale ratio if SVG dosen't have viewport size and viewBox.

Fix the conflict which is caused by reorder of header file in Bug 1282670

Carry r+ from comment 35

Try result shows in comment 31

Comment 44

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5bd0b92309c7
(Part 1) Decouple SetImageOverridePreserveAspectRatio from mIsPaintingSVGImageElement setting. r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a7eb4a7864c
(Part 2) Preserve scale ratio if SVG dosen't have viewport size and viewBox. r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/d19e8687583e
(Part 3) Add reftest. r=dholbert

Comment 45

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5bd0b92309c7
https://hg.mozilla.org/mozilla-central/rev/8a7eb4a7864c
https://hg.mozilla.org/mozilla-central/rev/d19e8687583e

Comment 46

[Sebastian Zartner [:sebo]]

The cases with the size 60 and 300 SVGs are fixed now. Thank you for that!

Unfortunately the one with the SVG without sizes is still unproportionally stretched. Also the size 10 SVG is incorrectly stretched, as the circles are cut a bit outside of their middle (I'll attach a screenshot for clarification).
In both cases the border images should look the same as for the size 60 and 300 SVGs.

Sebastian

Comment 47

[Robert Longson [:longsonr]]

Additional issues should be raised in a new bug.

Comment 48

[Sebastian Zartner [:sebo]]

Attachment: Incorrectly stretched SVGs without sizes and with 10 pixels

Here´s a screenshot with the erroneous parts marked and explained.
Again, all four cases with SVGs are expected to look the same.

Sebastian

Comment 49

[Sebastian Zartner [:sebo]]

(In reply to Robert Longson from comment #47)
> Additional issues should be raised in a new bug.

Those are part of the same issue, but I´ll file additional bugs if that's easier to track.

Sebastian

Comment 50

[Robert Longson [:longsonr]]

Please file new bugs, it's easier to track that way. We'd only reopen this bug if its patches need to be backed out.

Comment 51

[Sebastian Zartner [:sebo]]

(In reply to Robert Longson from comment #50)
> Please file new bugs, it's easier to track that way. We'd only reopen this
> bug if its patches need to be backed out.

Ok. So I've created bug 1284797 and bug 1284798 accordingly and provided adjusted test cases for them.

Sebastian

Comment 52

[Sebastian Zartner [:sebo]]

I've added a compatibility note about this to https://developer.mozilla.org/en-US/docs/Web/CSS/border-image.

Sebastian

Comment 53

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Is 49 also affected? Is this work all planned to ride with 50 or does any of it need uplift? I'm asking because bug 1269528 is marked as a regression in 48 and 49 and so it shows up in platform triage.

Comment 54

[Kevin Chen[:kechen] (UTC + 8)]

Yes, I think the versions after bug 619500, which is firefox48, are affected.
Should I also mark the regression tags in bug 1269528 and uplift the patches ?

Comment 55

[Astley Chen (inactive)]

Hi Kevin,
  Talked with FF48 release manager Gerry, both of us think that for such trivial visual artifact and regressed from FF48 will need to be fixed before we punt 48 to release channel users.
As long as you think the risk is manageable, please uplift the patch to FF48 and FF49.

Thanks.

Comment 56

[Gerry Chang [:gchang]]

Track this in 48/49/50 as this is a regression. After the patch is uplift to beta, need QE to verify it.

Comment 57

[David Baron :dbaron:]

What was this a regression from?

I thought this was simply an issue that was not fixed by bug 619500?  Did something actually *regress*?

Uplifting to beta at this point in the cycle is risky and shouldn't be done without a good reason.  We don't get substantial Web-compatibility testing and feedback, plus a chance to fix based on that feedback, in 3 weeks.

Comment 58

[Sebastian Zartner [:sebo]]

(In reply to David Baron :dbaron: ⌚️UTC-4 (away July 7-10) (review requests must explain patch) from comment #57)
> What was this a regression from?
> 
> I thought this was simply an issue that was not fixed by bug 619500?  Did
> something actually *regress*?

Bug 619500 regressed some behavior of 'border-image-slice', see bug 619500 comment 85.

Sebastian

Comment 59

[David Baron :dbaron:]

Is that regression fully fixed in this bug?  Or are bug 1284797 and bug 1284798 also required to fix the regression from bug 619500?

Comment 60

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

How common will it be for users to hit these issues? I don't understand comment 55. If this is a trivial issue let's keep it in 50.

Comment 61

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Marking this as fix-optional for 48, while we could still consider uplift to beta I don't think it needs to block release.  We could still take this for aurora.

Comment 62

[David Baron :dbaron:]

Could you answer comment 59?

(Also, it helps to explain this more clearly when you report the bug.)

Comment 63

[Kevin Chen[:kechen] (UTC + 8)]

Attachment: (Uplift - aurora)(Part 2) Preserve scale ratio if SVG dosen't have viewport size and viewBox.

Approval Request Comment
[Feature/regressing bug #]:
  Bug 619500
[User impact if declined]:
  The websites who uses SVG , which doesn't contain a viewBox attribute, as the source of border-image might find display errors.
[Describe test coverage new/current, TreeHerder]:
  Two reftests are contained in patch part 3.
[Risks and why]: 
  I think the risk is low, because in this patch the changes I made only affect the flow of rendering border-image.

Comment 64

[Kevin Chen[:kechen] (UTC + 8)]

Comment on attachment 8767071 [details] [diff] [review]
(Part 3) Add reftest.

Approval Request Comment
[Feature/regressing bug #]:
  Bug 619500
[User impact if declined]:
  The websites who uses SVG , which doesn't contain a viewBox attribute, as the source of border-image might find display errors.
[Describe test coverage new/current, TreeHerder]:
  Two reftests are contained in patch part 3.
[Risks and why]: 
  I think the risk is very low, because in this patch I only add two tests and doesn't change any code flow.

Comment 65

[Kevin Chen[:kechen] (UTC + 8)]

Comment on attachment 8767069 [details] [diff] [review]
(Part 1) Decouple SetImageOverridePreserveAspectRatio from mIsPaintingSVGImageElement setting

Approval Request Comment
[Feature/regressing bug #]:
  Bug 619500
[User impact if declined]:
  The websites who uses SVG , which doesn't contain a viewBox attribute, as the source of border-image might find display errors.
[Describe test coverage new/current, TreeHerder]:
  Two reftests are contained in patch part 3.
[Risks and why]: 
  I think the risk is low, because the changes I made is only to decide whether the SVG image are allowed to synthesize the viewBox. And the logic is pretty clear.
But, still, I think it's better if someone who is more familiar with this module can help to evaluate the risk of this patch because the codes which is affected here is relatively big.

Comment 66

[Kevin Chen[:kechen] (UTC + 8)]

Hi Liz,

I've already applied my patches to aurora and run the related tests in my local machine. The only conflict happened in patch part 2, which I've fixed it and uploaded it as the attachment above.

Please notice me if these patches are also needed to be uplifted to beta.

Thanks.

Comment 67

[Kevin Chen[:kechen] (UTC + 8)]

Attachment: testcase

Comment 68

[Kevin Chen[:kechen] (UTC + 8)]

Attachment: screenshot

Comment 69

[Kevin Chen[:kechen] (UTC + 8)]

Attachment: small-size svg screenshot

Screenshot of small-size SVG before bug 619500.

Comment 70

[Kevin Chen[:kechen] (UTC + 8)]

Hi David,

I think maybe I can provide some information to question in comment 59.

Yes, I think this bug can fully fix the regression from Bug 619500.

The regression caused by Bug 619500 happened only when we use SVG, which has viewport size but no viewBox size and also contains <circle> elements, as source of the border-image attribute.

The image in attachment 8769552 [details] is the screenshot of the testcase in attachment 8769551 [details].

The left-hand side is the result with build after Bug 619500[1] and the right-hand side is the result with build before Bug 619500[2], the result shows that:
  1. Bug 619500 fix the case which SVG source has viewBox attribute set.
  2. Break the case which SVG source has viewport size but doesn't has viewBox.
  3. The act of SVG source which has no viewport size nor viewBox attribute changes but still remains broken.

And the patches in this bug solved the case 2 & 3 mentioned above.

And the image in attachment 8769553 [details] is the screenshot with small-size SVG source with build before Bug 619500[2], which is the problem Bug 1284797 wants to solve.
In the other hand, the situation Bug 1284798 wants to discuss is just like the first case in testcase in attachment 8769551 [details], it is broken before and after Bug 619500.
Therefore, IMHO, these two bugs are not regression from Bug619500.


Needinfo to Sebastian again if I miss something. Thank you.

[1] https://hg.mozilla.org/mozilla-central/rev/bc277a82a03e
[2] https://hg.mozilla.org/mozilla-central/rev/4adaa4b1ddb2

Comment 71

[Kevin Chen[:kechen] (UTC + 8)]

Needinfo to Sebastian again if I miss something. Thank you.

Comment 72

[Kevin Chen[:kechen] (UTC + 8)]

Attachment: SVG without viewport size and viewBox

Comment 73

[Kevin Chen[:kechen] (UTC + 8)]

Attachment: SVG without vieport size but with viewBox

Comment 74

[Kevin Chen[:kechen] (UTC + 8)]

Attachment: SVG with viewportsize but without viewBox

Comment 75

[Kevin Chen[:kechen] (UTC + 8)]

Attachment: SVG with viewport size and viewBox

Comment 76

[Kevin Chen[:kechen] (UTC + 8)]

Attachment: testcase

Comment 77

[Sebastian Zartner [:sebo]]

Excuse my late reply! Thank you Kevin for explaining the regression in detail!

(In reply to David Baron :dbaron: ⌚️UTC-4 (away July 7-10) (review requests must explain patch) from comment #57)
> What was this a regression from?
> 
> I thought this was simply an issue that was not fixed by bug 619500?  Did
> something actually *regress*?

As Kevin wrote, bug 619500 regressed the display for SVGs that don't have a viewBox defined. That got fixed in this bug.

> Uplifting to beta at this point in the cycle is risky and shouldn't be done
> without a good reason.  We don't get substantial Web-compatibility testing
> and feedback, plus a chance to fix based on that feedback, in 3 weeks.

(In reply to David Baron :dbaron: ⌚️UTC-4 (away July 7-10) (review requests must explain patch) from comment #59)
> Is that regression fully fixed in this bug?

Yes.

> Or are bug 1284797 and bug 1284798 also required to fix the regression from bug 619500?

Both are independent from bug 619500.

Sebastian

Comment 78

[Daniel Holbert [:dholbert]]

(In reply to Kevin Chen[:kechen] (UTC + 8) from comment #65)
> [Risks and why]: 
>   I think the risk is low, because the changes I made is only to decide
> whether the SVG image are allowed to synthesize the viewBox. And the logic
> is pretty clear.
> But, still, I think it's better if someone who is more familiar with this
> module can help to evaluate the risk of this patch because the codes which
> is affected here is relatively big.

I'll back up Kevin on this -- I think the risk here is sufficiently low for an Aurora uplift.

(I think it's too late for a beta uplift, though, for a patch of this size/complexity.)

Comment 79

[Astley Chen (inactive)]

Thanks Daniel. Kevin, per discussion, please uplift your patches to aurora only. We will back out the fix in bug 619500 in beta to get rid of regression in firefox 48.

Comment 80

[Kevin Chen[:kechen] (UTC + 8)]

Hi Liz,

I've tagged my patches as "approval-mozilla-aurora?" and evaluated the risks.

Can you help me to approve the requests and uplifts the patches?

Thank you.

Comment 81

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8767069 [details] [diff] [review]
(Part 1) Decouple SetImageOverridePreserveAspectRatio from mIsPaintingSVGImageElement setting

Better graphics support, adds new tests, let's uplift to aurora.

Comment 82

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d183d71a7545
https://hg.mozilla.org/releases/mozilla-aurora/rev/9b3ee31983dc
https://hg.mozilla.org/releases/mozilla-aurora/rev/39f61988e8fb

Comment 83

[Sebastian Zartner [:sebo]]

Thank you for the uplift! I've updated the documentation at https://developer.mozilla.org/en-US/docs/Web/CSS/border-image#Browser_compatibility accordingly.

Sebastian

Comment 84

[Gerry Chang [:gchang]]

Since the fix of bug 619500 is back out, mark 48 as won't fix.

Comment 85

[Gerry Chang [:gchang]]

Comment on attachment 8769538 [details] [diff] [review]
(Uplift - aurora)(Part 2) Preserve scale ratio if SVG dosen't have viewport size and viewBox.

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

As mentioned in comment #81, better graphics support. Let's uplift to aurora.

Comment 86

[u459114]

Kevin, please add a patch to comment what FLAG_FORCE_UNIFORM_SCALING means(bug 619500 comment 107)

Comment 87

[Kevin Chen[:kechen] (UTC + 8)]

Attachment: (Part 4) Add comment for FLAG_FORCE_UNIFORM_SCALING.

Hi Daniel,

I add a comment for FLAG_FORCE_UNIFORM_SCALING in this patch.

Can you help me to review it ?

Thank you.

Comment 88

[Daniel Holbert [:dholbert]]

Comment on attachment 8777637 [details] [diff] [review]
(Part 4) Add comment for FLAG_FORCE_UNIFORM_SCALING.

Hi Kevin! Sorry for the delay -- I was away for a few days and am now catching up on review backlog.

>+++ b/image/imgIContainer.idl
>+   * FLAG_FORCE_UNIFORM_SCALING: Force scaling this image uniformly. This flag
>+   * is for vector image only. A raster image should ignore this flag. This
>+   * flag prevents vector image with no fixed ratio from twisting itself after
>+   * the ratio is assigned.

This isn't clear enough -- it's kind of restating the name of the flag (first and 3rd sentence), and saying it's vector-specific.

In particular:
 * It's not clear enough (from this description) why/when this flag would actually take effect. Please clarify that a bit.
 * "for vector image only" -- VectorImage.* never use this flag at all. ClippedImage.cpp is the only place it's used (for *clipped* vector images).
 * "twisting" is probably the wrong word-choice here -- that implies "rotating", which isn't what you mean here. Maybe "distorting" would be a better choice?
 * "after the ratio is assigned" is unclear/confusing.

Please reword this to explain the scenario where this flag comes into play and why we need it a bit better.

Comment 89

[Daniel Holbert [:dholbert]]

Comment on attachment 8777637 [details] [diff] [review]
(Part 4) Add comment for FLAG_FORCE_UNIFORM_SCALING.

(er, sorry, meant to tag as r- for now; accidentally set r+)

Comment 90

[Kevin Chen[:kechen] (UTC + 8)]

Attachment: (Part 4) Add comment for FLAG_FORCE_UNIFORM_SCALING.:

Hi Daniel can you help me to review this again, thank you.

Comment 91

[Daniel Holbert [:dholbert]]

Comment on attachment 8781004 [details] [diff] [review]
(Part 4) Add comment for FLAG_FORCE_UNIFORM_SCALING.:

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

::: image/imgIContainer.idl
@@ +196,5 @@
>     * viewBox (if specified or implied by height/width attributes) exactly
>     * matches the viewport rectangle.
> +   *
> +   * FLAG_FORCE_UNIFORM_SCALING: Force scaling this image uniformly if
> +   * necessary. When painting border image, nsImageRenderer uses ClippedImage

This first sentence is too broad still. "Force scaling this image uniformly" isn't correct - there are lots of places where images are scaled (e.g. the imgIContainer::Draw API scales images, I'm pretty sure) -- but we don't check for this flag in most of those places.  We only pay attention to this flag in *one specific place* -- ClippedImage::OptimalSizeForDest.

How about something like the following text -- this makes more sense in my head, at least, and I think it's more precise about where this flag has an effect & why we need it (I think -- correct me if I'm misstating something):

 FLAG_FORCE_UNIFORM_SCALING: Signal to ClippedImage::OptimalSizeForDest that its returned size can only scale the image's size *uniformly* (by the same factor in each dimension).  We need this flag when painting border-image section with SVG image source-data, if the SVG image has no viewBox and no intrinsic size.  In such a case, we synthesize a viewport for the SVG image (a "window into SVG space") based on the border image area, and we need to be sure we don't subsequently scale that viewport in a way that distorts its contents by stretching them more in one dimension than the other.

Comment 92

[Kevin Chen[:kechen] (UTC + 8)]

Attachment: (Part 4) Add comment for FLAG_FORCE_UNIFORM_SCALING

Hi Daniel, very thank you for the modification. The version in your comment is more precise.

Comment 93

[Daniel Holbert [:dholbert]]

Comment on attachment 8783493 [details] [diff] [review]
(Part 4) Add comment for FLAG_FORCE_UNIFORM_SCALING

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

Thanks! r+

Comment 94

[Kevin Chen[:kechen] (UTC + 8)]

Please help me to checkin patch part 4, which only adds some comments to the modified codes.

Comment 95

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ec83da3f24a9
(Part 4) Add comment for FLAG_FORCE_UNIFORM_SCALING. r=dholbert

Comment 96

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ec83da3f24a9

Comment 97

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8767069 [details] [diff] [review]
(Part 1) Decouple SetImageOverridePreserveAspectRatio from mIsPaintingSVGImageElement setting

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

This needs another followup, I'm afraid.

::: layout/svg/SVGImageContext.h
@@ +33,4 @@
>      : mViewportSize(aViewportSize)
>      , mPreserveAspectRatio(aPreserveAspectRatio)
>      , mGlobalOpacity(aOpacity)
> +    , mIsPaintingSVGImageElement(aIsPaintingSVGImageElement)

So you added a new member variable...

@@ +55,4 @@
>    bool operator==(const SVGImageContext& aOther) const {
>      return mViewportSize == aOther.mViewportSize &&
>             mPreserveAspectRatio == aOther.mPreserveAspectRatio &&
>             mGlobalOpacity == aOther.mGlobalOpacity;

But you didn't add it to the operator==() implementation, so two SVGImageContexts with two different values for mIsPaintingSVGImageElement will compare equal. This is bad.

It's not strictly necessary, but you should've also mixed it into the hash value in Hash().

Comment 98

[Seth Fowler [:seth] [:s2h]]

Kevin, can you please write a followup to fix SVGImageContext::operator==() and SVGImageContext::Hash()? Whoever reviewed the patches in this bug should probably review the followup; I didn't read through the bug, but it looks like that might be Daniel.

Comment 99

[Daniel Holbert [:dholbert]]

Thanks for catching that, Seth! (Sorry for missing it in review)

Comment 100

[Kevin Chen[:kechen] (UTC + 8)]

Seth, thanks for the reminding! I've filed a followup bug for this and I will upload the patch later.

Bug number: bug 1298318

Comment 101

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

Hello! I managed to reproduce this issue on 48.0a1 (2016-04-14). Also, I investigated it on 
- Windows 10 x64
- Ubuntu 14.04 x86
- Mac OS X 10.11
using  
- latest Nightly 51.0a1 (2016-09-08) 
- latest Aurora 50.0a2 (2016-09-08)
- 49.0 build2 (20160907153016)

Indeed, 60 and 300 SVGs are fixed. 
No-size SVGs and 10 SVGs are still problematic, but based on comment 51, these issues are separately filed.
These being said, I will set the corresponding flags.