Closed Bug 1021356 Opened 5 years ago Closed 5 years ago

Refine visuals in empty private tabs screen

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 33
Tracking Status
firefox30 --- unaffected
firefox31 --- unaffected
firefox32 --- fixed
firefox33 --- verified

People

(Reporter: mcomella, Assigned: mcomella)

References

Details

Attachments

(7 files, 10 obsolete files)

114.31 KB, image/png
Details
68.62 KB, image/png
antlam
: feedback+
Details
79.34 KB, image/png
antlam
: feedback+
Details
72.39 KB, image/png
antlam
: feedback+
Details
10.86 KB, patch
lucasr
: review+
Details | Diff | Splinter Review
8.33 KB, patch
lucasr
: review+
Details | Diff | Splinter Review
8.90 KB, patch
lucasr
: review+
Details | Diff | Splinter Review
Some desired changes, quoted via bug 1002303 comment 13:
  * The line-height of the copy on the left definitely needs to be increased
  * 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.
Don't forget to check for small screen and l10n issues, like bug 1007442.
Assignee: nobody → michael.l.comella
Status: NEW → ASSIGNED
Attached image Portrait (obsolete) —
I increased the line spacing for all views (including the remote tabs panel). See the other attached files.

Anthony, what do you think?
Attachment #8443793 - Flags: feedback?(alam)
Attached image Landscape (obsolete) —
Attachment #8443795 - Flags: feedback?(alam)
Attached image RemoteTabs portrait (obsolete) —
Attachment #8443796 - Flags: feedback?(alam)
Comment on attachment 8443796 [details]
RemoteTabs portrait

What font are you using here Michael? It seems a bit off to me. It might help if I knew what font size and line height measurements you were using :)
Comment on attachment 8443793 [details]
Portrait

Same questions about the font size and line height for you here :)
Comment on attachment 8443795 [details]
Landscape

Line height seems excessive here especially if I try to imagine what it would look like if the length of this copy extends even just a little. Can we also try to center the "Want to learn more?" (equal padding top and bottom)?
Attachment #8443795 - Flags: feedback?(alam) → feedback-
Attachment #8443796 - Flags: feedback?(alam) → feedback-
Attachment #8443793 - Flags: feedback?(alam) → feedback-
Attached image Landscape w/ bounds
(In reply to Anthony Lam (:antlam) from comment #6)
> Comment on attachment 8443796 [details]
> RemoteTabs portrait
> 
> What font are you using here Michael?

It should be the Android default, Roboto [1]. I'm not sure which style (thin, light, regular, etc.). this is, however.

> It might help if I knew what font size and line height measurements you were using :)

The font size is 16dp, or 16 density-independent pixels. For a more thorough explanation, see [2].

As for line height, that is declared through a multiplier which is currently 1.2. I can get the exact pixel measurements if you need them.

(In reply to Anthony Lam (:antlam) from comment #7)
> Same questions about the font size and line height for you here :)

They currently share the same styles and so the values should be equivalent.

(In reply to Anthony Lam (:antlam) from comment #8)
> Line height seems excessive here especially if I try to imagine what it
> would look like if the length of this copy extends even just a little.

What do you mean by the length of this copy?
> Can we also try to center the "Want to learn more?"
> (equal padding top and bottom)?

The padding is actually equal on the top and the bottom, to the tab edge, but it doesn't align with the center of the text to the left, which probably gives it this illusion. See the attached file for screenshot with the view bounds.

What do you think I should do here?

[1]: https://developer.android.com/design/style/typography.html
[2]: https://developer.android.com/guide/practices/screens_support.html#terms
Flags: needinfo?(alam)
(In reply to Michael Comella (:mcomella) from comment #9)
> It should be the Android default, Roboto [1]. I'm not sure which style
> (thin, light, regular, etc.). this is, however.

We should be using Regular.

> The font size is 16dp, or 16 density-independent pixels. For a more thorough
> explanation, see [2].

Body font and header font should be different in size. Let's try 16 dp for the header and call-to-action button text and use 14 dp for everything else? I checked over my older files and unfortunately I made them in a pretty old school manner so I can't give you those measurements as they wouldn't round nicely.

Let's try these :)

> As for line height, that is declared through a multiplier which is currently
> 1.2. I can get the exact pixel measurements if you need them.

Can we use 1.5 as the multiplier? That usually makes for the best readability wise but we might still have to tweak it as we go.
Flags: needinfo?(alam)
Attached image Portrait v2 (obsolete) —
Anthony, I made the changes - what do you think?
Attachment #8443793 - Attachment is obsolete: true
Attachment #8449061 - Flags: feedback?(alam)
Attached image RemoteTabs portrait v2 (obsolete) —
Attachment #8443796 - Attachment is obsolete: true
Attachment #8449062 - Flags: feedback?(alam)
For the record, the headers and call-to-action button font size was 20dp in the previous revision.
Comment on attachment 8443792 [details] [diff] [review]
Empty private tabs panel visual refinements.

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

I assume this is to avoid truncating the view's content on smaller screens or while in landscape orientation?
Attachment #8443792 - Flags: review?(lucasr.at.mozilla) → review+
(In reply to Lucas Rocha (:lucasr) from comment #14)
> I assume this is to avoid truncating the view's content on smaller screens
> or while in landscape orientation?

Primarily, yes.
Comment on attachment 8449062 [details]
RemoteTabs portrait v2

The bounding width of the text box where the "sign in to sync your tabs..." seems awfully wide. Can we make that the width of the button?
Attachment #8449062 - Flags: feedback?(alam) → feedback-
Comment on attachment 8449061 [details]
Portrait v2

Same thing WRT the width of the text field.

Can we also equate the padding that exists above and below the "Private browsing" title?

Thanks Michael!
Attachment #8449061 - Flags: feedback?(alam) → feedback-
Attached image RemoteTabs portrait v3 (obsolete) —
Attachment #8449062 - Attachment is obsolete: true
Attachment #8451909 - Flags: feedback?(alam)
Attached image Portrait v3 (obsolete) —
Attachment #8449061 - Attachment is obsolete: true
Attachment #8451912 - Flags: feedback?(alam)
Attached image Landscape v2 (obsolete) —
Attachment #8443795 - Attachment is obsolete: true
Attachment #8451913 - Flags: feedback?(alam)
Attachment #8451909 - Flags: feedback?(alam) → feedback+
Comment on attachment 8451913 [details]
Landscape v2

Yeah, that overflow is a bit of an issue.

What're our options when it comes to pushing the "active tab" down there further down? I feel like this may have larger implications.

Out of curiosity, does using a ratio of 1.35 for the line-height help this at all here?
Attachment #8451913 - Flags: feedback?(alam) → feedback-
Comment on attachment 8451912 [details]
Portrait v3

This is looking good. I am on the fence ATM when it comes to the visual relationship between the header and the body. I feel like font size difference may not be enough to create a hierarchy here. 

Can we try 18dp for the header? Sorry Michael, I realize I've previously asked for 16dp. The body can remain at 14dp. I think this would really drive it home. :)
Attachment #8451912 - Flags: feedback?(alam) → feedback-
Attached image RemoteTabs portrait v4
The styles are shared between panels, so here's the updated RemoteTabs panel.
Attachment #8451909 - Attachment is obsolete: true
Attachment #8453359 - Flags: feedback?(alam)
Attached image Portrait v4
Attachment #8451912 - Attachment is obsolete: true
Attachment #8453360 - Flags: feedback?(alam)
Attached image Landscape v3 (obsolete) —
I'm unfortunately not sure how to lower the currently open tab, but we can consider that in a followup bug, if you'd like.
Attachment #8451913 - Attachment is obsolete: true
Attachment #8453362 - Flags: feedback?(alam)
Comment on attachment 8453359 [details]
RemoteTabs portrait v4

Looks good! Thanks Michael
Attachment #8453359 - Flags: feedback?(alam) → feedback+
Comment on attachment 8453360 [details]
Portrait v4

Nice! Looks better. :)
Attachment #8453360 - Flags: feedback?(alam) → feedback+
Comment on attachment 8453362 [details]
Landscape v3

This is getting there, I think if we just equate the padding above and below the copy on the left, we can wrap this up for now.
Attachment #8453362 - Flags: feedback?(alam) → feedback-
Attached image Landscape v4
Attachment #8453362 - Attachment is obsolete: true
Attachment #8455655 - Flags: feedback?(alam)
Updated styles as per Anthony's comments; Functionally equivalent.
Attachment #8455657 - Flags: review?(lucasr.at.mozilla)
Attachment #8443792 - Attachment is obsolete: true
Comment on attachment 8455655 [details]
Landscape v4

awesome!
Attachment #8455655 - Flags: feedback?(alam) → feedback+
Attachment #8455657 - Flags: review?(lucasr.at.mozilla) → review+
Forgot to include r=lucasr on the patch in comment 32.
Back-port the changes to Aurora, only for the remote tabs panel. Note that I
forgot to do tablets, so this patch includes the tablet changes (okayed by
antlam in-person). I'll make a part 2 to deal with this on trunk.
Attachment #8455810 - Flags: review?(lucasr.at.mozilla)
There were a few extra changes over the patch in comment 34 because we now have to work with the private tabs panel which shares the same styles. Note that this patch does not fix bug 1037740.
Attachment #8455832 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 8455810 [details] [diff] [review]
(aurora) Remote tabs tray visual refinements to match trunk

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

As long as antlam approves :-)
Attachment #8455810 - Flags: review?(lucasr.at.mozilla) → review+
Comment on attachment 8455832 [details] [diff] [review]
Part 2: Empty private tabs panel visual refinements for tablet

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

Good.
Attachment #8455832 - Flags: review?(lucasr.at.mozilla) → review+
https://hg.mozilla.org/mozilla-central/rev/0bc4b2de1f95
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 33
Reopening for part 2.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment on attachment 8455810 [details] [diff] [review]
(aurora) Remote tabs tray visual refinements to match trunk

Approval Request Comment
[Feature/regressing bug #]:
  Initially implemented the remote tabs tray in bug 958889, bug 1014999 (tablet), and various other refinement bugs. The style of these trays changed again with the creation of the private tabs tray (this bug) - this patch uplifts these stylistic changes.

[User impact if declined]:
  Users will see one version of the remote tabs tray in 32, and a visual refresh in 33.

[Describe test coverage new/current, TBPL]:
  None, though these are purely style changes.

[Risks and why]:
  We exclusively change the styles used by the remote tabs tray so in the worst case, the remote tabs tray appeareance is broken, and the tray can become completely useless.

[String/UUID change made/needed]: None
Attachment #8455810 - Flags: approval-mozilla-aurora?
Comment on attachment 8455810 [details] [diff] [review]
(aurora) Remote tabs tray visual refinements to match trunk

Aurora+
Attachment #8455810 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/e9d78c3d2eb5
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Depends on: 1039738
Verified as fixed in build 33 beta 2;
Devices:
- Google Nexus 7 (Android 4.4.4);
- Samsung Galaxy R (Android 2.3.4).
You need to log in before you can comment on or make changes to this bug.