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)

defect

Tracking

()

RESOLVED FIXED

People

(Reporter: dennisml, Assigned: roc)

References

Details

Attachments

(5 files)

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.
Version: unspecified → Trunk
Attached image This is a page in its normal state. (deleted) —
Attached image Page just before the new one appears. (deleted) —
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.
I'm no longer seeing this in the latest nightly.
Status: UNCONFIRMED → RESOLVED
Closed: 17 years ago
Resolution: --- → WORKSFORME
Scratch that. The problem is still there.
Status: RESOLVED → UNCONFIRMED
Resolution: WORKSFORME → ---
Component: General → GFX
Product: Firefox → Core
QA Contact: general → general
I can confirm the issue with today's nightly.
Blocks: pagezoom
Status: UNCONFIRMED → NEW
Ever confirmed: true
Does this have something to do with site-specific preferences and/or page zoom? There are comments hinting at both in bug 406577.
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)
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.
(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
Flags: blocking1.9?
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
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.
Attached file pausing server for testing (deleted) —
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.
(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."
(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.
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...
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?
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...
(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.
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?
(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.
> 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?
(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.
> 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.
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.
(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.
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.
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?
Maybe... Someone should try.
I guess that'd be me!
Assignee: myk → roc
Component: GFX → Embedding: Docshell
OS: Linux → All
Hardware: PC → All
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)
Whiteboard: [needs review]
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+
I meant "I have to admit". That is, it looks OK to me, but that doesn't mean much in this case. :(
> 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 :-)
Checked in, waiting for perf numbers.
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 :)
This fix stuck. Michael: not sure. See if you can still reproduce it in today's build.
Status: NEW → RESOLVED
Closed: 17 years ago17 years ago
Resolution: --- → FIXED
Whiteboard: [needs review]
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?
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.
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.
(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.

Attachment

General

Created:
Updated:
Size: