Zooming breaks canvas-based graphs on mozilla-central, not on 1.9.1

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: johnath, Assigned: vlad)

Tracking

Trunk
x86
macOS
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

()

Attachments

(2 attachments)

The perf dashboard in the URL field uses flot to draw a set of 360x240 graphs. Because they don't all fit on the screen, I tend to view the dashboard zoomed out a couple notches using full page zoom.

In 1.9.1 this works fine, the graphs are all made appropriately smaller.  On mozilla-central, it looks like the grid behind the graphs is shrinking, but the lines themselves are not.  The graphs all overlap, and the result is pretty messy (see attachment).

Talked to jeff from gfx, and he suggested Layout was the right first stop.
Hrm, so what you actually mean is that the page layout is being zoomed, but the canvases themselves are not being zoomed.  I wonder if I broke this with bug 487693.
Yeah, pretty sure this is caused by this change:

-  nsIntSize canvasSize = GetCanvasSize();
-  nsSize sizeAppUnits(PresContext()->DevPixelsToAppUnits(canvasSize.width),
-                      PresContext()->DevPixelsToAppUnits(canvasSize.height));
+  nsIntSize sizeCSSPixels = GetCanvasSize();
+  nsSize sizeAppUnits(nsPresContext::CSSPixelsToAppUnits(sizeCSSPixels.width),
+                      nsPresContext::CSSPixelsToAppUnits(sizeCSSPixels.height));

GetCanvasSize() just returns the width/height of the canvas element (the attributes set on it), which are supposed to be in CSS pixels.

then we have:

 nsRect inner = GetInnerArea();
 gfxFloat sx = inner.width / (gfxFloat) sizeAppUnits.width;
 gfxFloat sy = inner.height / (gfxFloat) sizeAppUnits.height;

So we're basically never taking the pixel scale into account.   Do I then want to just get the pixel scale and multiply by that, rounding the final rect to int pixels?
Confirming that changing CSSPixelsToAppUnits back to DevPixelsToAppUnits fixes the problem, but I don't think it's the right way to do it.
RenderContexts expects the ctx to have one unit == 1 CSS pixel. But (assuming the nsHTMLCanvasFrame is the intrinsic size of the canvas), sx and sy are 1 and ctx will have one unit == 1 device pixel.

The most logical way to get that is probably to convert inner.width/height to device pixels and then divide that by the GetCanvasSize() in CSS pixels to compute sx/sy.
Posted patch fixSplinter Review
This works and seems more correct than the old approach.
Assignee: nobody → vladimir
Attachment #374784 - Flags: review?(roc)
Would be good to have a reftest here.
Pushed with reftest: http://hg.mozilla.org/mozilla-central/rev/69246843ecf4
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Had to disable the reftest on non-Mac, because canvas RenderContexts() calls PixelSnappedRectangleAndSetPattern() and then Fill(); I guess it should really set EXTEND_PAD on the pattern there?
You need to log in before you can comment on or make changes to this bug.