1124190 - [tablet] Tabs opened in background take selected tab's back/forward button state if switched to before loading is completed

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Catalin Suciu [:csuciu]]

Firefox 36 Beta 2

Steps:
1. Open news.google.com
2. Long tap on a title and 'Open in a new tab'
3. Go to the new tab and check the state of the 'Back' button

Expected: The button should be grayed out

Actual: The button is active and doesn't do anything 

This is not reproducible on other branches nor on Firefox 36 Beta 1

Comment 1

[Aaron Train [:aaronmt]]

[Tracking Requested - why for this release]:

Comment 2

[Catalin Suciu [:csuciu]]

Trying to find the regression range on a second device (Galaxy Note 3 with Android 4.4.2), I found out that this issue is reproducible on Firefox 36,37 and 38 and was introduced in Nightly 36.0a1 2014-11-13
(so please disregard the last phrase from comment #0)

Here's the pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=688f821edcd4&tochange=ab137ddd3746

I suspect that bug #960746 introduced this regression.

Comment 3

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

We need to make sure devs have devices that can reproduce the bug.

Comment 4

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Mark Finkle (:mfinkle) from comment #3)
> We need to make sure devs have devices that can reproduce the bug.

Yep, on my N7.

If bug 960746 is to blame, this is interesting because from there we're syncing our back/forward state directly with Gecko.

Notes (using STR from comment 0):
  * The back button only becomes enabled when the page has *completely* finished loading.
  * After ^, switching away from the tab and back to the tab does *not* show the enabled state
  * (Undesired behavior?) Hitting back on this page (with the back button disabled) does not close the browser, but rather does nothing. Hitting back on about:home, however, will close the browser

Also discovered the back stab can be inconsistent from a new tab (sometimes the back button is enabled and about:home is accessbile, sometimes it is not - once, the back stack was accessible, clicking about:home did not go to about:home and the back button was disabled). I'm not sure if I did anything special to make it this wonky.

Comment 5

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Seems we sync Gecko tab state with the *selected* tab, not the tab handling the location change [1]:

  let webNav = BrowserApp.selectedTab.window...

[1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js?rev=174cc41e7e03#4454

Comment 6

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Get tab state from tab in arguments; fix bug in forward/back button state

Sorry, but this review is a bit of a doosey without prior enter/exit editing
mode experience! Let me know if you have questions!

The forward/back button state appeared incorrectly because a newly added
method made assumptions that BrowserToolbarNewTablet.cancelEdit would only be
called when we were in editing mode - fixed that by only doing that code while
we're still in editing mode.

The fix for the back/forward state is a bit more systemic - I've tried to make
small fixes before (e.g. bug 998000) but they never made it in. I hope to
better fix this with the editing mode/awesomescreen improvements. For now, this
is easier to uplift.

Comment 7

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Michael Comella (:mcomella) from comment #6)
> The fix

Should read "The proper fix"

> for the back/forward state is a bit more systemic

For now, this is a hack that is easier to uplift.

Comment 8

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8554064 [details] [diff] [review]
Get tab state from tab in arguments; fix bug in forward/back button state

Review of attachment 8554064 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/BrowserApp.java
@@ +2008,5 @@
>          hideHomePager(url);
>          loadUrlOrKeywordSearch(url);
>      }
>  
> +    private void cancelEditingMode() {

Did not mean to include BrowserApp changes. Please ignore.

Comment 9

[Martyn Haigh (:mhaigh)]

Comment on attachment 8554064 [details] [diff] [review]
Get tab state from tab in arguments; fix bug in forward/back button state

Review of attachment 8554064 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me with the BrowserApp change removed.

Comment 10

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Get tab state from tab in arguments; fix bug in forward/back button state

Updated patch.

Comment 11

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

https://hg.mozilla.org/integration/fx-team/rev/f2cc06f8089a

Comment 12

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8554804 [details] [diff] [review]
Get tab state from tab in arguments; fix bug in forward/back button state

Approval Request Comment
[Feature/regressing bug #]: New tablet UI (bug 1014156)

[User impact if declined]:
  When switching tabs to currently loading pages on new tablet, users will have an incorrect back/forward button state (it will be the state of the currently selected tab) - functionality works if the proper button is enabled. Hitting an enabled button that shoeld be disabled does nothing (which may confuse the users and looks amateur).

[Describe test coverage new/current, TreeHerder]:
  None, filed bug 1126048 to followup (though it will likely take a while to get assigned - it's a mentee bug)

[Risks and why]: 
  Low - in the worst case, we screw up the back/forward button state even more (including for the selected tab).

[String/UUID change made/needed]: None

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f2cc06f8089a

Comment 14

[Catalin Suciu [:csuciu]]

Verified as fixed on Nightly 38.0a1 (2015-01-28)

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/37d53b66b10f
https://hg.mozilla.org/releases/mozilla-beta/rev/a4e0de0e6aaa

Comment 16

[Teodora Vermesan (:TeoVermesan)]

The page opened in new tab has the back button grayed out, so:
Verified fixed on:
Device: Alcatel One Touch (Android 4.1.2)
Build: Firefox for Android 37.0a2 (2015-01-29)

Comment 17

[Teodora Vermesan (:TeoVermesan)]

Opened an article from news.google.com in a new tab. The page has the back button grayed out, so:
Verified fixed on:
Device: Nexus 4 (Android 4.4.2)
Build: Firefox for Android 36 Beta 6