405483 - When zooming: web sites not centered, no scrollbars

Status: VERIFIED FIXED

(Toolkit :: Preferences, defect, P1)

Description

[Brian Polidoro]

Some sites are off center and don't have scrollbars when zoomed and reloaded. 

1. Goto a site like the URL given.
2. Zoom the site.  I was zooming in twice. I believe this is needed to invoke the site pref code. 
3. Reload the page from server (Ctrl-F5).

1126 0051 broke 
1126 0034 ok 

http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=1196066040&maxdate=1196067059

Comment 1

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

Yup, I see this too.  Investigating...

Comment 2

[Volkmar Kostka]

For me it is not necessary to reload. Any link within the site will do too.
Example: In the mozillazine forums go to quick reply, zoom in and sent the post. The result page is already without scrollbars.
Reseting zoom (CTRL-0) and reloading (CTRL-F5) brings them back.

Comment 3

[Brian Polidoro]

Reloading is not necessary if you already have a site pref set for that site.  But starting with a clean profile you need to get a site pref set.  And those were the simplest steps to do that.  

Comment 4

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

This is being caused by the changes to nsPresContext.cpp from bug 385224.  Looking into why...

Comment 5

[Dave Townsend [:mossop]]

If you zoom out then click a link then you get the new page only taking up a smaller portion of the normal content area.

Comment 6

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

Better back that part out then. Should be able to back the nsPresContext changes out and leave the rest in.

Comment 7

[Mike Connor [:mconnor]]

-> myk

Comment 8

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

Attachment: patch v1: fixes problem

Although I don't understand the exact issue yet, the bug seems to be caused by the devicecontext and the prescontext being out of sync on prescontext initialization when the previous page has a non default zoom, since the prescontext constructor sets its mFullZoom to the default value (1.0), while the devicecontext's mPixelScale and mAppUnitsPerDevPixel retain their non-default values based on the zoom from the previous page.

Before the checkin for bug 385224, they quickly got back in sync when DocumentViewerImpl::InitPresentationStuff called nsPresContext::SetFullZoom with the default zoom value and nsPresContext::SetFullZoom then called nsThebesDeviceContext::SetPixelScale, even though the new (default) zoom value was the same as the existing (default) one.

Now, however, nsPresContext::SetFullZoom doesn't do anything if the new value is the same as the existing one, so the two contexts stay out of sync until later in the process, when some caller (not sure what yet) calls nsPresContext::SetFullZoom again with the previous page's non-default zoom value.

Reverting the change to nsPresContext.cpp from bug 385224 fixes the bug, but it seems like it would be preferable to explicitly synchronize the two contexts on prescontext initialization, so here's a patch that does that.

Note: it's not clear to me why nsPresContext::SetFullZoom gets called twice, first by DocumentViewerImpl::InitPresentationStuff and then by some other caller (not sure what yet), before being called a *third* time on location change by the front-end TextZoom controller, which sets the zoom to the site-specific value.

It seems like only the third of these calls would be necessary or desirable, and there might be a perf impact here for pages where the default value, the value for the previous page, and the value for the current page are all different (and if either of the first two calls trigger a reflow).  But that's another bug.

Comment 9

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

Comment on attachment 290315 [details] [diff] [review]
patch v1: fixes problem

r+sr=me

Comment 10

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

Checking in layout/base/nsPresContext.cpp;
/cvsroot/mozilla/layout/base/nsPresContext.cpp,v  <--  nsPresContext.cpp
new revision: 3.337; previous revision: 3.336
done

Comment 13

[Brian Polidoro]

verified that this problem is fixed.
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b2pre) Gecko/2007112705 Minefield/3.0b2pre ID:2007112705

But my content prefs seem to be out of whack.  If I open a new tab from an existing tab with them both being the same domain then the new tab is at the default zoom level instead of the level I have in the existing tab.  And If I change the zoom in one of those tabs it doesn't affect the other tab.  I checked in a new profile and this doesn't happen there.  So I'll see if it sorts itself out.  If not I'll delete my content-prefs.sqlite file and see what happens.  

Comment 14

[Brian Polidoro]

In the console I see:
Error: this._cps is undefined
Source File: chrome://browser/content/browser.js
Line: 5945

Warning: reference to undefined property this._cps
Source File: chrome://browser/content/browser.js
Line: 5945

Comment 15

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

That error would explain why content prefs don't work for you, since the TextZoom controller needs a reference to the content pref service to get and set prefs.  But I can't reproduce the problem here on Mac and Linux.

Comment 16

[Brian Polidoro]

Attachment: problem file

Well the sqlite file is the problem.  I removed it and things work as expected.  I put that file in another profile and it had the problems too.  I also saw the startup error to go with this:

Error: Failed to init content pref service:
[Exception... "Component returned failure code: 0x80570016 (NS_ERROR_XPC_GS_RETURNED_FAILURE) [nsIJSCID.getService]"  nsresult: "0x80570016 (NS_ERROR_XPC_GS_RETURNED_FAILURE)"  location: "JS frame :: chrome://browser/content/browser.js :: FullZoom_get__cps :: line 5822"  data: no]
Source File: chrome://browser/content/browser.js
Line: 857

From browsing the file I guess it's because there's also left over text zoom prefs in there.  I'm attaching the file in case it's worth looking at.  I just wonder why it shows up now instead of when the switch was made to page zoom.  IRC there are no site prefs in FF2 so I guess upgrading users would not encounter this in the release.

Comment 17

[Dave Townsend [:mossop]]

(In reply to comment #15)
> That error would explain why content prefs don't work for you, since the
> TextZoom controller needs a reference to the content pref service to get and
> set prefs.  But I can't reproduce the problem here on Mac and Linux.

I'm seeing the problem on my mac now, zoom level just isn't getting remembered.

Comment 18

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

(In reply to comment #16)
> From browsing the file I guess it's because there's also left over text zoom
> prefs in there.  I'm attaching the file in case it's worth looking at.

Thanks!  I think this is actually a regression from the checkin for bug 403375, not a regression from this bug or a problem with the text zoom prefs (which shouldn't cause such problems, as the content pref datastore is designed to store arbitrary numbers and types of prefs).  Investigating...

Comment 19

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

(In reply to comment #18)
> (In reply to comment #16)
> > From browsing the file I guess it's because there's also left over text zoom
> > prefs in there.  I'm attaching the file in case it's worth looking at.
> 
> Thanks!  I think this is actually a regression from the checkin for bug 403375,
> not a regression from this bug or a problem with the text zoom prefs (which
> shouldn't cause such problems, as the content pref datastore is designed to
> store arbitrary numbers and types of prefs).  Investigating...

Yup, that's what it is.  Filed as bug 405683.

Comment 21

[Dave Townsend [:mossop]]

I can still cause this bug using a slightly more tricky set of steps, filed bug 406181