Refine private browsing new tablet browser toolbar

RESOLVED FIXED in Firefox 35

Status

()

Firefox for Android
General
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mcomella, Assigned: mcomella)

Tracking

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

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 5 obsolete attachments)

8.21 KB, application/zip
Details
2.47 KB, patch
lucasr
: review+
Details | Diff | Splinter Review
158.91 KB, image/png
antlam
: feedback+
Details
3.47 KB, patch
lucasr
: review+
Details | Diff | Splinter Review
Primarily, create and use a private browsing menu icon, then make sure everything else looks okay.

NI :antlam for the icon.
Flags: needinfo?(alam)
Created attachment 8493409 [details]
icon_tabmasq.zip

Attaching icons (the mask and the hat)
Flags: needinfo?(alam)
Created attachment 8495605 [details] [diff] [review]
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.
Attachment #8495605 - Flags: review?(lucasr.at.mozilla)
Assignee: nobody → michael.l.comella
Status: NEW → ASSIGNED
Created attachment 8495607 [details]
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.
Attachment #8495607 - Flags: feedback?(alam)
Created attachment 8495626 [details] [diff] [review]
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.
Attachment #8495626 - Flags: review?(lucasr.at.mozilla)
Created attachment 8495627 [details]
Part 2: new tabs panel selector buttons
Attachment #8495607 - Attachment description: Same menu icon in pb → Part 1: Same menu icon in pb
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 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.
Attachment #8495605 - Flags: review?(lucasr.at.mozilla) → review+
(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 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
Attachment #8495607 - Flags: feedback?(alam) → feedback+
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.
Attachment #8495626 - Attachment is obsolete: true
Attachment #8495626 - Flags: review?(lucasr.at.mozilla)
Created attachment 8496143 [details]
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.
Attachment #8495607 - Attachment is obsolete: true
Attachment #8496143 - Flags: feedback?(alam)
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)
Attachment #8496143 - Flags: feedback?(alam) → feedback+
Flags: needinfo?(michael.l.comella)
Created attachment 8496288 [details] [diff] [review]
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.
Attachment #8496288 - Flags: review?(lucasr.at.mozilla)
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?
Attachment #8496288 - Flags: review?(lucasr.at.mozilla)
Created attachment 8496900 [details] [diff] [review]
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).
Attachment #8496900 - Flags: review?(lucasr.at.mozilla)
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?
Attachment #8496900 - Flags: review?(lucasr.at.mozilla) → feedback+
Created attachment 8496908 [details] [diff] [review]
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.
Attachment #8496908 - Flags: review?(lucasr.at.mozilla)
Flags: needinfo?(michael.l.comella)
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.
Attachment #8496908 - Flags: review?(lucasr.at.mozilla) → review+
https://hg.mozilla.org/mozilla-central/rev/c6a2ba054ff8
https://hg.mozilla.org/mozilla-central/rev/85ba68c632ef
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
You need to log in before you can comment on or make changes to this bug.