To test: 1. Run Fx on high-res screen, or set layout.css.dpi to 200 2. Go to http://www.croczilla.com/svg/samples/butterfly/butterfly.svg Expected: Butterly is scaled up. Actual: Butterfly is too small. I don't know the SVG code well enough to know how to fix this. Note that this doesn't apply to all SVG; the SVG in http://www.croczilla.com/svg/samples/events1/events1.xml gets scaled up correctly.
Created attachment 260366 [details] [diff] [review] adjust for css/device pixel difference Maybe not the cleanest approach, but preserves the common case unitless number fastpath.
Comment on attachment 260366 [details] [diff] [review] adjust for css/device pixel difference >+ mCoordCtxMmPerPx *= >+ float(nsIDeviceContext::AppUnitsPerCSSPixel()) / >+ float(context->AppUnitsPerDevPixel()); /= presContext->AppUnitsToFloatCSSPixels(PresContext()->AppUnitsPerDevPixel()) perhaps? >+ vb2vp->Scale(float(nsIDeviceContext::AppUnitsPerCSSPixel()) / >+ float(PresContext()->AppUnitsPerDevPixel()), >+ getter_AddRefs(mCanvasTM)); 1 / presContext->AppUnitsToFloatCSSPixels(PresContext()->AppUnitsPerDevPixel())? or some other prescontext function maybe. There are a number to choose from.
Created attachment 260834 [details] [diff] [review] adjust per suggestion
Attachment #260834 - Flags: review?(elifree) → review?(sharparrow1)
It seems like it would be much more straightforward to use something like: 25.4f * float(nsPresContext::AppUnitsPerCSSPixel()) / float(presContext->AppUnitsPerInch()); instead of calling GetScreenPixelToMillimeterX and correcting the result. I actually preferred the way you wrote the scale factor in the original version: (float(nsIDeviceContext::AppUnitsPerCSSPixel()) / float(PresContext()->AppUnitsPerDevPixel())); the corrected version requires an extra division for no good reason. I'll leave that up to you, though; it's not very important. I'm assuming that the results of GetCanvasTM aren't visible to script; that seems to be true, but I'm not completely sure. We don't want to be changing the way any script calculations happen that don't explicitly request the size a screen pixel.
Changing mCoordCtxMmPerPx to device pixels instead of CSS pixels will break things like nsSVGLength2::ConvertToUserUnits which expect GetMMPerPixel to be returning a mm per _CSS_ pixel (user unit) value. We'd then have the potential for scripts to act very strangly on high resolution screens.
(In reply to comment #7) > Changing mCoordCtxMmPerPx to device pixels instead of CSS pixels will break > things like nsSVGLength2::ConvertToUserUnits which expect GetMMPerPixel to be > returning a mm per _CSS_ pixel (user unit) value. We'd then have the potential > for scripts to act very strangly on high resolution screens. No, this patch puts mCoordCtxMmPerPx into CSS pixels, although in a very indirect way. Although, like I said earlier, "It seems like it would be much more straightforward to use something like: 25.4f * float(nsPresContext::AppUnitsPerCSSPixel()) / float(presContext->AppUnitsPerInch()); instead of calling GetScreenPixelToMillimeterX and correcting the result." The bigger issue here is whether the GetCanvasTM change is correct. (See bug 379616 comment 13 for background.) I'm not sure if it's the best approach, but it seems like it'll work (I suspect it breaks foreignobject, though.)
So let me think this through out loud. context->AppUnitsToFloatCSSPixels(context->AppUnitsPerDevPixel()) That evaluates to the number of CSS pixels per device pixel. It seems to me that if you _divide_ by that number, as the patch does, then you're converting from CSS pixels to device pixels. GetScreenPixelToMillimeterX returns the number of mm per device pixel, and if you multiply that by device pixels per CSS pixel, you get, uh, hmm, err, wow, mm per CSS pixel! Hmph. So that means our code is currently wrong and the patch is right. I have to say though, I've seen conversions that were easier to follow. :-P You know what, I think the correct way to fix this is to change GetScreenPixelToMillimeterX. The space says: readonly float screenPixelToMillimeterX User interface (UI) events in DOM Level 2 indicate the screen positions at which the given UI event occurred. When the user agent actually knows the physical size of a "screen unit", this attribute will express that information; otherwise, user agents will provide a suitable default value such as .28mm. DOM event coordinates are in CSS pixels, not device pixels, right? It seems clear then that we're doing the wrong thing by returning a mm per device pixel value here ("screen" unit seems to be a misleading term). We should be returning a mm per CSS pixel value for this to be of any use to anyone using the DOM. (Of course CSS and device pixels are typically the same, so nobody has noticed this bug yet.) Guess what, if we do that then GetScreenPixelToMillimeterX will give us the mm per CSS pixel value we needed all along. Thoughts?
(In reply to comment #8) > The bigger issue here is whether the GetCanvasTM change is correct. (See > bug 379616 comment 13 for background.) I'm not sure if it's the best approach, > but it seems like it'll work (I suspect it breaks foreignobject, though.) Yeah, if we were to apply the CSS pixel to device pixel conversion for gfxContext drawing by using the GetCanvasTM change in the patch then we'd need to reverse this scale when passing through foreignObject boundaries. I'm also not yet clear whether that's how we should do it though.
IMO, we definitely want this for 1.9; nominating to get this on the radar.
Comment on attachment 260834 [details] [diff] [review] adjust per suggestion With the fix to bug 390161, the first part of this patch is not needed. The second part is very SVG-specific, so I'm not really sure if it's the right thing to do; redirecting the review request so that an SVG peer can decide whether the method of scaling used is appropriate.
Attachment #260834 - Flags: review?(sharparrow1) → review?(jwatt)
Applying the second part of the patch fixes print preview on Windows although actual printing of SVG still doesn't work for me.
Created attachment 281330 [details] [diff] [review] second chunk only, updated to tip
I'm going to try to look at this properly tomorrow. Sorry I still haven't got to it.
Note that contrary to comment 0, http://www.croczilla.com/svg/samples/events1/events1.xml does not get scaled up correctly. That file consists of SVG embedded inline in XHTML, and while the text in the XHTML gets scaled, the embedded SVG circles do not.
Looks like you need to trace through the call paths and fix things as necessary. For example, nsSVGForeignObjectFrame::TransformPointFromOuterPx and nsSVGForeignObjectFrame::TransformPointFromOuter look like they would need to be changed to use nsPresContext::DevPixelsToAppUnits and nsPresContext::AppUnitsToDevPixels respectively (or whatever the names are). Also note that this patch in its current form would break getClientRects and getBoundingClientRect on high resolution devices since nsNSElementTearoff::TryGetSVGBoundingRect calls nsSVGUtils::GetOuterSVGFrameAndCoveredRegion. If GetCanvasTM is changed to include a device pixel to CSS pixel transform then nsSVGPathGeometryFrame::UpdateCoveredRegion (which calls nsSVGPathGeometryFrame::GeneratePath) and nsSVGForeignObjectFrame::UpdateCoveredRegion will both return device pixel rects instead of CSS pixel rects.
I think we've always assumed that CoveredRegion was device pixels - see nsSVGOuterSVGFrame::InvalidateRect.
Created attachment 286200 [details] [diff] [review] address review comments
WSere you going to ask for a review for this?
Comment on attachment 286200 [details] [diff] [review] address review comments The AppUnitsPerCSSPixel in nsSVGForeignObjectFrame::FlushDirtyRegion needs to be changed to device pixels. With this patch nsSVGPathGeometryFrame::GetBBox would be wrong since GeneratePath would then create a path in device pixels instead of CSS px. Similarly nsSVGGlyphFrame::GetBBox would be wrong since nsSVGGlyphFrame::GetGlobalTransform would then contain the CSS px to device pixel scaling. I'm not sure quite how to fix that. If you can't come up with a fix, then I think it's still worth taking this patch. At least then most SVG will be rendered correctly, and only SVG using getBBox in script will be slightly broken. >+ svgElement->GetViewboxToViewportTransform(getter_AddRefs(vb2vp)); >+ >+ vb2vp->Scale(1 / PresContext()->AppUnitsToFloatCSSPixels( >+ PresContext()->AppUnitsPerDevPixel()), >+ getter_AddRefs(mCanvasTM)); To make this easier to follow can you either split the unit conversion stuff out into a variable called devPxPerCSSPx, or else add a comment along the lines of "Scale the transform from CSS pixel space to device pixel space"?
Comment on attachment 286200 [details] [diff] [review] address review comments It's not entirely clear to me whether nsSVGPatternFrame::PaintPattern will do the right thing if nsSVGPatternFrame::GetCallerGeometry changes to output device pixels. If you can figure that out, please add a comment above the declaration of GetCallerGeometry saying what units it's outparams are in, and perhaps rename the variables where it's called so they don't look like they're in CSS px units. ("callerBBox" and "callerCTM" both say CSS px units to me.)
(In reply to comment #19) > I think we've always assumed that CoveredRegion was device pixels I've always assumed that everything was in CSS pixels, and so has a fair bit of the code. Since CSS pixels and device pixels have been equivalent on all the machines that I've developed on, I haven't noticed any problems. Clearly we need to do a far better job of documenting the units of function arguments and return values. In that vein... Please add this comment above GetCanvasTM in nsSVGContainerFrame.h: // Returns the transform to our gfxContext (i.e. to device pixels, not CSS px) Please add this comment above GetCoveredRegion in nsISVGChildFrame.h: // Get bounds in our gfxContext's coordinate space (i.e. in device pixels) Please add a comment above GetFrameForPointSVG saying what units x and y are in, and what exactly the point is relative to. Please add a comment above nsSVGUtils::HitTestClip saying what units x and y are in. Please add a comment above GetInvalidationRegion saying what unit the returned nsRect is in. For anyone who isn't familiar with the SVG code these comments could be very helpful.
I'm commenting here because this bug seems relevant and is 'live' zoom is currently working for many examples BUT the polarity is wrong ie the opposite of expectations http://peepo.com/svg/games.svg http://www.peepo.co.uk/index.svg on my system os x ppc nightlies 1440/900 however #1 re http://www.croczilla.com/svg/samples/events1/events1.xml the text scales, but not the SVG which is contrary to the comment. fwiw, I came to this from the 'fixed' XUL bug 275254 where again the text scales but not the svg in my dupe attachment. let me know if this should be in a separate bug, nb there are already a few svg zoom bugs
Created attachment 290612 [details] [diff] [review] more functional patch Doesn't address all review comments yet, but getting closer to desired behavior.
Created attachment 290613 [details] [diff] [review] leave old textZoom behavior, in case it get revived
Attachment #290612 - Attachment is obsolete: true
(In reply to comment #22) > (From update of attachment 286200 [details] [diff] [review]) > With this patch nsSVGPathGeometryFrame::GetBBox would be wrong since > GeneratePath would then create a path in device pixels instead of CSS px. > Similarly nsSVGGlyphFrame::GetBBox would be wrong since GetBBox actually works in most cases because we turn off matrix propagation before calling it. The couple cases where we don't for object space coordinates need further checking.
Comment on attachment 290613 [details] [diff] [review] leave old textZoom behavior, in case it get revived >Index: layout/svg/base/src/nsSVGGlyphFrame.cpp >=================================================================== >RCS file: /cvsroot/mozilla/layout/svg/base/src/nsSVGGlyphFrame.cpp,v >retrieving revision 1.115 >diff -u -8 -p -r1.115 nsSVGGlyphFrame.cpp >--- layout/svg/base/src/nsSVGGlyphFrame.cpp 21 Oct 2007 09:05:41 -0000 1.115 >+++ layout/svg/base/src/nsSVGGlyphFrame.cpp 29 Nov 2007 00:04:59 -0000 >@@ -117,17 +117,18 @@ nsSVGGlyphFrame::DidSetStyleContext() > const nsStyleFont* fontData = GetStyleFont(); > nsFont font = fontData->mFont; > > // Since SVG has its own scaling, we really don't want > // fonts in SVG to respond to the browser's "TextZoom" > // (Ctrl++,Ctrl+-) > nsPresContext *presContext = PresContext(); > float textZoom = presContext->TextZoom(); >- double size = presContext->AppUnitsToDevPixels(fontData->mSize) / textZoom; >+ double size = >+ presContext->AppUnitsToFloatCSSPixels(fontData->mSize) / textZoom; Doesn't SeaMonkey still have text zoom? Will this change be OK there? >Index: layout/svg/base/src/nsSVGUtils.h > /* Hit testing - check if point hits the clipPath of indicated >- * frame. Returns true of no clipPath set. */ >- >+ * frame. (x,y) are specified in device pixels relateive to the s/relateive/relative/ >+ * origin of the outer svg frame. Returns true of no clipPath s/of no/if no/ >+ * set. */
> Doesn't SeaMonkey still have text zoom? Will this change be OK there? Scratch that first comment I clearly can't read.
Created attachment 290905 [details] [diff] [review] fix clip/mask object bounding space, other review comments
+ *aOut = nsPoint(PRInt32(aX * PresContext()->AppUnitsPerDevPixel()), + PRInt32(aY * PresContext()->AppUnitsPerDevPixel())); Use NSToCoordRound here instead of the PRInt32 cast. It's worth loading AppUnitsPerDevPixel into a local variable because PresContext() isn't quite trivial. + TransformPointFromOuterPx(aPt.x / PresContext()->AppUnitsPerDevPixel(), + aPt.y / PresContext()->AppUnitsPerDevPixel(), Ditto. Looks like code in nsSVGClipPathFrame::GetCanvasTM and nsSVGMaskFrame::GetCanvasTM should be shared?
Created attachment 291362 [details] [diff] [review] adjust per comments
Attachment #291362 - Flags: superreview?(roc) → superreview+
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.