1122074 - "Normal Tabs" tray has an empty state

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Steps to reproduce
---
 1) Go into Tabs tray
 2) Swap to Private Tabs tray
 3) Create a new Private Tab
 4) Return to Private Tabs tray
 5) Switch over to Normal Tabs tray
 6) Last Normal Tab can be removed!


Device
---
Nexus 7, Kit Kat

Comment 1

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

(In reply to Anthony Lam (:antlam) from comment #0)

>  6) Last Normal Tab can be removed!

We allow this since it's not the last tab (you still have a private tab). If you want an empty state info graphic in the "normal tabs" tray, that's fine. Not tracking this though.

Comment 2

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

We should just restore a normal tab then (just like on mobile). I want to avoid creating a normal tab tray empty state right now without carefully considering the entire picture cross device/platforms.

Comment 3

[Martyn Haigh (:mhaigh)]

Attachment: Normal Tabs tray has an empty state

Make sure that there's always at least one open 'normal' tab open, else open one.

Comment 4

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

Comment on attachment 8552489 [details] [diff] [review]
Normal Tabs tray has an empty state

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

lgtm, w/ fix for "only if removed, right?" or a good explanation.

::: mobile/android/base/tabs/TabsGridLayout.java
@@ +231,5 @@
>                  }
> +
> +                final Tabs tabsInstance = Tabs.getInstance();
> +
> +                if (tab.isPrivate() == mIsPrivate && mTabsAdapter.getCount() > 0) {

nit: We already make this check above - perhaps we should move this code block (sans redundant check) inside the if statement above.

@@ +239,5 @@
> +                    }
> +                }
> +
> +                // Make sure we always have at least one normal tab
> +                if (!tab.isPrivate()) {

We should only do this if we successfully removed a tab, right?

@@ +241,5 @@
> +
> +                // Make sure we always have at least one normal tab
> +                if (!tab.isPrivate()) {
> +                    final Iterable<Tab> tabs = tabsInstance.getTabsInOrder();
> +                    boolean lastNormalTab = true;

nit: -> removedTabIsLastNormalTab

Comment 5

[Martyn Haigh (:mhaigh)]

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

Comment 6

[Martyn Haigh (:mhaigh)]

Attachment: Normal Tabs tray has an empty state

Approval Request Comment
[Feature/regressing bug #]:
Prevent the new tablet tabs tray getting in to a state where a user has no open 'normal' tabs

[User impact if declined]:
The user could reach a state where they have no open 'normal' tabs

[Describe test coverage new/current, TreeHerder]:
Local testing

[Risks and why]: 
The code touches parts of the new tablet tab panel close functionality - potentially we run the risk that closing tabs may not work.

[String/UUID change made/needed]:
N/A

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0c7496cca523

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/408f892a2f0b
https://hg.mozilla.org/releases/mozilla-beta/rev/c1e9f11144a5

Comment 9

[Cristina Madaras, QA [:CristinaM]]

Verified as fixed in builds:
Firefox for Android 38.0a1 (2015-01-26)
Firefox for Android 37.0a2 (2015-01-26)

Device: Asus Transformer Pad TF300T (Android 4.2.1)

Comment 10

[Cristina Madaras, QA [:CristinaM]]

Verified as fixed in Firefox for Android 36 Beta 4;
Device: Asus Transformer Pad TF300T (Android 4.2.1).