Closed
Bug 403565
Opened 17 years ago
Closed 17 years ago
Old page gets resized when browsing to a new one
Categories
(Core :: DOM: Navigation, defect, P3)
Core
DOM: Navigation
Tracking
()
RESOLVED
FIXED
People
(Reporter: dennisml, Assigned: roc)
References
Details
Attachments
(5 files)
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
application/x-gzip
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
bzbarsky
:
review+
bzbarsky
:
superreview+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Updated•17 years ago
|
Version: unspecified → Trunk
Reporter | ||
Comment 1•17 years ago
|
||
Reporter | ||
Comment 2•17 years ago
|
||
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.
Reporter | ||
Comment 3•17 years ago
|
||
I'm no longer seeing this in the latest nightly.
Status: UNCONFIRMED → RESOLVED
Closed: 17 years ago
Resolution: --- → WORKSFORME
Reporter | ||
Comment 4•17 years ago
|
||
Scratch that. The problem is still there.
Status: RESOLVED → UNCONFIRMED
Resolution: WORKSFORME → ---
Updated•17 years ago
|
Component: General → GFX
Product: Firefox → Core
QA Contact: general → general
Comment 5•17 years ago
|
||
I can confirm the issue with today's nightly.
Comment 7•17 years ago
|
||
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•17 years ago
|
||
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)
Reporter | ||
Comment 9•17 years ago
|
||
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•17 years ago
|
||
(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
Reporter | ||
Updated•17 years ago
|
Flags: blocking1.9?
Comment 11•17 years ago
|
||
Myk, can you take a look at this and determine if this is a layout bug?
Assignee: nobody → myk
Flags: blocking1.9? → blocking1.9+
Priority: -- → P3
Comment 12•17 years ago
|
||
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•17 years ago
|
||
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•17 years ago
|
||
(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."
Reporter | ||
Comment 15•17 years ago
|
||
(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•17 years ago
|
||
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•17 years ago
|
||
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•17 years ago
|
||
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•17 years ago
|
||
(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•17 years ago
|
||
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•17 years ago
|
||
(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•17 years ago
|
||
> 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•17 years ago
|
||
(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•17 years ago
|
||
> 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•17 years ago
|
||
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•17 years ago
|
||
(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•17 years ago
|
||
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.
Assignee | ||
Comment 28•17 years ago
|
||
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•17 years ago
|
||
Maybe... Someone should try.
Assignee | ||
Comment 30•17 years ago
|
||
I guess that'd be me!
Assignee: myk → roc
Component: GFX → Embedding: Docshell
OS: Linux → All
Hardware: PC → All
Assignee | ||
Comment 31•17 years ago
|
||
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.
Attachment #297680 -
Flags: superreview?(bzbarsky)
Attachment #297680 -
Flags: review?(bzbarsky)
Assignee | ||
Updated•17 years ago
|
Whiteboard: [needs review]
Comment 32•17 years ago
|
||
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. :(
Attachment #297680 -
Flags: superreview?(bzbarsky)
Attachment #297680 -
Flags: superreview+
Attachment #297680 -
Flags: review?(bzbarsky)
Attachment #297680 -
Flags: review+
Comment 33•17 years ago
|
||
I meant "I have to admit". That is, it looks OK to me, but that doesn't mean much in this case. :(
Assignee | ||
Comment 34•17 years ago
|
||
> 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 :-)
Assignee | ||
Comment 35•17 years ago
|
||
Checked in, waiting for perf numbers.
Comment 36•17 years ago
|
||
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 :)
Assignee | ||
Comment 37•17 years ago
|
||
This fix stuck.
Michael: not sure. See if you can still reproduce it in today's build.
Status: NEW → RESOLVED
Closed: 17 years ago → 17 years ago
Resolution: --- → FIXED
Whiteboard: [needs review]
Comment 38•17 years ago
|
||
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•17 years ago
|
||
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•17 years ago
|
||
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•17 years ago
|
||
(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.
You need to log in
before you can comment on or make changes to this bug.
Description
•