551193 - page position changes on backgrounds tabs when the foreground tab is scrolled

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Panning/Zooming, defect)

Description

[Aakash Desai [:aakashd]]

Build Id:
Mozilla/5.0 (Windows; U; WindowsCE 5.2; en-US; rv:1.9.2.2pre) Gecko/20100309 Namoroka/3.6.2pre Fennec/1.1a1

and

Mozilla/5.0 (X11; U; Linux armv7l; Nokia N900; en-US; rv:1.9.2.2pre) Gecko/20100309 Namoroka/3.6.2pre Fennec/1.1a2pre

and

Mozilla/5.0 (X11; U; Linux armv6l; en-US; rv:1.9.3a2pre) Gecko/20100309 Namoroka/3.7a2pre Fennec/1.1a2pre

Steps to Reproduce:
1. Go to www.google.com/news
2. Open a new tab and go to www.espn.com
3. Go back to the first tab with the loaded Google News page
4. Pan down to the middle of the page
5. Go to the second tab with the loaded ESPN page

Actual Results:
The page on the second tab, ESPN, will be panned past its top nav bar

Expected Results:
The page on the second tab should not be panned

Comment 1

[Matt Brubeck (:mbrubeck)]

It seems the urlbar position is shared across tabs.  Changing it on one tab changes it on all tabs where the urlbar is visible.  If you pan until it is half-visible, it will be half-visible on the other tabs.  (Tabs that are scrolled all the way past the urlbar are not affected.)  Investigating...

Comment 2

[Matt Brubeck (:mbrubeck)]

Attachment: patch against mobile-browser tip

We were saving and restoring the #content-scrollbox offset per tab, but not the #page-scrollbox offset.  (#page-scrollbox contains the urlbar and #content-scrollbox.)

Here's a patch that fixes the issue by also saving the #page-scrollbox offset.  I'd appreciate some feedback.

* Should the old "scrollOffset" name be changed to "contentScrollOffset" (for symmetry with "pageScrollOffset")?

* There is one place where scrollOffset is set but pageScrollOffset is not, in ProgressController.prototype._documentStop.  I think it's probably not necessary to change this, but I wasn't sure.

* Is this actually the best way to solve this problem, or is there something I didn't think of?

Comment 3

[Matt Brubeck (:mbrubeck)]

Attachment: don't access pageScrollOffset if it's not defined

Comment 4

[Matt Brubeck (:mbrubeck)]

I've discovered a problem with this patch; for some reason the scroll position when Fennec first launches is off slightly.  Looking into it.

Comment 5

[Matt Brubeck (:mbrubeck)]

Nevermind, it looks like the problem in comment 4 was already fixed by my revised patch; I was just testing an old build.

Comment 6

[Matt Brubeck (:mbrubeck)]

Err, nevermind again.  The problem in comment 4 is still there, but unrelated to my patch.  (The problem exists both with and without my patch.)  Will file a separate bug if there's not one already.

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 434358 [details] [diff] [review]
don't access pageScrollOffset if it's not defined

Code looks OK. I want to test it a bit too.

Also, adding Ben to take a look too.

Comment 8

[Benjamin Stover (:stechz)]

Comment on attachment 434358 [details] [diff] [review]
don't access pageScrollOffset if it's not defined

Ugh, someday we'll have to do a prettier job of storing scrollbox state for individual tabs.

Looks fine.  Please move the XXX incorrect behavior above the first if statement so it looks more like it applies to both comments.

And please rename scrollOffset to contentScrollOffset.

Comment 9

[Matt Brubeck (:mbrubeck)]

Attachment: revised patch with feedback from stechz

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

pushed:
http://hg.mozilla.org/mobile-browser/rev/e1136b98004d

Comment 11

[Aakash Desai [:aakashd]]

verified FIXED on builds:

Mozilla/5.0 (X11; U; Linux armv7l; Nokia N900; en-US; rv:1.9.2.3pre) Gecko/20100329 Namoroka/3.6.3pre Fennec/1.1a2pre

and

Mozilla/5.0 (X11; U; Linux armv7l; Nokia N900; en-US; rv:1.9.3a4pre) Gecko/20100329 Namoroka/3.7a4pre Fennec/1.1a2pre

litmus testcase https://litmus.mozilla.org/show_test.cgi?id=7587 regression tests this bug.