600707 - Update back/forward buttons on every location change

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Matt Brubeck (:mbrubeck)]

Attachment: patch

Steps to reproduce:
1. In a new tab, open www.google.com
2. Type about:blank in the urlbar and press enter
3. Type www.google.com in the urlbar and press enter
4. Press the back button

Expected results: www.google.com is displayed (about:blank is ignored in back/forward history), back button is disabled, forward button is enabled.

Actual results: www.google.com is displayed, back button is enabled but does not work, forward button is disabled.

Alternate steps to reproduce:
1. Open https://bug598391.bugzilla.mozilla.org/attachment.cgi?id=477213
2. Click on "test 1"

Expected results: back button is enabled and goes back to the top
Actual results: back button is disabled

Comment 1

[Matt Brubeck (:mbrubeck)]

Attachment: patch v2

Add some browser-chrome tests.

Comment 2

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

Comment on attachment 479634 [details] [diff] [review]
patch v2

>diff -r 526a42fca835 chrome/content/browser.js

>+    BrowserUI.updateURI();

Shouldn't we only do this if we are the active (selected) tab?

>     if (location != this.browser.lastLocation) {
 
>       if (this._tab == Browser.selectedTab) {
>-        BrowserUI.updateURI();
>-

The old code might have failed because of the outer location check

>diff -r 526a42fca835 chrome/tests/browser_navigation.js

>+  onPageReady: function() {
>+    ok(back.disabled, "Can't go back");
>+    ok(forward.disabled, "Can't go forward");
>+
>+    Browser.loadURI(testURL_01 + "#fragment");
>+    setTimeout(gCurrentTest.onFragmentLoaded, 0);
>+  },

The setTimeouts in this new test frighten me. I worry the code won't be ~instant~ on devices. Can we use "pageshow"? It should fire when moving through session history

r- for the questions

Comment 3

[Matt Brubeck (:mbrubeck)]

Attachment: patch

(In reply to comment #2)
> >+    BrowserUI.updateURI();
> 
> Shouldn't we only do this if we are the active (selected) tab?

Oh, of course.  Fixed.

> The setTimeouts in this new test frighten me. I worry the code won't be
> ~instant~ on devices. Can we use "pageshow"?

"pageshow" doesn't fire for navigation within the same page.  I tried adding a "hashchange" listener and message, but it fired too soon, before the chrome onLocationChange observer was notified.  Instead I just changed this to listen for WebProgress:LocationChange.

Comment 4

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

Comment on attachment 479793 [details] [diff] [review]
patch


>diff -r 4c4a20e44f33 chrome/tests/browser_preferences_basic.js

>   onPrefsView: function() {

>+    info("Prefs list size is " + w + ", " + h);

>+    let event = document.createEvent("Events");
>+    event.initEvent("CancelTouchSequence", true, true);
>+    window.dispatchEvent(event);
>+

Different bug fix? or should these be here?

r+ but remove the extra code if it is not needed.

Comment 5

[Matt Brubeck (:mbrubeck)]

(In reply to comment #4)
> Different bug fix? or should these be here?

Accidentally included from work on a different bug.  Removed.

Pushed: http://hg.mozilla.org/mobile-browser/rev/79c093eb1efd

Comment 6

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Verified : 
Mozilla/5.0 (Maemo;Linux armv71; rv:2.0b7pre)Gecko/20101001 Firefox/4.0b7pre Fennec/4.0b2pre
Mozilla/5.0 (Android; Linux armv71; rv2.0b7pre) Gecko/20101001 Firefox/4.0b7pre Fennec/4.0b2pre