Closed Bug 515878 Opened 10 years ago Closed 10 years ago

refreshing page scrolls to the top

Categories

(Firefox for Android Graveyard :: General, defect)

ARM
Windows Mobile 6 Professional
defect
Not set

Tracking

(fennec1.0+)

VERIFIED FIXED
Tracking Status
fennec 1.0+ ---

People

(Reporter: blassey, Assigned: vingtetun)

References

()

Details

Attachments

(1 file, 2 obsolete files)

if a page refreshes itself, we end up being at the top of the page after the refresh.
tracking-fennec: --- → ?
tracking-fennec: ? → 1.0+
Assignee: nobody → webapps
Assignee: webapps → nobody
Attached patch Patch (obsolete) — Splinter Review
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)
Assignee: nobody → 21
Attachment #414733 - Flags: review?(mark.finkle)
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
Attachment #414733 - Flags: review?(mark.finkle) → review-
Attached patch Patch v0.2 (obsolete) — Splinter Review
(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.
Attachment #414733 - Attachment is obsolete: true
Attachment #415096 - Flags: review?(mark.finkle)
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) {

>+    }
Attachment #415096 - Flags: review?(mark.finkle) → review+
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
Attachment #415096 - Flags: review+ → review-
Attached patch Patch v0.3Splinter Review
* Addressed notifications bugs + comments
Attachment #415096 - Attachment is obsolete: true
Attachment #415166 - Flags: review?(mark.finkle)
Attachment #415166 - Flags: review?(mark.finkle) → review+
pushed:
https://hg.mozilla.org/mobile-browser/rev/a5e63b0dee01
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
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
Status: RESOLVED → VERIFIED
Flags: in-litmus?
litmus testcase 9802 has been added to regression test this bug.
Flags: in-litmus? → in-litmus+
You need to log in before you can comment on or make changes to this bug.