407959 - Embedded SVG object disappears when zooming or in Print Preview

Status: RESOLVED FIXED

(Core :: SVG, defect, P2)

Description

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Visit http://graphics.tudelft.nl/~jorik/svgtest/2007-12-11.html. Zoom in. Notice that the SVG object at the bottom of the page has disappeared. It also doesn't appear in print preview.

Comment 1

[jorik]

I've made a minimal version of the html page that reproduces the bug:

http://graphics.tudelft.nl/~jorik/svgtest/minimal.html

All files needed can also be downloaded here:

http://graphics.tudelft.nl/~jorik/svgtest/minimal.tgz

Comment 2

[Jonathan Watt [:jwatt]]

Attachment: testcase

The problem is caused by having a viewBox with a non-zero x or y component.

Comment 3

[Jonathan Watt [:jwatt]]

Attachment: testcase

This is a problem in pure SVG. The SVG doesn't need to be embedded in HTML in any way.

Comment 4

[Jonathan Watt [:jwatt]]

The problem is in nsSVGOuterSVGFrame::GetCanvasTM where we're post-multiplying the viewBox transform by devPxPerCSSPx using the SVG definition of scale matrix here:

  http://www.w3.org/TR/SVG11/coords.html#ScalingDefined

As a result the scale has no effect on the translate component of the viewBox transform. What we really want is to pre-multiply the viewBox transform by devPxPerCSSPx.

Comment 5

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think it would be better not to use a global overloaded operator here.

You need a comment explaining what the operator/function does do.

Can you make your testcase into a reftest?

Comment 7

[Jonathan Watt [:jwatt]]

Why do you dislike the global operator? I quite like it that way. I can surely add a comment and create a reftest (or mochitest if the zoom isn't accessible using reftest).

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I don't like the global operator because we generally refrain from using that C++ feature.

Comment 9

[Jonathan Watt [:jwatt]]

I think in this case it makes sense. We use member operator in other places, but since in this case the float needs to come first, it can't be a member, it has to be global. It also makes sense (and is a tasteful use of operator) from the pov that it's a well defined mathematical operation, whereby the result is produced by multiplying each component of the matrix by the pre-multiplying constant. I'm not sure "we generally refrain from this" is a good objection.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We try to limit the subset of C++ we use. I don't think we should increase that subset on a patch-by-patch basis. Please make it a function, this is not going to have much impact on you.

Comment 11

[Jonathan Watt [:jwatt]]

I would, except that I'm not increasing the subset of C++ we use. We already use global operator in many places, including core files such as nsCOMPtr.h.

http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/xpcom/glue/nsCOMPtr.h&rev=1.131&mark=1508,1517,1528,1536,1544,1552,1577,1585,1593,1601,1616,1625,1634,1643,1658,1667#1504

I can give you many other examples if you like.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

nsCOMPtr is special, it does a lot of tricky things we wouldn't allow anywhere else. Show me some other examples :-)

Comment 13

[Jonathan Watt [:jwatt]]

I don't get it. Why should nsCOMPtr be special? Surely your point was that you don't want to increase the compiler requirements, but if the compiler MUST support global operator for nsCOMPtr, what's the issue with using it elsewhere? My use of it is even lighter than the nsCOMPtr use since it doesn't involve a template (and therefore it's even more likely to be supported).

Nevertheless, I looked for some other instances of global operator. There are a ton of 'new' and 'delete' operator overloads, but I suspect you won't count them. :-P Here are the others I found:

http://mxr.mozilla.org/seamonkey/source/xpcom/string/public/nsTSubstringTuple.h#103
http://mxr.mozilla.org/seamonkey/source/xpcom/string/public/nsStringIterator.h#340
http://mxr.mozilla.org/seamonkey/source/xpcom/string/public/nsTSubstring.h#702
http://mxr.mozilla.org/seamonkey/source/xpcom/ds/nsTime.h#111
http://mxr.mozilla.org/seamonkey/source/xpcom/ds/nsInt64.h#368
http://mxr.mozilla.org/seamonkey/source/xpcom/ds/nsCppSharedAllocator.h#112
http://mxr.mozilla.org/seamonkey/source/xpcom/base/nsAutoPtr.h#334
http://mxr.mozilla.org/seamonkey/source/parser/htmlparser/public/nsScannerString.h#533

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

No, the goal is to minimize the amount of C++ knowledge that developers have to know/remember.

Those are all low-level base types really. I'm not convinced. Ask Brendan?

Comment 15

[Jonathan Watt [:jwatt]]

Interesting - this is the first time I've heard anyone give that reason. I'm not sure I buy it in this case (which I consider simple and intuitive), but then it's years since I first encountered operator overloading so perhaps I've forgotten a confusing learning curve.

Anyway, the reason I was being stubborn is because I believe devs must at least understand the reasons they're being asked to make a change to their patch, even if they don't agree with the change. Thanks for your patience in getting to that point. In this case the actual change to a function I can live with so I'll attach a new patch shortly.

Comment 16

[Robert Longson [:longsonr]]

Why not just create a nsIDOMSVGMatrix with the appropriate scale e.g.

nsCOMPtr<nsIDOMSVGMatrix> scale;
NS_NewSVGMatrix(getter_AddRefs(scale),
                devPxPerCSSPx, 0.0f,
                0.0f, devPxPerCSSPx);

Then you can do a pre-multiply using this matrix. No changes to nsIDOMSVGMatrix would be required.

Comment 17

[Jonathan Watt [:jwatt]]

That's the same as calling nsIDOMSVGMatrix::Scale - the translation components mE and mF would be unaffected.

Comment 18

[Jonathan Watt [:jwatt]]

Um, or not.

Comment 19

[Jonathan Watt [:jwatt]]

Attachment: patch plus reftest

Patch using Translate as suggested by longsonr.

Comment 20

[Jonathan Watt [:jwatt]]

I'm wondering if the output from nsIDOMCanvasRenderingContext2D::drawWindow shouldn't be scaled by fullZoom (it's not). Nevertheless, the testcase still works in that it breaks (the ellipse is offset) without this patch. It just isn't scaled in the raster.

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

drawWindow is intentionally not scaling by fullZoom.

Comment 22

[Jonathan Watt [:jwatt]]

checked in

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I backed this out because of test failures.

Comment 24

[Jonathan Watt [:jwatt]]

I checked the code back in without the reftest this time and it caused no problems.

I then modified zoomed-svg-with-viewBox-01.svg to set fullZoom onload as reftests/bugs/401361-1.xul does. For some reason the test still causes itself and the same four other reftests to fail. Four of these are a failure to *load*, and one is a flat failure (it renders red instead of green).

REFTEST UNEXPECTED FAIL (LOADING):
reftests/svg/moz-only/zoomed-svg-with-viewBox-01.svg
reftests/svg/foreignObject-ancestor-style-change-01.svg
reftests/svg/foreignObject-change-transform-01.svg
reftests/svg/foreignObject-move-repaint-01.svg

REFTEST UNEXPECTED FAIL:
reftests/svg/foreignObject-display-01.svg

Strangely this seems to happen on the Windows, Mac and Linux tinderboxes, but I have no problems running the reftests on my own machine.

Comment 25

[Jonathan Watt [:jwatt]]

I bet this is because I'm not resetting the value of fullZoom to 1. I'm not really sure now to handle that though, since reftests doesn't have facilities for a callback to execute some code _after_ the drawWindow has occurred.