When switching tabs, use the last pan x, y offsets

VERIFIED FIXED

Status

defect
VERIFIED FIXED
10 years ago
10 years ago

People

(Reporter: mfinkle, Unassigned)

Tracking

(Blocks 1 bug)

Trunk
x86
macOS
Dependency tree / graph
Bug Flags:
in-litmus +

Firefox Tracking Flags

(fennec1.0+)

Details

Attachments

(1 attachment, 4 obsolete attachments)

Fennec forces each tab back to 0,0 when tabs are switched. We should remember the last pan coords and reuse them.

http://mxr.mozilla.org/mobile-browser/source/chrome/content/browser.js#345
tracking-fennec: --- → ?
Blocks: 477628

Updated

10 years ago
tracking-fennec: ? → 1.0+
Posted patch patch v1 (obsolete) — Splinter Review
By this patch, Fennec backups scroll position and restores it after TabSelect event is fired.
Attachment #383090 - Flags: review?(combee)
Posted patch patch v1.1 (obsolete) — Splinter Review
Save/restore the zoom level too.
Attachment #383090 - Attachment is obsolete: true
Attachment #383091 - Flags: review?(combee)
Attachment #383090 - Flags: review?(combee)
Posted patch patch v1.2 (obsolete) — Splinter Review
For safety. This version sets the default scroll position and zoom level, and restores them always.
Attachment #383091 - Attachment is obsolete: true
Attachment #383093 - Flags: review?(combee)
Attachment #383091 - Flags: review?(combee)

Updated

10 years ago
Attachment #383093 - Flags: review?(combee) → review?(mark.finkle)
How would this handle a tab changing pages while in the background, perhaps under script control?  When you switched back, you might not be in the right zoom/position for the new page.
Yes, this version doesn't consider scrollings of background pages. However,
a fundamental problem, Fennec cannot handle scrolling in pages not only by
scripts but also by in-page-links, even if the tab is in foreground.
(bug 452286 and bug 464984)

I think that the topic in your comment 4 should be filed as a new bug, after
bug 464984 is fixed.
> I think that the topic in your comment 4 should be filed as a new bug,
> after bug 464984 is fixed.

Or, should I wait that the bug becomes to be fixed and update this patch after it?
Comment on attachment 383093 [details] [diff] [review]
patch v1.2

>diff -r 4a73f3e10e4a chrome/content/browser-ui.js

>+    Browser.canvasBrowser.zoomLevel = Browser.selectedBrowser.lastZoomLevel;
>+    ws.panTo(browser.lastScrollX, browser.lastScrollY);
>+

I thinking we should do this in CanvasBrowser.setCurrentBrowser()

>diff -r 4a73f3e10e4a chrome/content/browser.js

>+      // backup last state
>+      Browser.selectedBrowser.lastScrollX = Math.max(0, vr.left);
>+      Browser.selectedBrowser.lastScrollY = Math.max(0, vr.top);
>+      Browser.selectedBrowser.lastZoomLevel = self._canvasBrowser.zoomLevel;
>+

I think we could do this in CanvasBrowser.setCurrentBrowser as well. It would be nice to keep this code contained in one place.

Let's rename the properties:
lastScrollX -> mScrollX
lastScrollY -> mScrollY
lastZoomLevel -> mZoom

We seem to use the "m" prefix in these situations.

>+
>+    browser.lastScrollX = 0;
>+    browser.lastScrollY = 0;
>+    browser.lastZoomLevel = 1;

Can we not do this? You could just check for the property in CanvasBrowser.setCurrentBrowser() first.

if (browser.mZoom) {
 // reset the zoom & scroll
}

We can deal with the background scrolling later.
Attachment #383093 - Flags: review?(mark.finkle) → review-
Thanks. I'm trying to put all of changes into CanvasBrowser.js now.
Posted patch patch v2 (obsolete) — Splinter Review
Updated version. Most changes are inside of setCurrentBrowser().

However, the viewport becomes broken by bug 494715 after you switch tabs. I posted anotehr patch to the bug 494715 too.
Attachment #383171 - Flags: review?(mark.finkle)
Depends on: 494715

Updated

10 years ago
Attachment #383093 - Attachment is obsolete: true
Comment on attachment 383171 [details] [diff] [review]
patch v2

>diff -r 4a73f3e10e4a chrome/content/browser.js
> 
>-    ws.panTo(0, -BrowserUI.toolbarH);
>+    ws.panBy(0, -BrowserUI.toolbarH);

We want to remove this completely, right?

r+ with that change
Attachment #383171 - Flags: review?(mark.finkle) → review+
Should I remove only the CHANGE, or the LINE?:

>-    ws.panTo(0, -BrowserUI.toolbarH);
>+    ws.panBy(0, -BrowserUI.toolbarH);

Because ws.panTo() is called after I restore the panned position in setCurrentBrowser(), the restored state seems to be lost. So, I changed ws.panTo() to ws.panBy() to save the state. And, the ws.panTo() was originally introduced by the patch on the bug 489510. Now I'm trying to read the will of the ws.panTo().
(In reply to comment #11)
> Should I remove only the CHANGE, or the LINE?:
> 
> >-    ws.panTo(0, -BrowserUI.toolbarH);
> >+    ws.panBy(0, -BrowserUI.toolbarH);

Remove the LINE

> Because ws.panTo() is called after I restore the panned position in
> setCurrentBrowser(), the restored state seems to be lost. So, I changed
> ws.panTo() to ws.panBy() to save the state. And, the ws.panTo() was originally
> introduced by the patch on the bug 489510. Now I'm trying to read the will of
> the ws.panTo().

Now that you update the panning postion as we change tabs, we do not nee to reset the pan position to the top. We do not want to show the toolbar either. We only want to show the toolbar on new page loads.

Bug 489510 added the ws.panTo as a hack, since we did not yet support resetting the actual pan and zoom.
Posted patch patch v2.1Splinter Review
The ws.panTo() following the browser switching is removed. (by comment #12)
Attachment #383171 - Attachment is obsolete: true
Attachment #383394 - Flags: review?(mark.finkle)
Attachment #383394 - Flags: review?(mark.finkle) → review+
There is some current breakage with zoom (bug 498561) that is not due to this patch. It's not helped by this patch either.

pushed:
http://hg.mozilla.org/mobile-browser/rev/5e0ab5179c38
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Duplicate of this bug: 483336
unable to verify with 20090818 nightly on maemo.

I open www.mozilla.org in tab 1 and www.yahoo.com in tab 2.  In yahoo.com I pan to the bottom, then I switch tabs to tab1 then back to tab2.  Now in tab2, I am at the top.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 17

10 years ago
leave this one closed -- it was fixed.  510997 is for the recent regression
Status: REOPENED → RESOLVED
Last Resolved: 10 years ago10 years ago
Resolution: --- → FIXED
verified FIXED on builds:

Mozilla/5.0 (X11; U; Linux armv6l; en-US; rv:1.9.2b1pre) Gecko/20090928
Fennec/1.0b4pre

and

Mozilla/5.0 (X11; U; Linux armv7l; en-US; rv:1.9.2b1pre) Gecko/20090928
Fennec/1.0b4pre
Status: RESOLVED → VERIFIED
Need to add a new regression testcase to tabbed browsing BFT for this bug
Flags: in-litmus?
Never mind, there is a test case for this on the screen navigation subgroup: https://litmus.mozilla.org/show_test.cgi?id=7587

I'm moving this over to the tabbed browsing subgroup
Flags: in-litmus? → in-litmus+
You need to log in before you can comment on or make changes to this bug.