403565 - Old page gets resized when browsing to a new one

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect, P3)

Description

[Dennis Jacobfeuerborn]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b2pre) Gecko/2007111204 Minefield/3.0b2pre
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b2pre) Gecko/2007111204 Minefield/3.0b2pre

When I enter a new URL or click a bookmark the old page that is currently displayed gets resized for a short moment before the new one gets displayed.

Reproducible: Always

Steps to Reproduce:
1. Load page
2. Load another page

Actual Results:  
The old page gets resized a moment before the new one appears.

Expected Results:  
The old page shouldn't change at all.

This doesn't happen immediately but only when Firefox begins to render the new page. This effect is easier to see when the bandwidth is in short supply so that the loading of the new page gets slowed down. On a fast and free connection the transition might happen too fast so that the effect is hard to notice.

Comment 1

[Dennis Jacobfeuerborn]

Attachment: This is a page in its normal state.

Comment 2

[Dennis Jacobfeuerborn]

Attachment: Page just before the new one appears.

This is the same page as before just after it got resized as described in the bug and just before the next page appears. The visual effect is that of a zoom (sometimes in and sometimes out) but sometimes the pages get resized in really buggy ways with lots of rendering errors.

Comment 3

[Dennis Jacobfeuerborn]

I'm no longer seeing this in the latest nightly.

Comment 4

[Dennis Jacobfeuerborn]

Scratch that. The problem is still there.

Comment 5

[Sylvain Pasche]

I can confirm the issue with today's nightly.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Does this have something to do with site-specific preferences and/or page zoom?  There are comments hinting at both in bug 406577.

Comment 8

[Sylvain Pasche]

Yes, this has something to do with page zoom.

I see now that it is not clear from the description. The steps to reproduce would be:

1) Go to site A and zoom the page
2) Click on a link which goes to another domain B
3) See how the original page is first redrawn smaller before the new one appears (as in the attached screenshot)

Comment 9

[Dennis Jacobfeuerborn]

Yes, it looks like the old page first gets zoomed to its "normal" size before the new one is displayed.

Since Firefox remembers the zoom state for each page is there a way to see/edit/reset the page specific settings? If not that would maybe be a good addition to the "View Page Info" dialog.

Comment 10

[ourasi]

(In reply to comment #7)
> Does this have something to do with site-specific 
> preferences and/or page zoom?
> There are comments hinting at both in bug 406577.

Isn't page zoom saved to site-specific preferences?

(In reply to comment #9)
> Yes, it looks like the old page first gets zoomed to its "normal" size before
> the new one is displayed.
> 

I think it is zoomed to new sites size, and this happens in both ways.

As I tried to explain in my bug report 406577

If you go from more zoomed page to lesser zoomed

1. Firefox first draws some smaller frame within page window
2. Fills that with smaller text from original site (obviously zoomed to new page's zoom factor) 
3. Draws after that new page (and this can take many seconds)

If you go from less zoomed page to more zoomed

1. Firefox draws bigger frame, obviously extending outside page window 
2. Fills the page with bigger text from original site (maybe zoomed to new sites zoom percentage) 
3. Draws then new page

I can very clearly see this happening in my slow computer and it should be visible in faster computers with that test case I gave.

Something similar happens maybe (much faster?) in Windows. I can see it only if I stop page loading with that security warning question AND take a screen shot. 

This can be seen in my last picture:
http://i20.photobucket.com/albums/b239/ourasi/Win_zoom2.png

Comment 11

[Damon Sicore (:damons)]

Myk, can you take a look at this and determine if this is a layout bug?

Comment 12

[Myk Melez [:myk] [@mykmelez]]

I see the behavior in comment 10.  I think what's happening is that the page zoom controller is reacting to the location change event by setting the zoom for the browser to the value for the new page before the layout engine has torn down the old page, and since zoom changes get applied immediately, the zoom change thus gets applied to the old page.

As I understand it, we intentionally don't tear down the old page on location change, since that would result in a jerky transition from old page to blank page to new page rather than directly from old page to new page.  So I'm not sure what the solution is here.  Maybe we can define an additional event that fires right before the layout engine starts rendering the new page, and the page zoom controller can set the zoom at that point.

Comment 13

[Sylvain Pasche]

Attachment: pausing server for testing

This attachment is a small python script with .html files to reproduce the issue more easily. The webserver will pause when it sends the .html files, so that the redrawing during the page transitions at different zoom levels is more visible. To launch this testcase:
* extract the archive somewhere
* go in that directory
* launch with "python server.py"
* Go to http://localhost:8000 and follow the instructions.

One strange thing to notice is that when the old page is redrawn at the new page zoom level the text is not painted, only form controls or borders.

Instead of changing the zoom level in the onLocationChange handler, it could be set when the pageshow event is fired. This will prevent the old page being resized. The side effect is that the new page zoom level will only be changed once that event is dispatched, so that it will be drawn at the current zoom level during load.

Comment 14

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #13)
> Instead of changing the zoom level in the onLocationChange handler, it could be
> set when the pageshow event is fired. This will prevent the old page being
> resized. The side effect is that the new page zoom level will only be changed
> once that event is dispatched, so that it will be drawn at the current zoom
> level during load.

Yeah, we could do that.  Unfortunately that side-effect seems worse than the current problem, since it causes the content you want to see now to jump after it has been loaded, probably after you are already beginning to browse it (and perhaps after you've tried to increase its zoom level not knowing that a zoom level increase is already pending).

I think we're really wanting an intermediate event between locationchange and pageshow, something that says "the old page has been torn down, and the new one is about to be built up."

Comment 15

[Dennis Jacobfeuerborn]

(In reply to comment #14)
> I think we're really wanting an intermediate event between locationchange and
> pageshow, something that says "the old page has been torn down, and the new one
> is about to be built up."

That sounds like the proper way to do this. I'm also wondering about the performance impact of this re-rendering of pages. An intermediate event should make it possible to not re-render the old page and render the new one with the correct zoom factor right away thus avoiding any unnecessary work.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Could you ignore zoom changes in zombie viewers (and propagate them forward to the viewer which is being paint-suppressed; I assume this already happens, since it does work on the new page).

Or is the zoom change not coming through the document viewer?

Alternately, we could dispatch a notification when we Show() the new viewer, but doing that could be a bit scary...

Comment 17

[Myk Melez [:myk] [@mykmelez]]

The zoom change does come through the document viewer.  FullZoom in browser-textZoom.js calls ZoomManager in viewZoomOverlay.js, which sets the zoom via:

  getBrowser().markupDocumentViewer.fullZoom = aVal

How/where might we detect a zombie viewer and propagate the zoom change forward?

Comment 18

[Boris Zbarsky [:bzbarsky]]

When you're making that call, what is getBrowser().markupDocumentViewer.previousViewer?

Looking at the code again, I'm not sure how the zoom currently ends up happening on the new document if it's being applied to the old document...

Comment 19

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #18)
> When you're making that call, what is
> getBrowser().markupDocumentViewer.previousViewer?

It's "undefined" in every case, but I don't see that property in the nsIMarkupDocumentViewer interface, so perhaps it isn't exposed to chrome.

> Looking at the code again, I'm not sure how the zoom currently ends up
> happening on the new document if it's being applied to the old document...

Text zoom used to work the same way: the zoom you set was applied persistently to every document loaded into that tab.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Gah.  previousViewer is noscript (on nsIContentViewer) for no good reason I can think of.  OK.

Quite honestly... I'm pretty sure that OnLocationChange fires right before we set the new viewer on the docshell.  Does setting the zoom off a 0-msec timeout work, by any chance?

Comment 21

[Myk Melez [:myk] [@mykmelez]]

Attachment: test patch: sets zoom in timeout

(In reply to comment #20)
> Quite honestly... I'm pretty sure that OnLocationChange fires right before we
> set the new viewer on the docshell.

Is the zoom value really a property of the viewer as opposed to the docshell itself?  The latter fits the behavior better, even though we're setting the value through the viewer, given that the value persists across viewers.

> Does setting the zoom off a 0-msec timeout work, by any chance?

Here's a patch that sets the zoom in a 0ms timeout.  Unfortunately, this doesn't fix the problem.

Comment 22

[Boris Zbarsky [:bzbarsky]]

> Is the zoom value really a property of the viewer as opposed to the docshell

Yes.

> given that the value persists across viewers.

See http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/docshell/base/nsDocShell.cpp&rev=1.878&mark=6301-6305#6287

Out of curiosity, in your case, what's the order in which nsDocShell::SetCurrentURI(), your SetFullZoom(), and the code in nsDocShell::SetupNewViewer() run?

Comment 23

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #22)
> See
> http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/docshell/base/nsDocShell.cpp&rev=1.878&mark=6301-6305#6287

Ah, indeed.  Hmm, I wonder if it's still necessary to copy the page zoom state now that we have chrome code that updates it for each page load.


> Out of curiosity, in your case, what's the order in which
> nsDocShell::SetCurrentURI(), your SetFullZoom(), and the code in
> nsDocShell::SetupNewViewer() run?

When loading the new page, the functions are called in this order:

  nsDocShell::SetCurrentURI
  nsDocShell::SetupNewViewer
  FullZoom::onLocationChange

In some cases (presumably when the page contains iframes) there are a bunch more SetCurrentURI and SetupNewViewer calls after that.

Comment 24

[Boris Zbarsky [:bzbarsky]]

> now that we have chrome code that updates it for each page load.

Firefox does.  Gecko clients in general don't.

> When loading the new page, the functions are called in this order:

Uh... then the viewer you're working with should be the new viewer, no?  It's not clear to me why the old page is getting resized in that case...  Is the viewer you end up calling SetFullZoom on the one that has a previous viewer?  It should be, given that call sequence.  You can probably just remove the noscript in the interface locally if you want to test this from JS.

Comment 25

[Sylvain Pasche]

It looks like the new fullZoom value is set on the new documentViewer, may it be from nsDocShell::SetupNewViewer or from chrome in the locationChange listener.

I was wondering why setting the fullZoom value on the new documentViewer/presContext had an impact when the old documentViewer is painted again. nsPresContext::SetFullZoom() is calling nsThebesDeviceContext::SetPixelScal which changes the mAppUnitsPerDevPixel constant.

I guess there is only one deviceContext during a page transition? That would explain why changing the zoomLevel has effects on the previous viewer when it is painted again.

Comment 26

[Sylvain Pasche]

(In reply to comment #25)
> I guess there is only one deviceContext during a page transition?

Seems to be true. If I set a breakpoint in nsThebesDeviceContext::SetPixelScale(), "this" always points to the same address during a page transition.

Comment 27

[Boris Zbarsky [:bzbarsky]]

Gah.  So I guess device contexts are tied to the widget or something?  ccing some gfx folks who might know.

One option would perhaps be to have Show() fire an event, probably asynchronously, that chrome could detect...  Have to think about what happens when multiple Show()s happen quickly.

Comment 28

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

The device context comes from the docshell and is shared across all its contentviewers. That's definitely the problem. Maybe the docshell could create a new devicecontext when it does a page transition?

Comment 29

[Boris Zbarsky [:bzbarsky]]

Maybe...  Someone should try.

Comment 30

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

I guess that'd be me!

Comment 31

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

Attachment: create a new device context for every documentviewer

I think this works. The big question is whether it will affect pageload performance or not. I'm hoping not; the device context does contain a fontmetrics cache, but we flush that cache on every page transition anyway as far as I can tell, so this isn't a lot worse.

Comment 32

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 297680 [details] [diff] [review]
create a new device context for every documentviewer

>+++ mozilla-trunk/docshell/base/nsDocShell.cpp	
>+        deviceContext->Init(widget->GetNativeData(NS_NATIVE_WIDGET));

So it's OK to have multiple device contexts pointing to the same widget, right?

>+++ mozilla-trunk/layout/base/nsDocumentViewer.cpp	
>-  if (mDeviceContext) {
>-    mDeviceContext->FlushFontCache();
>-  }

This will mean not flushing the font cache when the page goes into bfcache.  Maybe we want to flush here, in fact?  I would guess we do.

Either way, r+sr=bzbarsky, but I have to agree to not having a good feel for how this change could go wrong.  It's a bit off my beaten paths.  :(

Comment 33

[Boris Zbarsky [:bzbarsky]]

I meant "I have to admit".  That is, it looks OK to me, but that doesn't mean much in this case.  :(

Comment 34

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

> This will mean not flushing the font cache when the page goes into bfcache. 
> Maybe we want to flush here, in fact?  I would guess we do.

Well ... I'm not sure. Yes, it would save memory, but the point of bfcache is to burn memory for speed. If we really wanted to save memory we wouldn't put the viewer into bfcache in the first place :-).

> So it's OK to have multiple device contexts pointing to the same widget, right?

Yes, I'm pretty sure it is, because widgets have their own devicecontexts and we've been creating additional device contexts pointing at those widgets.

I know what you mean but these paths are not beaten by anyone AFAIK :-)

Comment 35

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

Checked in, waiting for perf numbers.

Comment 36

[Michaël Arnauts]

I don't know if this is related, or should be filed as a new bug, i might be the same bug...

When you open a page in a new tab in the background (middle click), and that page has a zoom set (site specific), the zoom level jumps from 100% to the set zoom level when activating that tab.

How to reproduce:

go to images.google.com
enlarge the zoom level
go to google.com
middleclick on "images" (so that images.google.com opens in a new window)
activate the tab
see the page jump :)

Comment 37

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

This fix stuck.

Michael: not sure. See if you can still reproduce it in today's build.

Comment 38

[Michaël Arnauts]

I updated to Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b3pre) Gecko/2008012304 Minefield/3.0b3pre and it still happens. Should I file a new bug?

Comment 39

[Boris Zbarsky [:bzbarsky]]

Michaël, that build predates roc checkin in the patch, if you look at the build ID and the timestamp on his "this fix stuck" comment.

Comment 40

[Michaël Arnauts]

I think it was already fixed in the 2008012304-build, because the use case for this bug was fixed (the old page resizing when the new was loaded). Anyway, I tried it with 2008012404 and I have the same behavior... Switching a tab still makes a the zoom jump from 100% to your defined zoom level.

Comment 41

[Adam Kowalczyk]

(In reply to comment #40)
> I think it was already fixed in the 2008012304-build, because the use case for
> this bug was fixed (the old page resizing when the new was loaded). Anyway, I
> tried it with 2008012404 and I have the same behavior... Switching a tab still
> makes a the zoom jump from 100% to your defined zoom level.

That's bug 386835, which is a separate issue.