Closed Bug 1002303 Opened 10 years ago Closed 10 years ago

Provide a description on private tabs page if there are no private tabs

Categories

(Firefox for Android Graveyard :: General, defect)

All
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 33

People

(Reporter: wesj, Assigned: mcomella)

References

Details

Attachments

(9 files, 7 obsolete files)

122.13 KB, image/png
Details
14.83 KB, patch
lucasr
: review+
Details | Diff | Splinter Review
74.03 KB, image/png
antlam
: feedback+
Details
271.92 KB, image/png
antlam
: feedback+
Details
215.88 KB, image/png
antlam
: feedback+
Details
108.68 KB, image/png
antlam
: feedback+
Details
3.42 KB, patch
lucasr
: review+
Details | Diff | Splinter Review
20.28 KB, patch
mcomella
: review+
Details | Diff | Splinter Review
1.75 KB, patch
eedens
: review+
Details | Diff | Splinter Review
      No description provided.
Ooh, good idea.  There's a plan in place to show things in the Synced Tabs tray, and great progress in place: https://bugzilla.mozilla.org/show_bug.cgi?id=958889.
This bug isn't quite ready for action, but I'm interested in mentoring this.  We'll need to get UX feedback to get a visual mockup to see what the description would look like; and we should wait for Bug 958889 to land, since that's going to provide a model for how to implement this "panel that sometimes looks very different".
Whiteboard: [mentor=nalexander][lang=java][good second bug]
Since this is actionable, I'd like UX to take a crack at a mockup and have someone start on it ASAP.

Ian, Anthony: Can one of you take a crack.
Flags: needinfo?(ibarlow)
Flags: needinfo?(alam)
OS: Windows 7 → Android
Hardware: x86_64 → All
(In reply to Nick Alexander :nalexander from comment #1)
> Ooh, good idea.  There's a plan in place to show things in the Synced Tabs
> tray, and great progress in place:
> https://bugzilla.mozilla.org/show_bug.cgi?id=958889.

To Nick's point, I would definitely like to use a similar layout for the private tab (when it's empty). I'll mock something up for you guys but I want to ask what the "goal" of this page would be? so that I can write appropriate content.
Flags: needinfo?(ibarlow)
At the very least, just a simple "Your private tabs show up here" message to provide a hint what this is. I wondered for a bit if we'd be better to put the data from about:privatebrowsing in here (and show Top Sites like normal in private mode instead of showing about:privatebrowsing?). i.e. it reads something like:

====
Private Browsing

Firefox won't keep any browser history, search history, download history, web form history, cookies, or temporary internet files for tabs in here. However, files you download and bookmarks you make will be kept.

While this computer won't have a record of your browsing history, your internet service provider or employer can still track the pages you visit.
====

it also has a "Learn more" link. That interaction likely won't work well for people who create private tabs other ways though and never see the empty tabs tray.
Desktop also has a nifty about:privatebrowsing page if you open in in a non-private window:

=====
Would you like to start Private Browsing?

You are not currently in a private window.

In a Private Browsing window, Nightly won't keep any browser history, search history, download history, web form history, cookies, or temporary internet files. However, files you download and bookmarks you make will be kept.

To start Private Browsing, you can also select New Private Window from the menu.
=====
(In reply to Anthony Lam from comment #4)

> To Nick's point, I would definitely like to use a similar layout for the
> private tab (when it's empty). I'll mock something up for you guys but I
> want to ask what the "goal" of this page would be? so that I can write
> appropriate content.

I think the main goal is to let people know what Private Tabs can do and how they block history. We have some text in about:privatebrowsing that might be useful.
Attached image Tray Vertical.png
Here is a mock for content! It would ideally just follow the same style template for text that we are using on the sync side.

Note: The 'add a new tab' icon is different to what we have but it's not final at all, i just included it to see what it would look like.

I also want to point out that we have this bug: https://bugzilla.mozilla.org/show_bug.cgi?id=864958
Flags: needinfo?(alam)
Assignee: nobody → michael.l.comella
Status: NEW → ASSIGNED
Whiteboard: [mentor=nalexander][lang=java][good second bug]
Note, in particular, my changes to TabsPanelItem.Button.
Attachment #8432893 - Flags: review?(lucasr.at.mozilla)
Attached image Phone landscape (obsolete) —
Same styles as sync.

Removed the header because there wasn't much room.
Attachment #8432903 - Flags: feedback?(alam)
Comment on attachment 8432901 [details]
Phone portrait

Looking good!
Attachment #8432901 - Flags: feedback?(alam) → feedback+
Comment on attachment 8432903 [details]
Phone landscape

This looks a bit off. The line-height of the copy on the left definitely needs to be increased. Could we also try vertically raising the "Want to learn more?" link to be more centered?

I would also like to move the "active" tab a bit further down to open up these horizontal screens but I feel like that is going to open up a nest of other problems.
Attachment #8432903 - Flags: feedback?(alam) → feedback-
Comment on attachment 8432893 [details] [diff] [review]
Part 1: Rename RemoteTabs* styles to be used with all tabs panels.

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

Nice.
Attachment #8432893 - Flags: review?(lucasr.at.mozilla) → review+
Design questions:
- Isn't this empty state a bit too verbose? Maybe the text should be more concise?
- It would be nice if we replaced the longer description with a simpler empty state once the user has created and closed a few private tabs. Seeing this description all the time might get a bit annoying. Maybe in a follow-up?
(In reply to Lucas Rocha (:lucasr) from comment #15)
> Design questions:
> - Isn't this empty state a bit too verbose? Maybe the text should be more
> concise?
> - It would be nice if we replaced the longer description with a simpler
> empty state once the user has created and closed a few private tabs. Seeing
> this description all the time might get a bit annoying. Maybe in a follow-up?

I agree - I would like to follow up these visual inconsistencies and establish a more solid design framework in a later bug.
Comment on attachment 8433696 [details] [diff] [review]
Part 0: Add empty private browsing view description strings.

Hey, Ryan. Anthony said you might know where to point the "Want to learn more" link in his mock from comment 8 - where should it link the user to?

Clearing review until this link is updated.
Attachment #8433696 - Flags: review?(lucasr.at.mozilla)
Flags: needinfo?(rfeeley)
Comment on attachment 8433722 [details] [diff] [review]
Part 2: Show a description page when there are no private tabs.

These styles are getting stupidly messy because I'm trying to make them inherit where they perhaps should not be inheriting.

Assuming antlam likes my tablet design, TabsPanelItem.TextAppeareance.Header.HiddenInLandscape.PrivateTabs is ambiguous - on phone, we hide the title in landscape, not in portrait. On tablet, it's the reverse! I don't know how to make this obvious without "...HiddenInLandscape.ReversedOnTablet", which seems really dumb. Alteratively, don't make it an inheritance (i.e. remove the ".").

Similiar issues are abound with the 2ColCentered layout, and anything appended with "PrivateTabs". Do you have any suggestions?
Attachment #8433722 - Flags: feedback?(lucasr.at.mozilla)
Comment on attachment 8433722 [details] [diff] [review]
Part 2: Show a description page when there are no private tabs.

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

Looks generally good but yes, try to avoid all these style inheritances as they are not easy to follow and likely to be broken by the next person changing them due all the dependencies.
Attachment #8433722 - Flags: feedback?(lucasr.at.mozilla) → feedback+
Removed inheritance as much as I felt reasonable, as per comment 23.
Attachment #8435370 - Flags: review?(lucasr.at.mozilla)
Attached image Phone landscape v2
(In reply to Anthony Lam (:antlam) from comment #13)
> Comment on attachment 8432903 [details]
> Phone landscape
> 
> This looks a bit off. The line-height of the copy on the left definitely
> needs to be increased.

I didn't touch this because it may get stomped on with l10n and small screen sizes. I filed bug 1021356 for the visual refinements.

> Could we also try vertically raising the "Want to
> learn more?" link to be more centered?

Done.
Attachment #8432903 - Attachment is obsolete: true
Attachment #8435375 - Flags: feedback?(alam)
Comment on attachment 8435375 [details]
Phone landscape v2

This is looking better
Attachment #8435375 - Flags: feedback?(alam) → feedback+
Attachment #8433693 - Flags: feedback?(alam) → feedback+
Comment on attachment 8435370 [details] [diff] [review]
Part 2: Show a description page when there are no private tabs.

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

Looking good, just needs to change the structure of PrivateTabsPanel a bit.

::: mobile/android/base/resources/layout/private_tabs_panel.xml
@@ +12,5 @@
> +        android:visibility="gone">
> +
> +    <org.mozilla.gecko.tabspanel.TabsTray android:id="@+id/private_tabs_tray"
> +                                          style="@style/TabsList"
> +                                          android:layout_width="fill_parent"

fill_parent -> match_parent for consistency?

::: mobile/android/base/tabspanel/PrivateTabsPanel.java
@@ +28,5 @@
> +        super(context, attrs);
> +    }
> +
> +    @Override
> +    public void onFinishInflate() {

This composite view looks good overall but I'd rather avoid relying on onFinishInflate() as I've seen device-specific bugs where the inflation callbacks are not very reliable (e.g. callbacks called more than once, not called at all, etc). So, let's tweak the structure of this view to be more like, say, TwoLinePageRow:

1. Inflate the inner layout in the constructor (change private_tabs_panel.xml to have a 'merge' toplevel tag)
2. Initialize view members in the constructor
3. Use view tag (org.mozilla.gecko.tabspanel.PrivateTabsPanel) directly in tabs_panel.xml instead of using an include
Attachment #8435370 - Flags: review?(lucasr.at.mozilla) → feedback+
Comment on attachment 8433692 [details]
Tablet portrait

I think this is good for now.. as mentioned before, we're probably still going to have to take a look at all these trays at a later date.
Attachment #8433692 - Flags: feedback?(alam) → feedback+
Flags: needinfo?(rfeeley)
Hey, Ryan. Anthony said you might know where to point the "Want to learn more" link in his mock from comment 8 - where should it link the user to?
Flags: needinfo?(rfeeley)
Updated for lucas' comments.
Attachment #8439607 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 8439607 [details] [diff] [review]
Part 2: Show a description page when there are no private tabs.

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

Nice, thanks.

::: mobile/android/base/resources/layout-large-land-v11/tabs_panel.xml
@@ -39,5 @@
>                                                android:choiceMode="singleChoice"
>                                                android:visibility="gone"
>                                                gecko:tabs="tabs_normal"/>
>  
> -        <org.mozilla.gecko.tabspanel.TabsTray android:id="@+id/private_tabs"

It seems you forgot to update this file.
Attachment #8439607 - Flags: review?(lucasr.at.mozilla) → review+
Got the link from Ian via email.
Attachment #8440175 - Flags: review?(lucasr.at.mozilla)
Updated the patch to fill in the locale of the link.
Attachment #8440178 - Flags: review?(lucasr.at.mozilla)
Attachment #8440175 - Flags: review?(lucasr.at.mozilla) → review+
Comment on attachment 8440178 [details] [diff] [review]
Part 2: Show a description page when there are no private tabs.

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

Looks good with the fix in layout-large-land-v11/tabs_panel.xml

::: mobile/android/base/resources/layout-large-land-v11/tabs_panel.xml
@@ +39,5 @@
>                                                android:choiceMode="singleChoice"
>                                                android:visibility="gone"
>                                                gecko:tabs="tabs_normal"/>
>  
> +        <include layout="@layout/private_tabs_panel"/>

This should be <org.mozilla.gecko.tabspanel.PrivateTabsPanel ...> directly here instead of the include.
Attachment #8440178 - Flags: review?(lucasr.at.mozilla) → review+
Flags: needinfo?(rfeeley)
>+<!ENTITY private_tabs_panel_description "Your private tabs will show up here. While we don\'t keep any form of your browsing history or cookies, files that you download will still be saved on your device.">

Files that you download, and bookmarks you make will be kept. Also, I suggest dropping 'form' to not mix it up with 'form history'.
"Your private tabs will show up here. While we don\'t keep any of your browsing history or cookies, bookmarks and any files that you download will still be saved on your device."

Anthony, what do you think of the adjustments I made to the issues brought up in comment 38?
Flags: needinfo?(alam)
(In reply to Michael Comella (:mcomella) from comment #39)
> "Your private tabs will show up here. While we don\'t keep any of your
> browsing history or cookies, bookmarks and any files that you download will
> still be saved on your device."
> 
> Anthony, what do you think of the adjustments I made to the issues brought
> up in comment 38?

"Your private tabs will show up here. While we don't keep any of your browsing history or cookies, bookmarks and files that you download will still be saved on your device."

I removed the "any" because I don't really think that's necessary :)
Flags: needinfo?(alam)
Comment on attachment 8443651 [details] [diff] [review]
Part 3: Update empty private tabs panel description.

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

Actually... :)

See the above comments on the reasons for this change.
Attachment #8443651 - Flags: review?(wjohnston) → review?(eedens)
Attachment #8443651 - Attachment is obsolete: true
Attachment #8443651 - Flags: review?(eedens)
Comment on attachment 8443688 [details] [diff] [review]
Part 3: Update empty private tabs panel description.

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

Looks good to me -- text matches the text from antlam's previous comment
Attachment #8443688 - Flags: review?(eedens) → review+
Depends on: 1029989
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: