Remove empty private tabs tray content, replace with simple empty state.

RESOLVED FIXED in Firefox 41

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: antlam, Assigned: mhaigh)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 41
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41 verified)

Details

Attachments

(3 attachments)

(Reporter)

Description

3 years ago
If we do indeed consolidate the two empty states we currently have. I think this one in the "private tabs tray" could be a lot simpler. 

Designs to follow.
(Assignee)

Updated

3 years ago
Assignee: nobody → mhaigh
(Assignee)

Comment 1

3 years ago
Did you get a chance to have a look at this one?
Flags: needinfo?(alam)
(Reporter)

Comment 2

3 years ago
Created attachment 8615580 [details]
prev_pb_tray2.png

While I'd like to take this opportunity here to do something more whimsical here, I don't want to block on that. 

So in the mean time, let's reuse the icon from the empty state and place it in the same spot.
Flags: needinfo?(alam)
(Assignee)

Comment 3

3 years ago
Created attachment 8617319 [details] [diff] [review]
Remove empty private tabs tray content, replace with simple empty state

A load of cleanup in regards to layout, styles and strings around the removal of the private tabs tray empty state.
Attachment #8617319 - Flags: review?(michael.l.comella)
I'd like to point out that in the mock in comment 2, the mask in the panel selector and the empty state are different - expected?
Flags: needinfo?(alam)
(Reporter)

Comment 5

3 years ago
(In reply to Michael Comella (:mcomella) from comment #4)
> I'd like to point out that in the mock in comment 2, the mask in the panel
> selector and the empty state are different - expected?

Keen eye! the one that serves as the navigational element was a placeholder. I must've forgotten to replace it with the new masq icon I made. Since we weren't dealing with that part of the UI for this bug, I'll just leave that here.

This bug is still just about the contents under that bar, the empty state. And maybe changing that highlight to our PB purple if easy.
Flags: needinfo?(alam)
(Assignee)

Comment 6

3 years ago
reminder to self to change image name to private-masq before pushing
Comment on attachment 8617319 [details] [diff] [review]
Remove empty private tabs tray content, replace with simple empty state

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

Sorry for the delay!

Those styles I wrote were pretty garbage - I'm glad we have the opportunity to get rid of them. :)

Make sure the new assets are trimaged.

::: mobile/android/base/resources/layout/private_tabs_panel.xml
@@ +7,5 @@
>         xmlns:gecko="http://schemas.android.com/apk/res-auto">
>  
> +    <FrameLayout android:id="@+id/private_tabs_empty"
> +                 android:layout_width="fill_parent"
> +                 android:layout_height="fill_parent">

nit: -> match_parent

@@ +13,5 @@
> +        <ImageView android:layout_height="wrap_content"
> +                   android:layout_width="wrap_content"
> +                   android:src="@drawable/private_mask"
> +                   android:layout_gravity="center"
> +                   android:layout_marginTop="-10dp"/>

Negative margins are not recommended by lint:

Margin values should be positive. Negative values are generally a sign that you are making assumptions about views surrounding the current one, or may be tempted to turn off child clipping to allow a view to escape its parent. Turning off child clipping to do this not only leads to poor graphical performance, it also results in wrong touch event handling since touch events are based strictly on a chain of parent-rect hit tests. Finally, making assumptions about the size of strings can lead to localization problems. 

---

While I don't think we fall into this category, I'd rather not mute lint warnings for correctness. Could this be marginBottom=10dp instead?
Attachment #8617319 - Flags: review?(michael.l.comella) → review+
(Assignee)

Comment 11

3 years ago
I ended up leaving the -10 margin in there as it was very convoluted to achieve the same result any other way.  I've added a TODO as this hack won't been needed once we land Bug 1161638 and get rid of the browser chrome.
https://hg.mozilla.org/mozilla-central/rev/6d8206038e6c
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
Created attachment 8623586 [details]
Screenshot_2015-06-17-10-41-05.png

Tested with:
Device: Alcatel One Touch (Android 4.1.2)
Build: Firefox for Android 41.0a1 (2015-06-16)
status-firefox41: fixed → verified
You need to log in before you can comment on or make changes to this bug.