Closed Bug 401361 Opened 15 years ago Closed 14 years ago

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

Categories

(Core :: Layout, defect, P2)

x86
macOS
defect

Tracking

()

RESOLVED FIXED
mozilla1.9beta2

People

(Reporter: mossop, Assigned: roc)

References

Details

Attachments

(8 files, 2 obsolete files)

Attached image 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.
Does it still happen if the window is scrolled to its top-left?
The example screenshot is with the window scrolled to its top left.
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.
Attached image 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.
Attachment #286502 - Attachment is patch: false
Attachment #286502 - Attachment mime type: text/plain → image/png
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.
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.
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.
Attached file 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.
For reference this is the inner frame from the testcase.
I think we should block on this because it will break a bunch of extensions that use drawWindow.
Flags: blocking1.9?
(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.
(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?
Yeah, I think I found a scaling bug, I'm fixing it right now.
Attached patch fix (obsolete) — Splinter Review
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 :-).
Assignee: nobody → roc
Status: NEW → ASSIGNED
Attachment #286544 - Flags: superreview?(vladimir)
Attachment #286544 - Flags: review?(vladimir)
Attached image 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.
Attached patch updated fix (obsolete) — Splinter Review
Sorry, yes. We should not use the passed-in x/y to construct the clip-rect, of course. This fixes it.
Attachment #286544 - Attachment is obsolete: true
Attachment #286603 - Flags: superreview?(vladimir)
Attachment #286603 - Flags: review?(vladimir)
Attachment #286544 - Flags: superreview?(vladimir)
Attachment #286544 - Flags: review?(vladimir)
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.
Attachment #286603 - Flags: superreview?(vladimir)
Attachment #286603 - Flags: superreview+
Attachment #286603 - Flags: review?(vladimir)
Attachment #286603 - Flags: review+
Attachment #286603 - Flags: approval1.9+
Attached image 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.
Dare I ask for another testcase? :-)
Attached patch updated fix #2Splinter Review
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...
Attachment #286603 - Attachment is obsolete: true
Attachment #287380 - Flags: superreview?(vladimir)
Attachment #287380 - Flags: review?(vladimir)
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?
Attachment #287380 - Flags: superreview?(vladimir)
Attachment #287380 - Flags: superreview+
Attachment #287380 - Flags: review?(vladimir)
Attachment #287380 - Flags: review+
Yes, but we'd just scale by the fullZoom, so it would make considerably more sense.
Whiteboard: [needs approval/landing]
Flags: blocking1.9? → blocking1.9+
Whiteboard: [needs approval/landing] → [needs landing]
P2 because this will break some popular extensions
Priority: -- → P2
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
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing]
Target Milestone: --- → mozilla1.9 M10
Attached patch reftestSplinter Review
I'll check this in tomorrow.
Checked in the reftest.
Flags: in-testsuite+
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.
Actually I had to remove the line because it was causing other tests to fail. (Weird.)
Depends on: 448676
Depends on: 527804
You need to log in before you can comment on or make changes to this bug.