1007442 - Tune visual for not-signed-in remote tabs tray

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

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

I spoke with antlam and nalexander on IRC and we decided to land the functionality and clean up the visual aspects in this followup.

One outlier, via bug 958889 comment 6:
> It's looking good. I would focus on the alignment of the baseline of "Using
> an older version of Sync?" and "...Bookmarks, passwords & more."

Keep in mind how l10n will change this.

Comment 3

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

Some nits Anthony had in previous bugs:
  * Bug 1014999 comment 9: On tablets, the "Get Started" button should be the same width as the "Welcome to Sync" text - consider defining this button (and other buttons) in absolute sizes, rather than as padding
  * Bug 958889 comment 56 (same as comment 0): In the dual column layout of the Setup screen, "the copy that reads "Using an old version of Sync?" and "Sign in to sync your tabs bookmarks and more" should have their bottoms align."

Additionally, try different languages, and correct for locale issues.

Anthony, have you had a chance to take a look at this in Nightly? Is there anything else you might want to add? Note that the tablet version should first be in the 5/31 Nightly.

Comment 4

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

Also, different screen sizes!

Comment 5

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

Attachment: Part 1: Add scrolling to remote tabs tray where applicable.

Comment 6

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

Attachment: Part 2: Make remote tabs panel buttons more consistent across devices.

Having padding set the button width made the button size dependent on the text size, so I went with match_parent and margins this time around.

Comment 7

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

Attachment: Small device, large padding

I think this much paddingTop looks a bit silly on small devices, so I'm adjusting that in part 3.

Comment 8

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

Attachment: imported patch _3-padding_top

Comment 9

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

Attachment: Part 3: Reduce padding at the top of portrait remote tabs panel.

Comment 10

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

Comment on attachment 8436211 [details] [diff] [review]
Part 3: Reduce padding at the top of portrait remote tabs panel.

Actually, Lucas is a little more in touch with these styles.

Comment 11

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

Attachment: Small device, small padding

The aftermath.

Comment 12

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

Attachment: Large device, small padding

Comment 13

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

Comment on attachment 8436206 [details] [diff] [review]
Part 2: Make remote tabs panel buttons more consistent across devices.

Forgot tablet love.

Comment 14

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

Anthony, what do you think of the decision to change the padding from the attachment in comment 7 to the attachments in comment 11 and comment 12?

Comment 15

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

Attachment: Xlarge setup button (portrait)

Comment 16

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

Attachment: Xlarge setup button (landscape)

Comment 17

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

Comment on attachment 8437251 [details]
Xlarge setup button (portrait)

I'm not too concerned about the landscape version.

Comment 18

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

Attachment: Large setup button (portrait)

Large device is the Kobo Arc, the Xlarge device is a Galaxy Tab 3.

Note that the button is created by filling the screen with hard-coded margins, so it will not change sizes with l10n (with exception to line wrapping), but will change sizes with screen sizes. I'm not sure if this is the best practice - I'll get feedback from Lucas in the review.

Comment 19

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

Attachment: Large setup button (landscape)

Comment 20

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

Attachment: Part 4: Adjust button sizes across large/xlarge devices.

Comment 21

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

Attachment: Part 2: Make remote tabs panel buttons more consistent across devices.* * *

Forgot I was supposed to merge part 4 with part 2. Lucas, is layout_width="match_parent" with hard-coded margins an okay approach to defining button sizes?

Comment 22

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

(In reply to Michael Comella (:mcomella) from comment #14)
> Anthony, what do you think of the decision to change the padding from the
> attachment in comment 7 to the attachments in comment 11 and comment 12?

Yeah that works.

(In reply to Michael Comella (:mcomella) from comment #17)
> Comment on attachment 8437251 [details]
> Xlarge setup button (portrait)
> 
> I'm not too concerned about the landscape version.

The text wrapping could be done better here. Can we get the line to wrap after "tabs,"?

Also, we might want to try padding further from the tab since on this device it seems the active tab is a long way's down.

(In reply to Michael Comella (:mcomella) from comment #18)
> Created attachment 8437256 [details]
> Large setup button (portrait)
> 
> Large device is the Kobo Arc, the Xlarge device is a Galaxy Tab 3.
> 
> Note that the button is created by filling the screen with hard-coded
> margins, so it will not change sizes with l10n (with exception to line
> wrapping), but will change sizes with screen sizes. I'm not sure if this is
> the best practice - I'll get feedback from Lucas in the review.

Can we get the line to wrap after "tabs," as well?

Comment 23

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

Lucas, is it reasonable to define the width of a view in absolute units (as opposed to match_parent with margin sizes)? Using dp, this would allow us to ensure the width of the text is relatively consistent (and thus readable, like a consistent page width) across devices. Since each layout has a minimum size of dp, we can use this information that the width is never cut off across devices. For context, read on.

(In reply to Anthony Lam (:antlam) from comment #22)
> The text wrapping could be done better here. Can we get the line to wrap
> after "tabs,"?

Currently, the width of this text is decided by margins, meaning this wrapping is particular to this device (and other similar size/density screens). If we change the margins, we change the size of the text across devices (and less relevantly, languages). Is this okay?

Alternatively, I asked Lucas above whether or not we can define absolute widths for the text column, so we can wait until we see what he has to say.

Comment 24

[Lucas Rocha (:lucasr)]

(In reply to Michael Comella (:mcomella) from comment #23)
> Lucas, is it reasonable to define the width of a view in absolute units (as
> opposed to match_parent with margin sizes)? Using dp, this would allow us to
> ensure the width of the text is relatively consistent (and thus readable,
> like a consistent page width) across devices. Since each layout has a
> minimum size of dp, we can use this information that the width is never cut
> off across devices. For context, read on.
> 
> (In reply to Anthony Lam (:antlam) from comment #22)
> > The text wrapping could be done better here. Can we get the line to wrap
> > after "tabs,"?
> 
> Currently, the width of this text is decided by margins, meaning this
> wrapping is particular to this device (and other similar size/density
> screens). If we change the margins, we change the size of the text across
> devices (and less relevantly, languages). Is this okay?
> 
> Alternatively, I asked Lucas above whether or not we can define absolute
> widths for the text column, so we can wait until we see what he has to say.

I wouldn't go to far in terms of enforcing specific line wrapping behaviour here because this is almost guaranteed to look different in each locale. If we *really* want this, it would be better to just add a newline character to the string itself with a matching l10n comment explaining the UI guidelines around it. Please check with Pike if this is a valid l10n practice.

Comment 25

[Lucas Rocha (:lucasr)]

Comment on attachment 8437266 [details] [diff] [review]
Part 2: Make remote tabs panel buttons more consistent across devices.* * *

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

Looks good.

Comment 26

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

(In reply to Lucas Rocha (:lucasr) from comment #24)
> > (In reply to Anthony Lam (:antlam) from comment #22)
> > > The text wrapping could be done better here. Can we get the line to wrap
> > > after "tabs,"?
> > 
> > Currently, the width of this text is decided by margins, meaning this
> > wrapping is particular to this device (and other similar size/density
> > screens). If we change the margins, we change the size of the text across
> > devices (and less relevantly, languages). Is this okay?
> > 
> > Alternatively, I asked Lucas above whether or not we can define absolute
> > widths for the text column, so we can wait until we see what he has to say.
> 
> I wouldn't go to far in terms of enforcing specific line wrapping behaviour
> here because this is almost guaranteed to look different in each locale. If
> we *really* want this, it would be better to just add a newline character to
> the string itself with a matching l10n comment explaining the UI guidelines
> around it. Please check with Pike if this is a valid l10n practice.

I agree - that's just how I've been keeping tabs on cross-device spacial alignment in my head but I get that it doesn't make sense from an implementation point of view.

Comment 27

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

remote:   https://hg.mozilla.org/integration/fx-team/rev/8ef009362d13
remote:   https://hg.mozilla.org/integration/fx-team/rev/1e480afce4b3
remote:   https://hg.mozilla.org/integration/fx-team/rev/83be53bc8db9

Anthony, this should land in tomorrow's Nightly (6/12) - do you want to give it a spin and ensure all there are no more changes to make?

Comment 28

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/8ef009362d13
https://hg.mozilla.org/mozilla-central/rev/1e480afce4b3
https://hg.mozilla.org/mozilla-central/rev/83be53bc8db9

Comment 29

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

Comment on attachment 8437251 [details]
Xlarge setup button (portrait)

Spacing along the bottom is a bit concerning but I think it should be considered as a part of a bigger issue.

Comment 30

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

Marking FIXED as there are no more feedback requests for antlam.

Comment 31

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

Comment on attachment 8436191 [details] [diff] [review]
Part 1: Add scrolling to remote tabs tray where applicable.

This request applies to parts 1-3.

[Approval Request Comment]
Bug caused by (feature/regressing bug #):
  Adding the setup and verification panels to the remote tabs panel (see dependent bugs)

User impact if declined:
  Users will see an unpolished version of the remote tabs panel UI. Additionally, users of small screen devices or long languages will be unable to see the full content (a scroll view was added). 
 
Testing completed (on m-c, etc.):
  Local

Risk to taking this patch (and alternatives if risky):
  Low - these are mostly formatting changes. However, these patches *do* introduce a bug which is solved by the attachment in bug 1024778 comment 1, so that should be uplifted too.

String or IDL/UUID changes made by this patch: None

Comment 32

[Cristina Madaras, QA [:CristinaM]]

Verified as fixed in
Build: 33.0a1 (2014-06-16)

Devices:
Samsung S3 (Android 4.3)
Samsung Galaxy R (Android 2.3.4)
Motorola Razr (Android 4.0.4)
Asus Transformer Pad TF300T (Android 4.2.1)
AGPTEK TP10A (Android 4.0.4)

Comment 33

[Lawrence Mandel [:lmandel] (use needinfo)]

I'm unclear from the uplift request whether this change ended up having l10n impact such as that described in comment 18 and comment 24. Does the l10n team need to know about this change?

Comment 34

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

(In reply to Lawrence Mandel [:lmandel] from comment #33)
> I'm unclear from the uplift request whether this change ended up having l10n
> impact such as that described in comment 18 and comment 24. Does the l10n
> team need to know about this change?

No, in comment 18 we were discussing how the View layouts might change with longer or shorter text. Ideally, this would be little to none, so the l10n team will likely have nothing to add.

In comment 24, we were discussing using newlines to force word wrapping but decided this didn't make sense given variable screen sizes, didn't make the change, and thus do not need l10n input.

Comment 35

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8436191 [details] [diff] [review]
Part 1: Add scrolling to remote tabs tray where applicable.

Thanks for the confirmation about no l10n impact. Aurora approval granted.

Comment 36

[:Gijs (he/him)]

remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/44ef5ff8a390
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/3b319d395f50
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/71f5fa1512d2

Comment 37

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8436211 [details] [diff] [review]
Part 3: Reduce padding at the top of portrait remote tabs panel.

Missed setting approval earlier.

Comment 38

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8437266 [details] [diff] [review]
Part 2: Make remote tabs panel buttons more consistent across devices.* * *

Missed setting approval earlier.