Closed
Bug 515878
Opened 15 years ago
Closed 15 years ago
refreshing page scrolls to the top
Categories
(Firefox for Android Graveyard :: General, defect)
Firefox for Android Graveyard
General
ARM
Windows Mobile 6 Professional
Tracking
(fennec1.0+)
VERIFIED
FIXED
Tracking | Status | |
---|---|---|
fennec | 1.0+ | --- |
People
(Reporter: blassey, Assigned: vingtetun)
References
()
Details
Attachments
(1 file, 2 obsolete files)
2.82 KB,
patch
|
mfinkle
:
review+
|
Details | Diff | Splinter Review |
if a page refreshes itself, we end up being at the top of the page after the refresh.
Reporter | ||
Updated•15 years ago
|
tracking-fennec: --- → ?
Updated•15 years ago
|
tracking-fennec: ? → 1.0+
Updated•15 years ago
|
Assignee: nobody → webapps
Updated•15 years ago
|
Assignee: webapps → nobody
Assignee | ||
Comment 1•15 years ago
|
||
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 2•15 years ago
|
||
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-
Assignee | ||
Comment 3•15 years ago
|
||
(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 4•15 years ago
|
||
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 5•15 years ago
|
||
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-
Assignee | ||
Comment 6•15 years ago
|
||
* Addressed notifications bugs + comments
Attachment #415096 -
Attachment is obsolete: true
Attachment #415166 -
Flags: review?(mark.finkle)
Updated•15 years ago
|
Attachment #415166 -
Flags: review?(mark.finkle) → review+
Comment 7•15 years ago
|
||
pushed: https://hg.mozilla.org/mobile-browser/rev/a5e63b0dee01
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Comment 8•15 years ago
|
||
Gavin had concerns, so we did a partial revert: https://hg.mozilla.org/mobile-browser/rev/63a6702a9a2f
Comment 9•15 years ago
|
||
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?
Comment 10•15 years ago
|
||
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.
Description
•