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

VERIFIED FIXED in Firefox 36

Status

()

VERIFIED FIXED
4 years ago
2 years ago

People

(Reporter: csuciu, Assigned: mcomella)

Tracking

({regression})

36 Branch
Firefox 38
All
Android
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox35 unaffected, firefox36+ verified, firefox37 verified, firefox38 verified, fennec36+)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

4 years ago
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
[Tracking Requested - why for this release]:
tracking-fennec: --- → ?
tracking-firefox36: --- → ?
Keywords: regression, regressionwindow-wanted
(Reporter)

Comment 2

4 years ago
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.
status-firefox37: unaffected → affected
status-firefox38: unaffected → affected
Keywords: regressionwindow-wanted

Updated

4 years ago
Blocks: 960746
We need to make sure devs have devices that can reproduce the bug.
Assignee: nobody → michael.l.comella
tracking-fennec: ? → 36+
Status: NEW → ASSIGNED
Hardware: ARM → All
(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.
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
Created attachment 8554064 [details] [diff] [review]
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.
Attachment #8554064 - Flags: review?(mhaigh)
(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.
Summary: Back button is active in new tabs → [tablet] Tabs opened in background take selected tab's back/forward button state if switched to before loading is completed
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 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.
Attachment #8554064 - Flags: review?(mhaigh) → review+
Created attachment 8554804 [details] [diff] [review]
Get tab state from tab in arguments; fix bug in forward/back button state

Updated patch.
Attachment #8554064 - Attachment is obsolete: true
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
Attachment #8554804 - Flags: approval-mozilla-beta?
Attachment #8554804 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/f2cc06f8089a
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 38

Updated

4 years ago
Flags: needinfo?(catalin.suciu)
Keywords: verifyme
Attachment #8554804 - Flags: approval-mozilla-beta?
Attachment #8554804 - Flags: approval-mozilla-beta+
Attachment #8554804 - Flags: approval-mozilla-aurora?
Attachment #8554804 - Flags: approval-mozilla-aurora+
(Reporter)

Comment 14

4 years ago
Verified as fixed on Nightly 38.0a1 (2015-01-28)
Status: RESOLVED → VERIFIED
status-firefox38: affected → verified
Flags: needinfo?(catalin.suciu)
Keywords: verifyme
tracking-firefox36: ? → +
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)
status-firefox37: fixed → verified
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
status-firefox36: fixed → verified
You need to log in before you can comment on or make changes to this bug.