refreshing page scrolls to the top

VERIFIED FIXED

Status

VERIFIED FIXED
9 years ago
9 years ago

People

(Reporter: blassey, Assigned: vingtetun)

Tracking

Trunk
ARM
Windows Mobile 6 Professional
Bug Flags:
in-litmus +

Details

(URL)

Attachments

(1 attachment, 2 obsolete attachments)

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

Updated

9 years ago
tracking-fennec: --- → ?

Updated

9 years ago
tracking-fennec: ? → 1.0+

Updated

9 years ago
Assignee: nobody → webapps
Assignee: webapps → nobody
Created attachment 414733 [details] [diff] [review]
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)
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-
Created attachment 415096 [details] [diff] [review]
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.
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-
Created attachment 415166 [details] [diff] [review]
Patch v0.3

* 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
Last Resolved: 9 years ago
Resolution: --- → FIXED
Gavin had concerns, so we did a partial revert:
https://hg.mozilla.org/mobile-browser/rev/63a6702a9a2f
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.