Closed Bug 1122074 Opened 5 years ago Closed 5 years ago

"Normal Tabs" tray has an empty state

Categories

(Firefox for Android :: General, defect)

x86
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 38
Tracking Status
firefox36 --- verified
firefox37 --- verified
firefox38 --- verified
fennec - ---

People

(Reporter: antlam, Assigned: mhaigh)

References

Details

Attachments

(1 file, 1 obsolete file)

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
tracking-fennec: --- → ?
(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.
tracking-fennec: ? → -
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.
Assignee: nobody → mhaigh
Status: NEW → ASSIGNED
Make sure that there's always at least one open 'normal' tab open, else open one.
Attachment #8552489 - Flags: review?(michael.l.comella)
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
Attachment #8552489 - Flags: review?(michael.l.comella) → review+
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
Attachment #8553767 - Flags: approval-mozilla-beta?
Attachment #8553767 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/0c7496cca523
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 38
Attachment #8553767 - Flags: approval-mozilla-beta?
Attachment #8553767 - Flags: approval-mozilla-beta+
Attachment #8553767 - Flags: approval-mozilla-aurora?
Attachment #8553767 - Flags: approval-mozilla-aurora+
Attachment #8552489 - Attachment is obsolete: true
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)
Verified as fixed in Firefox for Android 36 Beta 4;
Device: Asus Transformer Pad TF300T (Android 4.2.1).
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.