1071226 - Refine private browsing new tablet browser toolbar

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Primarily, create and use a private browsing menu icon, then make sure everything else looks okay.

NI :antlam for the icon.

Comment 1

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

Attachment: icon_tabmasq.zip

Attaching icons (the mask and the hat)

Comment 2

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 1: Use same menu asset for private browsing mode

After speaking with Anthony, we decided to use the same menu asset for private browsing and not.

Comment 3

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 1: Same menu icon in pb

Anthony, what do you think?

I think the tabs panel font color may also need to be adjusted.

Comment 4

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 2: Update tabs panel selector icons

This is outside the scope of the toolbar but we had misunderstanding and I have the assets, so I updated these buttons. Note that these assets *will* land on old tablet.

Comment 5

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 2: new tabs panel selector buttons

Comment 6

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Assets in comment 5 on trimage'd.

Comment 7

[Lucas Rocha (:lucasr)]

Comment on attachment 8495626 [details] [diff] [review]
Part 2: Update tabs panel selector icons

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

It's more like adding new tabs panel icons, no? What happened to the old icons? Maybe this needs to be prefixed with new_tablet?

Comment 8

[Lucas Rocha (:lucasr)]

Comment on attachment 8495605 [details] [diff] [review]
Part 1: Use same menu asset for private browsing mode

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

Less resources = win.

Comment 9

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Lucas Rocha (:lucasr) from comment #7)
> It's more like adding new tabs panel icons, no?

Yes, these are tabs panel icons - see comment 4 for the motivation.

> What happened to the old icons? Maybe this needs to be prefixed with new_tablet?

We just used the phone icons. I don't think we should prefix with new_tablet because then we need to have a separate branch in the TabsPanel.initialize method for minimal gains though we could if you really don't want these showing up on old tablet.

Comment 10

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

Comment on attachment 8495607 [details]
Part 1: Same menu icon in pb

+ for the 3 dot not being an awkward gradient purple.

But yes, now the tabs icon needs to change colors to match

Comment 11

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

Splitted bug 1073614 off from this.

Talked on IRC, let's reuse the hat tab and the mask icon from the current tree. I'm going to file a follow up to update those for mobile and tablet.

Comment 12

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 1: Private browsing

(In reply to Anthony Lam (:antlam) from comment #10)
> But yes, now the tabs icon needs to change colors to match

I think that's because the assets from bug 1072466 hadn't landed yet - this screenshot includes them.

Comment 13

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

Comment on attachment 8496143 [details]
Part 1: Private browsing

Nice!

You brought up a good point, I wonder if the '1' should be in a diff color. What colour is it right now? (We don't want to hide it too much)

Comment 14

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 2: Update text color in private browsing and tab strip

:antlam gave me these colors on irc:
  * #777777 for selected and background tabs
  * #f5f5f5 for private selected tabs and tabs panel button.

Comment 15

[Lucas Rocha (:lucasr)]

Comment on attachment 8496288 [details] [diff] [review]
Part 2: Update text color in private browsing and tab strip

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

::: mobile/android/base/resources/color-large-v11/new_tablet_tab_strip_item_title.xml
@@ +3,5 @@
>  	      xmlns:gecko="http://schemas.android.com/apk/res-auto">
>  
>      <item android:state_checked="true"
>            gecko:state_private="true"
> +          android:color="@color/new_tablet_private_browsing_chrome_text"/>

Where's new_tablet_private_browsing_chrome_text defined? Does it actually need to be a new color resource?

Comment 16

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 2: Update text color in private browsing and tab strip

(In reply to Lucas Rocha (:lucasr) from comment #15)
> Where's new_tablet_private_browsing_chrome_text defined? Does it actually
> need to be a new color resource?

Forgot to `hg add` (again).

Comment 17

[Lucas Rocha (:lucasr)]

Comment on attachment 8496900 [details] [diff] [review]
Part 2: Update text color in private browsing and tab strip

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

This looks ok but creating a new color just for private mode on tablets looks a bit smelly. I'd expect us to be able to use one of the text_color_*_inverse colors here...

::: mobile/android/base/resources/values-large-v11/colors.xml
@@ +3,5 @@
> +   - License, v. 2.0. If a copy of the MPL was not distributed with this
> +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +
> +<resources>
> +  <color name="new_tablet_private_browsing_chrome_text">#F5F5F5</color>

Can't we just use one of the existing text_color_* colors here? Just pick one that is close enough to F5F5F5, or maybe just change one of the existing text colors?

Comment 18

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 2: Update text color in private browsing and tab strip

Done: text_color_primary_inverse is unused elsewhere and I doubt we ever want to use #FFF anyway.

Comment 19

[Lucas Rocha (:lucasr)]

Comment on attachment 8496908 [details] [diff] [review]
Part 2: Update text color in private browsing and tab strip

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

Great, thanks.

Comment 20

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

https://hg.mozilla.org/integration/fx-team/rev/fa408884a4e0
https://hg.mozilla.org/integration/fx-team/rev/06a3ad8c7d2c

Comment 21

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

https://hg.mozilla.org/integration/fx-team/rev/c6a2ba054ff8
https://hg.mozilla.org/integration/fx-team/rev/85ba68c632ef

Comment 22

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Ignore comment 20, that push got rejected.

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c6a2ba054ff8
https://hg.mozilla.org/mozilla-central/rev/85ba68c632ef