512367 - strange behaviour when user clicks to zoom an image in an iframe on a page that is being full paged zoomed

Status: RESOLVED FIXED

(Core :: Web Painting, defect, P2)

Description

[Mike Beltzner [:beltzner, not reading bugmail]]

STR:

1. Visit https://bugzilla.mozilla.org/attachment.cgi?id=395281&action=edit
2. Click to zoom the screenshot that's in the attachment details frame

Expected: scrollbars at the edge of the iframe
Actual: scrollbars are inset, scrolling the zoomed image leaves artifacts (see screenshot)

Think this might be OSX only

Comment 1

[Mike Beltzner [:beltzner, not reading bugmail]]

Attachment: screenshot of problem

Comment 2

[Mike Beltzner [:beltzner, not reading bugmail]]

Ah, this seems related to full-page zoom.

STR:
0. Visit bugzilla and hit cmd/ctrl+- a bit to zoom out the full page
1. Visit https://bugzilla.mozilla.org/attachment.cgi?id=395281&action=edit
2. Click to zoom the screenshot that's in the attachment details frame

Comment 3

[Mike Beltzner [:beltzner, not reading bugmail]]

Confirmed that this is a regression from Firefox 3; needs a full window, though. Maybe look around a month ago?

Comment 4

[Martijn Wargers (dead)]

Related to bug 495894?

Comment 5

[Mike Beltzner [:beltzner, not reading bugmail]]

Might be related, but that got duped to a bug that was present in Firefox 3.5, and I can't reproduce this in Firefox 3.5.2.

Comment 6

[Boris Zbarsky [:bzbarsky]]

OK, here's what I see:

2009-07-21-03 build (rev f2a58ffcd00c): Behavior as described in bug 475574 (weird zooming, scrollbars not invalidating right, but painting at the right place).

2009-07-22-03 build (rev 02f8bf10f441): Behavior as described in this bug.  Nice guess on "about a month ago".  ;)

Pushlog range: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=f2a58ffcd00c&tochange=02f8bf10f441

The obvious culprit in that range is bug 352093, which would just affect exactly which widget the bogus invalidates/paints are happening on, presumably...  The rest would be bug 475574.

I wonder whether this is just something that happens any time a page has a subframe with a different zoom level (so parts of the painting are using one zoom level and parts are using another one)...  That's exactly the situation with nsImageDocument, since it resets the zoom level.  When we apply site-specific zoom, do we just use the toplevel page setting and apply it to the , or can we set different frames to different values?  If the latter, this would give a non-imagedocument way of testing the above hypothesis.

Comment 7

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

Right, the problem is likely that we assume appunits-per-dev-pixel is constant for all frames in a display list, but it is not. This assumption is implicit in everything that compares geometry across display list items. It's also implicit in nsIFrame::GetOffsetTo, for what it's worth...

This is not trivial to fix. Best thing to do here is probably to make nsSubdocumentFrame::BuildDisplayList detect when appunits-per-dev-pixel is different in the subdocument and in that case create a special display item which renders the subdocument as an atomic unit, much like nsDisplayTransform does.

Comment 8

[Boris Zbarsky [:bzbarsky]]

That would probably be the robust fix, yeah...  We _might_ be able to fix this specific case by making nsImageDocument not mess with the zoom level if it's framed; that might be just as much work as the real fix, though, and won't help for extensions that zoom random subframes for some reason.

Comment 9

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

The real reason to do the fix in comment #7 is that when we unify all the frame trees, we'll need this fix to for full zoom of content windows to work.

Comment 10

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

Actually fixing it that way seems like a bad idea. We really do want to be able to inspect the display list geometry of zoomed subframes, so we can scroll with with bitblit scrolling and position and clip plugins in those subframes.

I think we may need to make all coordinates that are "relative to the reference frame" actually be scaled and converted correctly from the subframe to the coordinate system of the reference frame. That may be painful, I'll have to see how painful.

Comment 11

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

Attachment: fix

This fix just does what Boris suggested in comment #8: don't reset the zoom level if we're in a subframe and we toggle from zoomed-in to zoomed-out of vice versa. I think this is better behaviour, usually it's clear to the user that the containing document remains zoomed so there is no reason to unzoom the iframe.

The other thing I've done here is that in CheckOverflowing I don't want mVisibleWidth/mVisibleHeight to be altered to account for the zoom level, I'm leaving them as the true visible area. That's definitely what we want for subframes given the above change. I'm not sure what it's trying to achieve for top-level image documents. Tests pass anyway.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 405978 [details] [diff] [review]
fix

OK, works in the testcases I tried.

Comment 13

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

Checked in:
http://hg.mozilla.org/mozilla-central/rev/af032ca9e9ec

This may have caused bug 522640, but probably not.

Comment 15

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

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/8755440fb2a4