Eli Friedman Reporter
	Description • 18 years ago

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.

	tor Assignee
	Comment 2 • 18 years ago

Maybe not the cleanest approach, but preserves the common case unitless number fastpath.

	Robert Longson [:longsonr]
	Comment 3 • 18 years ago

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.

	Eli Friedman Reporter
	Comment 5 • 18 years ago

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.

	Jonathan Watt [:jwatt]
	Comment 7 • 17 years ago

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.

	Eli Friedman Reporter
	Comment 8 • 17 years ago

(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.)

	Jonathan Watt [:jwatt]
	Comment 9 • 17 years ago

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?

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 10 • 17 years ago

I agree.

	Jonathan Watt [:jwatt]
	Comment 11 • 17 years ago

(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.

	Eli Friedman Reporter
	Comment 12 • 17 years ago

IMO, we definitely want this for 1.9; nominating to get this on the radar.

	Eli Friedman Reporter
	Comment 13 • 17 years ago

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.

	Robert Longson [:longsonr]
	Comment 14 • 17 years ago

Applying the second part of the patch fixes print preview on Windows although actual printing of SVG still doesn't work for me.

	Jonathan Watt [:jwatt]
	Comment 16 • 17 years ago

I'm going to try to look at this properly tomorrow. Sorry I still haven't got to it.

	Jonathan Watt [:jwatt]
	Comment 17 • 17 years ago

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.

	Jonathan Watt [:jwatt]
	Comment 18 • 17 years ago

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.

	tor Assignee
	Comment 19 • 17 years ago

I think we've always assumed that CoveredRegion was device pixels - see nsSVGOuterSVGFrame::InvalidateRect.

	Robert Longson [:longsonr]
	Comment 21 • 17 years ago

WSere you going to ask for a review for this?

	Jonathan Watt [:jwatt]
	Comment 22 • 17 years ago

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"?

	Jonathan Watt [:jwatt]
	Comment 23 • 17 years ago

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.)

	Jonathan Watt [:jwatt]
	Comment 24 • 17 years ago

(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.

	jonathan chetwynd
	Comment 25 • 17 years ago

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

	tor Assignee
	Comment 26 • 17 years ago

Doesn't address all review comments yet, but getting closer to desired behavior.

	tor Assignee
	Comment 28 • 17 years ago

(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.

	Robert Longson [:longsonr]
	Comment 29 • 17 years ago

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. */

	Robert Longson [:longsonr]
	Comment 30 • 17 years ago

> Doesn't SeaMonkey still have text zoom? Will this change be OK there?

Scratch that first comment I clearly can't read.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 32 • 17 years ago

+  *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?

	tor Assignee
	Comment 34 • 17 years ago

Checked in.