264.77 KB, image/png
1.08 KB, text/html
39.97 KB, image/png
241.20 KB, image/tiff
1.06 KB, patch
|Details | Diff | Splinter Review|
Status: UNCONFIRMED → NEW
Component: Places → Layout
Ever confirmed: true
Product: Firefox → Core
QA Contact: places → layout
Version: unspecified → Trunk
I saw the same kind of problem reported for My Yahoo in the mozillazine forum. I reproduced it there but I couldn't figure the steps to reproduce that one reliably like this one.
Asking for blocking although this could be a wanted1.9(.0.x).
From Firebug I can see that the layout changes when an iframe has its display changed to block: iframesMenu[level].style.display = "block";
So I bet we reframe the IFRAME, which rebuilds the prescontext, which gets the default zoom factor. We probably just need to inherit the outer zoom factor into the inner in a more robust way, either in Places or internally in Gecko.
I can't reproduce this with a simple testcase. Could use a reduced testcase :-)
Related to bug 429402? But still the layout should not become so corrupted like that, when stuff gets zoomed out this way.
(In reply to comment #7) > Related to bug 429402? > But still the layout should not become so corrupted like that, when stuff gets > zoomed out this way. > Any chance you can get a reduced testcase? I'd like to see the risk of fix here to understand if we should take it in an RC2 or 0.x
I think the fix would be what roc said in comment 5. That would also prevent the layout corruption. I'll try to come up with a minimal testcase for that, though.
Created attachment 321432 [details] testcase 1. open testcase and change zoom to something other than default. 2. close tab with the tescase. 3. reopen testcase and press the go button.
Created attachment 321555 [details] [diff] [review] fix This fixes it. I think the patch is *reasonably* safe although I'd be skittish about taking it in an RC. I'm not sure who should review this.
Assignee: nobody → roc
Status: NEW → ASSIGNED
This doesn't seem to be widespread, and the fix seems slightly scary, so definitely not a ridealong. Wanted but not blocking.
I also confirmed this bug exists on MacOS 10.5: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008051304 Minefield/3.0pre I'll add a screen snap.
Comment on attachment 321555 [details] [diff] [review] fix I'll put this on Boris' queue, I guess.
So I'm trying to understand the flow of control here without this patch... If, instead of dynamically toggling display I instead dynamically insert an iframe into the DOM, the bug doesn't appear. Is that because the UI calls SetFullZoom on the outer page once the subframe loads? And in this case that doesn't happen because the child frame is already loaded by the time we toggle display? But I assume the UI is still calling SetFullZoom on the outer page at end of outer page load, which should be propagating down to our subframe, since the document viewer exists at that point. So it really seems like those cases should behave the same way, yet they do not. How come? As far as this change goes, would it make sense to inherit the zoom in docshell when setting up the new viewer, not just from the previous viewer but from the parent if there is no previous viewer? That is, is there a reason to be doing this as late as MakeWindow instead of picking up the zoom when we get created?
(In reply to comment #18) > So I'm trying to understand the flow of control here without this patch... > If, instead of dynamically toggling display I instead dynamically insert an > iframe into the DOM, the bug doesn't appear. Is that because the UI calls > SetFullZoom on the outer page once the subframe loads? I think so. > And in this case that doesn't happen because the child frame is already > loaded by the time we toggle display? Yeah. > But I assume the UI is still calling SetFullZoom on the outer page at end of > outer page load, which should be propagating down to our subframe, since the > document viewer exists at that point. Hmm. So the document viewer is created for the IFRAME when the IFRAME's subdocument loads, even though it's display:none? I didn't notice that. In that case, I don't know why the zoom wasn't inherited back then.
The document viewer should be created for the subframe, yes. We don't call Show() and don't create the prescontext until the subframe is shown, but the viewer should certainly be there well before that. I'm hesitant to take this patch as-is if we don't understand why this is broken to start with, to be honest...
Comment on attachment 321555 [details] [diff] [review] fix Yeah. I thought I understood, but I was wrong.
Created attachment 322214 [details] [diff] [review] better fix Much better fix to the root cause of the bug. The problem was that when we set up the documentViewer for the new document loading into the display:none IFRAME, we copy the full-zoom setting from the old documentViewer (an about:blank viewer) to the new documentViewer. And the old documentViewer's GetFullZoom call returns 1.0 because it has no prescontext. This fix is obvious, just fall back to mPageZoom from the documentViewer. I actually wonder why we're getting it from the prescontext at all, though, and not just returning the documentViewer's value always. Maybe that would be a better 1.9.1 patch.
> I actually wonder why we're getting it from the prescontext at all, Probably because of the print preview mess where we don't change the document viewer's members on set in print preview and just change the prescontext...
Comment on attachment 322214 [details] [diff] [review] better fix r+sr=bzbarsky with the comment adjusted to make sure people keep the print preview thing in mind when changing this code.
Created attachment 322215 [details] [diff] [review] better fix v2 OK thanks.
Roc is this safe enough for a 0.x or possible RC2?
I think it's very safe, RC2 material.
Comment on attachment 322215 [details] [diff] [review] better fix v2 a=beltzner, please land on cvs root asap
Attachment #322215 - Flags: approval1.9+
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
FWIW I can confirm that http://www.boursorama.com My Yahoo! page and the test case work just fine now on Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9pre) Gecko/2008052707 Minefield/3.0pre ID:2008052707
Yeah, verified fixed, using: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008052907 Minefield/3.0pre Tested with the testcase and http://www.boursorama.com/
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.