layout breaks on zoomed pages with dynamic menus accessed via places

VERIFIED FIXED

Status

()

Core
Layout
VERIFIED FIXED
10 years ago
9 years ago

People

(Reporter: Luc Attimont, Assigned: roc)

Tracking

(Blocks: 1 bug)

Trunk
x86
Windows XP
Points:
---
Bug Flags:
wanted-next +
blocking1.9 -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [RC2+], URL)

Attachments

(5 attachments, 2 obsolete attachments)

(Reporter)

Description

10 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008051403 Minefield/3.0pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008051403 Minefield/3.0pre

dynamic menus on the left of the page behave correctly on http://www.boursorama.com/, even if the page is zoomed in 

if page is accessed via places, the zoom level is remembered, and the page appears already zoomed

as soon as i move my mouse above the dynamic menus on the left, the layout breaks (this is probably more a javascript or Gecko problem than a Places problem, please correct me).



Reproducible: Always

Steps to Reproduce:
1.open a new tab and go to http://www.boursorama.com/
2.zoom in, at least once, to see a bigger page
3.close the tab
4.enter for example www.boursorama: places suggests http://www.boursorama.com/, open it: the page appears already zoomed
5.move your mouse above the menus on the left: the layout is completely corrupted, because the dynamic menus are not zoomed
Actual Results:  
layout deeply corrupted

Expected Results:  
correct layout when displaying dynamic menus, as when the page is zoomed for the first time.

this problem occurs on FF3b5 and all tested minefield levels:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008050503 Minefield/3.0pre
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008051403 Minefield/3.0pre

this problem do not occurs on FF2.0.0.12, IE7, Safari 3.0.4 (and Safari remembers the zoom level)

Updated

10 years ago
Status: UNCONFIRMED → NEW
Component: Places → Layout
Ever confirmed: true
Product: Firefox → Core
QA Contact: places → layout
Version: unspecified → Trunk

Comment 1

10 years ago
Created attachment 321085 [details]
Screenshot of the result

Comment 2

10 years ago
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.  

Comment 3

10 years ago
Asking for blocking although this could be a wanted1.9(.0.x). 
Flags: wanted1.9.0.x?
Flags: blocking1.9?

Comment 4

10 years ago
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.

Comment 8

10 years ago
(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.

Comment 10

10 years ago
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.

Comment 11

10 years ago
Created attachment 321434 [details]
result screenshot
Thanks Brian!
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.
Flags: wanted1.9.0.x?
Flags: wanted1.9.0.x+
Flags: wanted-next+
Flags: blocking1.9?
Flags: blocking1.9-

Comment 15

10 years ago
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 16

10 years ago
Created attachment 321810 [details]
Snap showing corruption after zoom
Comment on attachment 321555 [details] [diff] [review]
fix

I'll put this on Boris' queue, I guess.
Attachment #321555 - Flags: superreview?(bzbarsky)
Attachment #321555 - Flags: review?(bzbarsky)
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.
Attachment #321555 - Attachment is obsolete: true
Attachment #321555 - Flags: superreview?(bzbarsky)
Attachment #321555 - Flags: review?(bzbarsky)
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.
Attachment #322214 - Flags: superreview?(bzbarsky)
Attachment #322214 - Flags: review?(bzbarsky)
> 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.
Attachment #322214 - Flags: superreview?(bzbarsky)
Attachment #322214 - Flags: superreview+
Attachment #322214 - Flags: review?(bzbarsky)
Attachment #322214 - Flags: review+
Created attachment 322215 [details] [diff] [review]
better fix v2

OK thanks.
Attachment #322214 - Attachment is obsolete: true
Attachment #322215 - Flags: superreview+
Attachment #322215 - Flags: review+

Comment 26

10 years ago
Roc is this safe enough for a 0.x or possible RC2?
Whiteboard: [RC2?]
I think it's very safe, RC2 material.
Whiteboard: [RC2?] → [RC2+]
Comment on attachment 322215 [details] [diff] [review]
better fix v2

a=beltzner, please land on cvs root asap
Attachment #322215 - Flags: approval1.9+
Checked in
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED

Comment 30

10 years ago
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

Updated

10 years ago
Blocks: 429402

Updated

10 years ago
Duplicate of this bug: 422319
Flags: wanted1.9.0.x+
You need to log in before you can comment on or make changes to this bug.