515878 - refreshing page scrolls to the top

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Brad Lassey [:blassey] (use needinfo?)]

if a page refreshes itself, we end up being at the top of the page after the refresh.

Comment 1

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch

This patch check if the page is really different during a onLocationChange (it also add a call to removeTransientNotifications that we have forgot to do)

Comment 2

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

Comment on attachment 414733 [details] [diff] [review]
Patch

>diff -r 914a3f23bbb2 chrome/content/browser.js

>+    if (selectedBrowser.lastURI) {
>+      let oldSpec = selectedBrowser.lastURI;
>+      if (location == oldSpec)
>+        return;

Why do we need to save the lastURI? Can't we just use selectedBrowser.currentURI.spec ?
>+      
>+      let notificationBox = Browser.getNotificationBox();
>+      notificationBox.removeTransientNotifications();

Why do these lines need to be inside the if (selectedBrowser.lastURI) block?

>+
>+    selectedBrowser.lastURI = location;

If we do decide to save the location, we should not call it lastURI since it is not a true nsIURI. We would call it .lastLocation or .lastSpec

Comment 3

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch v0.2

(In reply to comment #2)
> (From update of attachment 414733 [details] [diff] [review])
> >diff -r 914a3f23bbb2 chrome/content/browser.js
> 
> >+    if (selectedBrowser.lastURI) {
> >+      let oldSpec = selectedBrowser.lastURI;
> >+      if (location == oldSpec)
> >+        return;
> 
> Why do we need to save the lastURI? Can't we just use
> selectedBrowser.currentURI.spec ?

selectedBrowser.currentURI.spec is already set to the new current URI.

> >+      
> >+      let notificationBox = Browser.getNotificationBox();
> >+      notificationBox.removeTransientNotifications();
> 
> Why do these lines need to be inside the if (selectedBrowser.lastURI) block?


I've done it the same way as Firefox (http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#4091)

> >+
> >+    selectedBrowser.lastURI = location;
> 
> If we do decide to save the location, we should not call it lastURI since it is
> not a true nsIURI. We would call it .lastLocation or .lastSpec

Adressed.

Comment 4

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

Comment on attachment 415096 [details] [diff] [review]
Patch v0.2

>+    if (this._hostChanged && this._tab == Browser.selectedTab) {
>       BrowserUI.updateURI();
> 
>       // We're about to have new page content, to scroll the content area
>       // to the top so the new paints will draw correctly.
>       Browser.scrollContentToTop();
>     }

We can nest the if (this._tab == Browser.selectedTab) part in the if (this._hostChanged) block below

>+    if (this._hostChanged) {

>+    }

Comment 5

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

Comment on attachment 415096 [details] [diff] [review]
Patch v0.2

We can't remove all transient notifications. Fennec notifications are shared.

Wemight need to override removeTransientNotifications or simply loop over the notifications and only remove those belonging to the selected tab

Comment 6

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch v0.3

* Addressed notifications bugs + comments

Comment 7

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

pushed:
https://hg.mozilla.org/mobile-browser/rev/a5e63b0dee01

Comment 8

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

Gavin had concerns, so we did a partial revert:
https://hg.mozilla.org/mobile-browser/rev/63a6702a9a2f

Comment 9

[Aakash Desai [:aakashd]]

Fantastic! verified FIXED on builds:

Mozilla/5.0 (Windows; U; Window3sCE 5.2; en-US; rv:1.9.2b5pre) Gecko/20091201 Namoroka/3.6b5pre Fennec/1.0a4pre

and

Mozilla/5.0 (X11; U; Linux armv7l; Nokia N900; en-US; rv:1.9.2b5pre) Gecko/20091201 Firefox/3.6b5pre Fennec/1.0b6pre

and

Mozilla/5.0 (X11; U; Linux armv6l; Nokia N8xx; en-US; rv:1.9.3a1pre) Gecko/20091201 Firefox/3.7a1pre Fennec/1.0b5

Comment 10

[Aakash Desai [:aakashd]]

litmus testcase 9802 has been added to regression test this bug.