386835 - page loaded in the background first displays in normal size, then the site-specific text zoom is applied, causing the page to jump

Status: VERIFIED FIXED

(Firefox :: General, defect, P3)

Description

[Steffen Wilberg]

A page loaded in the background first displays in normal size when switched to it.
Then the site-specific text zoom is applied. This causes the whole page to jump.

STR:
1. Visit a site like http://en.wikipedia.org/wiki/Main_Page.
2. Set text-zoom to larger or smaller than normal by pressing Ctrl + or Ctrl -.
3. Load a couple of pages from the same site in the background by middle-clicking e.g.
  http://en.wikipedia.org/wiki/Mozilla
  http://en.wikipedia.org/wiki/Mozilla_Firefox
  http://en.wikipedia.org/wiki/Page_zooming
4. Switch to the background tabs.
5. Optionally repeat steps 2, then 4.

Actual result:
After step 4, the page displays for a split-second (but long enough that you're able to start reading) in normal text-zoom, then jumps to the site-specific text-zoom.
After step 4, the page displays for a split-second in the *previous* displayed text-zoom, then jumps to the *new* site-specific text-zoom.

Expected result:
The page displays in the correct text-zoom from the beginning.

Comment 1

[Steve England [:stevee]]

This can also effect anchor usage. If you open a tab with a site, middle click (so it opens in a new tab in the background) a link to the same site that uses an anchor, change font size, and then change to the new tab, the initial rendering is positioned at the anchor, but then the page is redrawn with the new font size and page is no longer at the anchor position.

Comment 2

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

I haven't seen this in my tests, but the split-second effect is certainly possible, since Firefox doesn't change the zoom of background tabs until you focus them.

That's for performance, since updating existing background tabs could significantly effect the responsiveness of text zoom changes on the foreground tab.  But we should probably updating new background tabs to avoid the anchor horkage noted in comment 1, and perhaps there's a solution for existing background tabs as well (f.e. updating them via a timeout).

Comment 3

[Jesse Ruderman]

How does the existence of this bug help performance?  Setting the text size correctly before the background tab begins to render shouldn't hurt performance.

Comment 4

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

(In reply to comment #3)
> How does the existence of this bug help performance?  Setting the text size
> correctly before the background tab begins to render shouldn't hurt
> performance.

It doesn't for the case where you open some pages in background tabs after changing the text zoom.  It does, however, for the case where you open some pages in background tabs *before* changing the text zoom.

That's because it takes a non-negligible amount of time to change the text zoom on an already loaded tab, because it triggers a reflow.  So if there are many tabs open to the same site when you change the text zoom, and we were to change the zoom for all tabs at once (triggering reflows on all of them), then it would take noticeably longer for the zoom to change on the currently focused tab.

Don't get me wrong, I'm not suggesting we shouldn't fix this bug.  I'm just suggesting that we should fix it a different way for these two cases.

Comment 5

[Dave Townsend [:mossop]]

The broken linking to anchors is enough I think to consider whether to block on this.

Comment 6

[Mike Beltzner [:beltzner, not reading bugmail]]

(In reply to comment #5)
> The broken linking to anchors is enough I think to consider whether to block on
> this.

Agreed, though that might eventually imply a slight morph to focus just on that issue regardless of the solution, which drivers would support.

Comment 7

[Tony Chung [:tchung]]

Hi, can you please update the target milestone for this bug?   Thanks.

Comment 8

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

The way to fix this bug for the "open new background tab to site with custom setting" case is to listen for location change events on each individual browser element rather than just the parent tabbrowser element and then set the zoom for browsers in background tabs when their location changes.

The way to fix this bug for the "change zoom on the current tab while the same site is loaded in existing background tab(s)" case is to enumerate windows and tabs via nsIWindowMediator and the gBrowser.tabs property, respectively, then update the zoom for those on the same site as the current tab.

Note that only the first case regresses linking to anchors.  The second case has always broken anchors for the current tab (i.e. loading a page to an anchor and then changing the zoom changes the location of the anchor), so I wouldn't consider it a regression that it now breaks anchors for background tabs.

Nevertheless, it would be good to get fixes for the anchor bustage in both cases.

Comment 9

[Karl Tomlinson (:karlt)]

(In reply to comment #8)
> The way to fix this bug for the "change zoom on the current tab while the same
> site is loaded in existing background tab(s)" case is to enumerate windows and
> tabs via nsIWindowMediator and the gBrowser.tabs property, respectively, then
> update the zoom for those on the same site as the current tab.

Do we really want to change the zoom on tabs that are currently displaying the same site?

This makes it impossible to get all current tabs at the desired zoom level if one site uses two different font (or other) sizes, or if, for example, tinderbox.mozilla.org has one page that is very wide.

(See also bug 378549 comment 28).

Can the remembered site-specific text zoom be applied only when moving to a new site (including opening a new tab)?

Comment 10

[Dave Townsend [:mossop]]

(In reply to comment #8)
> The way to fix this bug for the "open new background tab to site with custom
> setting" case is to listen for location change events on each individual
> browser element rather than just the parent tabbrowser element and then set the
> zoom for browsers in background tabs when their location changes.

This I think covers the main case here and isn't too complex but has some minor performance implications I think.

> The way to fix this bug for the "change zoom on the current tab while the same
> site is loaded in existing background tab(s)" case is to enumerate windows and
> tabs via nsIWindowMediator and the gBrowser.tabs property, respectively, then
> update the zoom for those on the same site as the current tab.

This I'm actually confused about. In fact in thinking I'm confused about the whole bug. Why is it when I am viewing a page and change zoom the scroll point is updated so the top of part of the page I'm viewing stays at the top, yet when switching to a tab and haviong that zoom level updated the same doesn't happen. If it did then the anchor would should still be in view in most cases I think?

Comment 11

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

(In reply to comment #10)
> Why is it when I am viewing a page and change zoom the scroll point
> is updated so the top of part of the page I'm viewing stays at the top, yet
> when switching to a tab and haviong that zoom level updated the same doesn't
> happen. If it did then the anchor would should still be in view in most cases
> I think?

I actually can't reproduce this problem anymore.  Or rather, I mostly can't reproduce it.  If I load two Bugzilla bug reports in tabs, click on a comment anchor in one, switch to the other, jack up the page zoom, and then switch back to the first one, it gets resized, but the anchor stays mostly at the top of the page.

It's off by a line or so, but it's off by the same amount if I jack up the zoom in the page loaded to the anchor.  So there's no difference between applying the zoom when switching tabs and applying it while on a tab.

Perhaps changing the zoom skews away from the anchor point by larger amounts on other pages, but the skewing itself seems like a separate bug.  Nevertheless, folks wouldn't be experiencing it as much (nor the split second at a different zoom) if we changed the zoom earlier, before the browser jumps to the anchor point in a background tab, so it's still worth fixing this bug.


(In reply to comment #9)
> Do we really want to change the zoom on tabs that are currently displaying the
> same site?

Yes, that's what we expect most users will benefit the most from.


> This makes it impossible to get all current tabs at the desired zoom level if
> one site uses two different font (or other) sizes, or if, for example,
> tinderbox.mozilla.org has one page that is very wide.

Yup, that's a valid issue with the current behavior.


> Can the remembered site-specific text zoom be applied only when moving to a 
> new site (including opening a new tab)?

I don't think we want to make this the standard behavior, nor does it rise to the level of a visible pref, but we could have a hidden pref that enabled this behavior.

Comment 12

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

Attachment: patch v1: should fix first half of problem, but doesn't

Here's a patch that should fix the first half of the problem, i.e. opening pages in background tabs after changing the zoom.  The patch adds locationchange listeners to individual tabs that it uses to update the zoom of background tabs.

Strangely, however, the patch doesn't have the desired effect.  Without the patch, background tabs loaded to an anchor with a custom zoom setting get skewed when the user switches to them, but the skew is the same skew the user observes when changing to that zoom setting in a foreground tab.

With the patch, the skew not only doesn't go away, it gets worse!

With or without the patch, a force reload (Ctrl-Shift-R) loads the page to the anchor point without any skew at all.

To test the patch and current behavior, I first loaded this bug report, reset the zoom, clicked the "comment #10" anchor to jump to it, and increased the zoom three levels.

Then I context-clicked the "comment #10" link, selected "Open Link in New Tab" from the context menu, and switched to the new tab.

Expected behavior: after increasing the zoom three levels, the page should zoom in (get larger), but the "comment #10" header should remain flush with the top of the content window.  After switching to the new tab, the page should load zoomed in and with the "comment #10" header flush with the top of the content window.

Actual behavior without patch: after increasing the zoom, the page zooms in, but the "comment #10" header isn't flush with the top of the content window.  After switching to the new tab, the "comment #10" header isn't flush with the content window either.  Both headers are off by the same amount.

Actual behavior with patch: after increasing the zoom, the page zooms in, but the "comment #10" header isn't flush with the top of the content window.  After switching to the new tab, its "comment #10" header isn't flush with the top of the content window either.  The headers are off by different amounts.

I'm stumped, but it seems like there's something wonky going on here with the way Gecko scrolls to the anchor point during a zoom.

Comment 13

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

Note: regarding the second half of the problem (short flash before resize when switching tabs to a tab that hasn't had the latest zoom applied yet), I'm still not sure it's worth fixing.  The momentary flash is annoying, but I reckon it'd be even more annoying for zoom to be much slower when changing zoom on a site that you've loaded in a bunch of other tabs (like a bunch of Bugzilla bugs, for example).

So I'm inclined to wontfix that part of the bug on the grounds that the cure is worse than the disease, but we should definitely address the first half of the problem, and it's also worth having a hidden pref to disable site-specific zoom and another one to suppress application of a site-specific pref except when "when moving to a new site (including opening a new tab)".

I have filed bug 419609 on a pref to disable site-specific zoom entirely and bug 419612 on the hybrid per-site/per-session approach described by karl in comment 9.

Comment 14

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

Being stumped, I wonder if roc or bz might be able to help here.  roc and bz: see comment 12 for a description of the problem along with attempted solution that didn't work.

Comment 15

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

Gecko does not scroll to the anchor point during zoom. That is why a page in the foreground does not scroll if you change the zoom after it's fully loaded.

I don't remember when onLocationChange fires. I don't understand why that patch didn't work. There is a change to anchor scrolling in bug 317189, it might make things better for you (or worse!). Try it?

Comment 16

[Michaël Arnauts]

Hmm, but why can't the background-tab be rendered in the right zoom at the first place? The way it is now, (afaik), the page is rendered 2 times. 1 time when i middle click a link, and the page gets loaded in the background (at 100%), and 1 time when i click the tab, and the zoom gets applied.

I don't know a thing of the way firefox renders pages, but it seems strange that firefox can't render the page at the right zoom level at the first place...

(In reply to comment #13)
> Note: regarding the second half of the problem (short flash before resize when
> switching tabs to a tab that hasn't had the latest zoom applied yet), I'm still
> not sure it's worth fixing.  The momentary flash is annoying, but I reckon it'd
> be even more annoying for zoom to be much slower when changing zoom on a site
> that you've loaded in a bunch of other tabs (like a bunch of Bugzilla bugs, for
> example).
> 
> So I'm inclined to wontfix that part of the bug on the grounds that the cure is
> worse than the disease, but we should definitely address the first half of the
> problem, and it's also worth having a hidden pref to disable site-specific zoom
> and another one to suppress application of a site-specific pref except when
> "when moving to a new site (including opening a new tab)".
> 
> I have filed bug 419609 on a pref to disable site-specific zoom entirely and
> bug 419612 on the hybrid per-site/per-session approach described by karl in
> comment 9.
> 

Comment 17

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

(In reply to comment #16)
> Hmm, but why can't the background-tab be rendered in the right zoom at the
> first place?

It can.  That's exactly what my patch does.  But nevertheless the page still doesn't load at the anchor point.

Comment 18

[Michaël Arnauts]

Then what is the usecase for the "2nd part of the bug"?

Comment 19

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

(In reply to comment #18)
> Then what is the usecase for the "2nd part of the bug"?

The second half of the bug is updating the zoom for existing background tabs.  For example, you have several b.m.o bug reports loaded into tabs, and you change the zoom level in one of those tabs.

Currently we lazily update the zoom for the other tabs when you switch to them, which might cause a "flash" and brief delay as the zoom is updated.  The fix for this is to enumerate tabs and update them when the zoom level changes, but that has performance implications, so I think the current approach is actually preferable in that case.

Comment 20

[Jesse Ruderman]

It might be better to delay switching to the tab until the tab is re-zoomed, to avoid the flash and skip unnecessary painting.

Comment 21

[Boris Zbarsky [:bzbarsky]]

I seem to recall we suppress at least some kinds of reflows (just resize reflows, maybe?) in background tabs.  Not sure whether that's affecting this case.

I'd certainly expect the onlocationchange for the browser to fire before we do an anchor scroll from the sink, though.  I'd be very surprised if it doesn't.  I guess you could add some printfs in the relevant code to check...

Comment 22

[Mike Connor [:mconnor]]

Not blocking on this bug for final ship. Would take a safe enough patch if one comes through.

Comment 23

[Alex North-Keys]

I'm not a fan of per-site, retained zoom dominating over the more familiar and more flexible model of per-browser-window/tab, retained zoom.  Per-site zoom should be something established via a dialog - something like a "zoom-lock" feature - *not* something set by the control-+/- and control-wheel controls.

If one can't have two windows to the same website at different zoom levels, then the "feature" is broken and should be withdrawn.

[copied from bug 419612]

Drop the whole textzoom-OTHER tabs/windows thing by default entirely, it
interferes with a number of valid use cases, as several have already noted.
There's a fundamental perceptual clash in whether the existing zoom mechanism
applies to the webpage, the tab, or the whole browser.  It's complicated
further by different multiwindow/monowindow usage patterns.  For me, I'd expect
it to apply to only the current webpage/tab pair, and only to be preserved on
webpages ALREADY rendered in that single tab's back/next list, and only for
that session (unless restored through the Session Manager).

So my expectations seem to match what FF 2 implemented pretty well.  Only
tab/page duples were zoomed, and at zoom there was at least a chance that the
same midpage text would still be visible afterwards.  We were only missing a UI
mechanism to set retained default zoom levels - we DID have a painful way to do
it manually.

There was (is?) a userChrome.css hook for user-specified URI-specific CSS
overrides. I'd like to see a menu option to record the current text-zoom,
automatcally-converted into CSS form, into that mechanism. Ideally there would
be a dialog for selecting either that one webpage, a whole site, or for all
matching some regexp be affected, although I'm not sure if the userChrome.css
mechanism already has a regexp function.

Certainly we'd be better off making the userChrome.css more visible instead of
creating a unneeded new mechanism for retaining zoom settings.

Comment 24

[Brian Polidoro]

Your comment has nothing to do with this bug.  Bugzilla is not a forum. Stop spamming bugs.

Comment 25

[Alex North-Keys]

Nevermind.  The "site-specific zoom part" related, although *whether* it was applied was the issue.  I've since found around 10 better-related bug reports.  "spamming"? - try email instead of cluttering the bug commentary.

Comment 27

[Dave Townsend [:mossop]]

Still really annoying, still causes the page to not display the actual target of a link. Blocking 3.1?

Comment 28

[Mike Connor [:mconnor]]

Not blocking, but we'd like to see a patch.

Comment 31

[Dave Townsend [:mossop]]

Are you actually still working on this myk?

Comment 32

[Benjamin Smedberg]

I see something that looks likee this bug a lot on tinderbox pages, but I'm confused reading the comments. I haven't changed the zoom of tinderbox pages in months, but still I see this at least every time I start the browser with a saved session. Can't we at least fix the bug in that case?

Comment 33

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

(In reply to comment #31)
> Are you actually still working on this myk?

Erm, no, I'm not currently working on this bug (although I also wasn't able to reproduce it recently).  Updating the status to reflect that.

Comment 34

[Benjamin Smedberg]

Note: I can also reproduce this by middle-clicking a link and then switching to that tab: it happens a lot for tinderbox logs.

Comment 35

[Dave Townsend [:mossop]]

Taking

Comment 36

[Dave Townsend [:mossop]]

Attachment: patch rev 2

This fixes the problem but it depends on a couple of other patches landing first so I'll wait on them before requesting review.

Comment 37

[Dave Townsend [:mossop]]

Comment on attachment 346695 [details] [diff] [review]
patch rev 2

Fairly straightforward patch, updates the zoom of background browsers as they are loading.

Comment 38

[Dave Townsend [:mossop]]

Attachment: patch rev 3

This moves the zoom updating for the tab switch to updateCurrentBrowser

Comment 40

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 351253 [details] [diff] [review]
patch rev 3

>diff --git a/browser/base/content/browser-textZoom.js b/browser/base/content/browser-textZoom.js

>+  _applyPrefToSetting: function FullZoom__applyPrefToSetting(aBrowser, aValue) {

How about making the browser argument optional (second instead of first), and have it default to gBrowser (aBrowser || gBrowser)? Would eliminate the need to update all the other callers.

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js

>   onLocationChange: function (aBrowser, aWebProgress, aRequest, aLocationURI) {
>+    if (aBrowser.contentWindow == aWebProgress.DOMWindow)
>+      FullZoom.onLocationChange(aBrowser, aLocationURI);

Add a comment here that this is to filter out subframe loads?

>diff --git a/browser/base/content/test/browser_bug386835.js b/browser/base/content/test/browser_bug386835.js

>+function finishTest() {

Wouldn't hurt to call FullZoom.reset() here (before closing the relevant tab).

Comment 41

[:Gavin Sharp [email: [e-mail]]]

I'm a bit concerned about the effects this will have on e.g. a session restore with lots of tabs that have custom zoom settings. I suppose that's probably not too common, though, and I guess there isn't much that can be done to avoid that if we want to fix this bug. We could attempt to detect the session restore or open-bookmark-folder-in-tabs cases specifically and avoid this behavior then... worth a followup perhaps?

Comment 42

[Dave Townsend [:mossop]]

(In reply to comment #41)
> I'm a bit concerned about the effects this will have on e.g. a session restore
> with lots of tabs that have custom zoom settings. I suppose that's probably not
> too common, though, and I guess there isn't much that can be done to avoid that
> if we want to fix this bug. We could attempt to detect the session restore or
> open-bookmark-folder-in-tabs cases specifically and avoid this behavior then...
> worth a followup perhaps?

I'm hopeful that this won't have much perf impact. The zoom setting is being changed early in the load process, before there is even any page content I believe so I would hope that the change is actually very fast, though I guess it could still be measurable. I imagine we have no way to really measure this well?

Comment 43

[:Gavin Sharp [email: [e-mail]]]

A highly unscientific test that just loads 100 tinderbox logs with custom zoom settings and times how long it takes to receive 100 onload events with/without the patch might give you an idea of the cost.

Comment 44

[Dave Townsend [:mossop]]

Attachment: patch rev 4

This addresses the comments, I also made the browser argument optional on FullZoom.onLocationChange on the off chance any extensions are calling that. I'm going to land this on trunk to bake for a bit. I have some ideas for gathering perf numbers from this and I'll open a follow-up bug if they aren't good.

Comment 45

[Dave Townsend [:mossop]]

I've used dtrace to gather some numbers for calls to FullZoom.onLocationChange. This is session restore with 1 foreground tab that is unzoomed and 10 other tabs that are zoomed out two levels.

Before the patch it gets called once as you'd hope and costs 6ms. With the patch it gets called 21 times (we see an about:blank location change followed by the real page for each background tab) and costs a total of 38ms, so roughly just over 3ms per background tab. This is a bit more than I expected because I hadn't taken into account the content pref service. The actual call to FullZoom._applyPrefToSetting only totals 14ms, there is about 22ms cost for calling the content pref service, or about 1ms per call, 2ms per background tab.

So I guess the question is are we happy adding 3ms per background tab that the user has in session restore? Since the content pref service takes up the majority of the time it doesn't make a lot of difference whether those background tabs are zoomed or not.

Comment 46

[Dave Townsend [:mossop]]

I'll add that doing the zoom during the load as opposed to after is indeed a significant saving. In the same testcase (the 10 background tabs are long tinderbox logs), in the current trunk starting up and allowing session restore to load all the pages then manually switching to each one shows FullZoom.onLocationChange called 11 times as expected, with a total time of nearly 15 seconds.

Comment 47

[John Zwinck]

Dave, first, thank you for working on this.

From what you've said, I'd much rather have this patch than not have it.  If performance of content pref is an issue, that could be a new bug, but unless you see a path toward mitigating the (small) performance impact right now, I for one would gladly take the theoretically worse performance over the very visibly broken page viewing.

Comment 48

[Dave Townsend [:mossop]]

I've run some old-school Ts runs with and without this patch with 45 tabs in session restore. The way that Ts works in this case is the browser window is opened, all of the tabs are restored and start loading, then the Ts tab is opened and when it has completed loading it reports the time since startup. This should be a fairly good measurement of how much this patch actually affects the time before the browser is usable. The numbers say what I would expect really. It has no impact whatsoever.

I ran 10 runs without and 10 runs with the patch. If you use traditional Ts measurement (only looking at the lowest result) then the runs with the patch were faster by some 36ms. Probably more sensible to use the average though, and in this case the runs with the patch were slower by just 0.3ms.

I think these numbers confirm that the additional work added by this patch happens in the background, post-startup and should not impact the usability of the browser.

Comment 50

[Dave Townsend [:mossop]]

Landed on trunk: http://hg.mozilla.org/mozilla-central/rev/91c92eb7387d

Comment 51

[Dave Townsend [:mossop]]

Comment on attachment 356570 [details] [diff] [review]
patch rev 4

Seeking approval to land this on the branch. It is I believe very safe and includes tests. There is perf impact as noted in the bug but should not be noticeable, talos is clean and no-one has mentioned seeing issues.

Comment 53

[Alice0775 White]

Is it  not nominated in blocking1.9.1?

https://bugzilla.mozilla.org/buglist.cgi?query_format=advanced&keywords_type=nowords&keywords=fixed1.9.1+verified1.9.1&resolution=FIXED&field0-0-0=flagtypes.name&type0-0-0=equals&value0-0-0=blocking-firefox3.1%2B&field0-0-1=flagtypes.name&type0-0-1=equals&value0-0-1=blocking1.9.1%2B

Comment 54

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 356570 [details] [diff] [review]
patch rev 4

a191=beltzner

Comment 55

[Dave Townsend [:mossop]]

Landed on the branch: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/53e9fe26c579

Comment 56

[Aakash Desai [:aakashd]]

verified FIXED on builds: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090511 Minefield/3.6a1pre ID:20090511031307

and

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b5pre) Gecko/20090511 Shiretoko/3.5b5pre ID:20090511031352

Comment 57

[Alex Faaborg [:faaborg] (Firefox UX)]

This bug's priority relative to the set of other polish bugs is:
P4 - Polish issue that is rarely encountered, and is easily identifiable.

Clearly identifiable, but only effects users who have a set a different zoom level for a specific site, which isn't extremely common.

Comment 59

[Sylvain Pasche]

Apparently this issue still happens on Windows. I filed bug 510753 about it. Looks like there is some platform difference in how the zoom is set.