Closed
Bug 407959
Opened 17 years ago
Closed 17 years ago
Embedded SVG object disappears when zooming or in Print Preview
Categories
(Core :: SVG, defect, P2)
Core
SVG
Tracking
()
RESOLVED
FIXED
People
(Reporter: roc, Assigned: jwatt)
References
Details
Attachments
(2 files, 2 obsolete files)
(deleted),
image/svg+xml
|
Details | |
(deleted),
patch
|
tor
:
review+
roc
:
superreview+
|
Details | Diff | Splinter Review |
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.
Flags: blocking1.9?
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
Assignee | ||
Comment 2•17 years ago
|
||
The problem is caused by having a viewBox with a non-zero x or y component.
Assignee | ||
Updated•17 years ago
|
Assignee | ||
Updated•17 years ago
|
Assignee: nobody → jwatt
Assignee | ||
Comment 3•17 years ago
|
||
This is a problem in pure SVG. The SVG doesn't need to be embedded in HTML in any way.
Attachment #292741 -
Attachment is obsolete: true
Assignee | ||
Comment 4•17 years ago
|
||
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.
Assignee | ||
Comment 5•17 years ago
|
||
Attachment #292841 -
Flags: superreview?(roc)
Attachment #292841 -
Flags: review?(tor)
Reporter | ||
Comment 6•17 years ago
|
||
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?
Assignee | ||
Comment 7•17 years ago
|
||
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).
Reporter | ||
Comment 8•17 years ago
|
||
I don't like the global operator because we generally refrain from using that C++ feature.
Updated•17 years ago
|
Flags: blocking1.9? → blocking1.9+
Priority: -- → P2
Assignee | ||
Comment 9•17 years ago
|
||
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.
Reporter | ||
Comment 10•17 years ago
|
||
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.
Assignee | ||
Comment 11•17 years ago
|
||
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.
Reporter | ||
Comment 12•17 years ago
|
||
nsCOMPtr is special, it does a lot of tricky things we wouldn't allow anywhere else. Show me some other examples :-)
Assignee | ||
Comment 13•17 years ago
|
||
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
Reporter | ||
Comment 14•17 years ago
|
||
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?
Assignee | ||
Comment 15•17 years ago
|
||
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•17 years ago
|
||
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.
Assignee | ||
Comment 17•17 years ago
|
||
That's the same as calling nsIDOMSVGMatrix::Scale - the translation components mE and mF would be unaffected.
Assignee | ||
Comment 18•17 years ago
|
||
Um, or not.
Assignee | ||
Comment 19•17 years ago
|
||
Patch using Translate as suggested by longsonr.
Attachment #292841 -
Attachment is obsolete: true
Attachment #293014 -
Flags: superreview?(roc)
Attachment #293014 -
Flags: review?(tor)
Attachment #292841 -
Flags: superreview?(roc)
Attachment #292841 -
Flags: review?(tor)
Assignee | ||
Comment 20•17 years ago
|
||
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.
Reporter | ||
Comment 21•17 years ago
|
||
drawWindow is intentionally not scaling by fullZoom.
Reporter | ||
Updated•17 years ago
|
Attachment #293014 -
Flags: superreview?(roc) → superreview+
Attachment #293014 -
Flags: review?(tor) → review+
Assignee | ||
Comment 22•17 years ago
|
||
checked in
Status: NEW → RESOLVED
Closed: 17 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Reporter | ||
Comment 23•17 years ago
|
||
I backed this out because of test failures.
Reporter | ||
Updated•17 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 24•17 years ago
|
||
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.
Status: REOPENED → RESOLVED
Closed: 17 years ago → 17 years ago
Flags: in-testsuite+ → in-testsuite?
Resolution: --- → FIXED
Assignee | ||
Comment 25•17 years ago
|
||
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.
You need to log in
before you can comment on or make changes to this bug.
Description
•