Closed Bug 476703 Opened 15 years ago Closed 15 years ago

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

Categories

(Firefox for Android Graveyard :: General, defect)

x86
macOS
defect
Not set
normal

Tracking

(fennec1.0+)

VERIFIED FIXED
Tracking Status
fennec 1.0+ ---

People

(Reporter: mfinkle, Unassigned)

References

Details

Attachments

(1 file, 4 obsolete files)

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
tracking-fennec: ? → 1.0+
Attached 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)
Attached 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)
Attached 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)
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.
Attached 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
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.
Attached 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
Closed: 15 years ago
Resolution: --- → FIXED
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 → ---
leave this one closed -- it was fixed.  510997 is for the recent regression
Status: REOPENED → RESOLVED
Closed: 15 years ago15 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.