401361 - drawWindow on a window with pagezoom fails to draw content that would normally be hidden

Status: RESOLVED FIXED

(Core :: Layout, defect, P2)

Description

[Dave Townsend [:mossop]]

Attachment: screenshot

When pagezoom has been applied to a browser using drawWindow to draw its contents to a canvas fails to draw the entire selected area. See the attached screenshot which shows the webpage on the right and a scaled preview of it on the left. The right hand side of the page is missing from the preview. This area is the area that would not be visible in the page if pagezoom was not applied. Also something strange goes on at the bottom of the preview, it's like it is only rendering the table blocks that would be completely in view without zoom, but happily drawing those in full.

Comment 1

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

Does it still happen if the window is scrolled to its top-left?

Comment 2

[Dave Townsend [:mossop]]

The example screenshot is with the window scrolled to its top left.

Comment 3

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

No, in that screenshot the window (the *real* window) is scrolled to the right.

If this is something that's going to affect Firefox 3 UI then you'd better get blocking flags on it or I won't be able to work on it.

Comment 4

[Dave Townsend [:mossop]]

Attachment: screenshot 2

No, look at the horizontal scrollbar for the window, its all the way over to the left. My apologies for using a style hack hiding the tinderbox side panel that may be confusing the issue. Here is a screenshot without it.

I thought there was a plan to have tab previews in Firefox 3, I'll see if I can search out the bug. Otherwise perhaps if you have a rough guess of where I might go looking into this I might be able to find some spare time to debug it.

Comment 5

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

What are you using to get the width and height of the area to draw?

It would be useful to have a reduced testcase, like an <iframe> containing a document with 10 horizontal 100x100 pixel <img>s (so at least one is fully scrolled off to the right), then chrome to zoom in the <iframe> by 200% and then do an iframe.drawWindow(0,0,1000,100). That would help narrow down where the bug lies.

Comment 6

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Note that there's a bug with drawWindow when there's a scale on the -canvas- (you have to apply an inverse scale to the source rect, even though you shouldn't need to), but I don't think that's happening here.  The odd thing here is that there are entire elements missing, it's not just clipping (e.g. there's a bunch of dark gray chunks missing from the bottom of the tinderbox display below some other columns).

Either way, a testcase would be helpful.

Comment 7

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

Those elements being dropped indicates that clipping is OK but the dirty rect is wrong.

This probably isn't hard to diagnose and fix. I suspect we're just using the wrong prescontext to do appunits-to-dev-pixel conversions somewhere.

Comment 8

[Dave Townsend [:mossop]]

Attachment: testcase

This is a testcase demonstrating the issue, it requests elevated privileges so save it locally before use. Also ignore the odd stuff outside the top frame, that is bug 401543.

The testcase has an iframe at the top and a canvas at the bottom, both are 100x100. On load the iframe is zoomed to 0.666666 then painted into the canvas.

The drawWindow call passes a width and height of 100 which appears to be correct and the elements that render appear the correct size in the canvas.

The page in the iframe contains 4 tables that generate the following:

1 block 100px wide, this is drawn correctly in full in the canvas.
3 blocks (green, black, blue), 50px wide each. The first two are drawn in the canvas the last isn't.
1 block 150px wide, this is drawn correctly in full in the canvas.
3 blocks, greed is 50px, black is 49px, blue is 51px. All are drawn in full in the canvas.

So we're asking to draw 100x100 from the source window, it looks like it knows to scale that up to the 150x150 css area that is visible given how the end rendering is, but then it looks like its only painting elements that appear in the first 100x100 area of that.

Comment 9

[Dave Townsend [:mossop]]

Attachment: inner frame from testcase

For reference this is the inner frame from the testcase.

Comment 10

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

I think we should block on this because it will break a bunch of extensions that use drawWindow.

Comment 11

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

(In reply to comment #8)
> So we're asking to draw 100x100 from the source window, it looks like it knows
> to scale that up to the 150x150 css area that is visible given how the end
> rendering is, but then it looks like its only painting elements that appear in
> the first 100x100 area of that.

Hmmm.

I had intended that the coordinates passed to drawWindow be interpreted in the window's coordinate system. Therefore in this testcase you would pass 150x150 to capture the entire window contents. Note that the x,y were always interpreted relative to the window's origin, so the scale should be too IMHO. Is that a bad API?

If that API choice is OK, then the bug here is really that the clipping is wrong. In fact as far as I can tell drawWindow/nsPresShell::RenderDocument don't clip at all; probably RenderDocument needs some clippping code.

Comment 12

[Dave Townsend [:mossop]]

(In reply to comment #11)
> I had intended that the coordinates passed to drawWindow be interpreted in the
> window's coordinate system. Therefore in this testcase you would pass 150x150
> to capture the entire window contents. Note that the x,y were always
> interpreted relative to the window's origin, so the scale should be too IMHO.
> Is that a bad API?

No that makes sense, when I first saw this problem my immediate thought was that I had to adjust the width/height according to the zoom. However if that was the case then drawing the 100x100 window area should draw it in the 100x100 of the canvas, yet right now it is drawing it scaled into the canvas, or is that related to comment 6?

Comment 13

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

Yeah, I think I found a scaling bug, I'm fixing it right now.

Comment 14

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

Attachment: fix

Take this for a spin.

The major problem here is that nsPresShell::RenderDocument is constructing an nsThebesRenderingContext to wrap the passed-in gfxContext, and that gets set up to scale the rendering according to the full-zoom of the presshell. But that's not what we want, we shouldn't be doing any scaling by default on this path. So I have to add an nsIRenderingContext API to control the scaling, in this case setting it to the no-zoom value.

The secondary problem is that nsPresShell::RenderDocument should clip its rendering to the requested rectangle. This will also help performance when people draw thumbnails to large canvases because we'll restrict the size of the PushGroup.

I just realized I need to rev the IID of nsIRenderingContext. Assume I'll do that :-).

Comment 15

[Dave Townsend [:mossop]]

Attachment: clipping fault

That looks like it sorts out the scaling right, even when there is also scaling on canvas I see what I expect from the testcase. However there is something not quite right with the clipping I think.

This screenshot is drawing a 100x100 pixel area from 25x0. I would expect the 25px gap on the left of the result to contain red-green-yellow-green blocks.

Comment 16

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

Attachment: updated fix

Sorry, yes. We should not use the passed-in x/y to construct the clip-rect, of course. This fixes it.

Comment 17

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 286603 [details] [diff] [review]
updated fix

Looks good; approving for 1.9 post-M9, though if you think it should go into M9 set a ? on there.

Comment 18

[Dave Townsend [:mossop]]

Attachment: updated screenshot

Sorry to say that there is still something not quite right with the previous patch applied. Hopefully you can make it out in the screenshot but in the preview the table borders and text appears to be displayed at a different zoom level to the table backgrounds.

Comment 19

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

Dare I ask for another testcase? :-)

Comment 20

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

Attachment: updated fix #2

Okay, we need a new approach here. Monkeying with the appunits conversion factor in the nsIRenderingContext doesn't work 100%, because stuff like textruns has its own knowledge of the appunits conversion factor. Instead, give the nsIRenderingContext the usual conversion factor and just use a straight Scale to account for the mismatch.

This would be easier if layout's gfxContext CTMs were set up for the appunit coordinate space instead of the "device unit" coordinate space...

Comment 21

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 287380 [details] [diff] [review]
updated fix #2

Looks fine to me; it would be easier but you'd still need to keep the same Scale() call, no?

Comment 22

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

Yes, but we'd just scale by the fullZoom, so it would make considerably more sense.

Comment 23

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

P2 because this will break some popular extensions

Comment 24

[Reed Loden [:reed]]

Checking in layout/base/nsIPresShell.h;
/cvsroot/mozilla/layout/base/nsIPresShell.h,v  <--  nsIPresShell.h
new revision: 3.217; previous revision: 3.216
done
Checking in layout/base/nsPresShell.cpp;
/cvsroot/mozilla/layout/base/nsPresShell.cpp,v  <--  nsPresShell.cpp
new revision: 3.1068; previous revision: 3.1067
done

Comment 25

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

Attachment: reftest

I'll check this in tomorrow.

Comment 26

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

Checked in the reftest.

Comment 27

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

The reftest failed on Tinderbox and caused weird cascading failures of other tests. I marked it random while I figure out what the problem is.

Comment 28

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

Actually I had to remove the line because it was causing other tests to fail. (Weird.)