1022405 - Not enough padding on OSX translation infobar

Status: VERIFIED FIXED

(Firefox :: Translations, defect)

Description

[:Felipe Gomes (needinfo for replies!)]

Attachment: Current infobar

The padding on the infobar is supposed to be larger than it currently is. See attached screenshot for details.

I was testing the WIP patches from bug 988480 and they had the correct padding, so I think it's something that regressed in the last versions of the patch.

Comment 1

[:Felipe Gomes (needinfo for replies!)]

Attachment: Intended padding

Comment 2

[Michael Maslaney [:mmaslaney] ([e-mail])]

The info bar should have a height of 40px and the buttons 26px. These measurements should provide ample padding.

Comment 3

[Steven MacLeod [:smacleod]]

Attachment: Patch - Adjust infobar and button height to increase padding

Comment 4

[Steven MacLeod [:smacleod]]

Attachment: Padding After Patch

Comment 5

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8448413 [details] [diff] [review]
Patch - Adjust infobar and button height to increase padding

Sending it over to Florian, as I won't be able to look at this this week

Comment 6

[Florian Quèze [:florian]]

Comment on attachment 8448413 [details] [diff] [review]
Patch - Adjust infobar and button height to increase padding

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

After some UX discussions in #translation with Sevaan, I think the values given in comment 2 aren't the best we can do.

::: browser/themes/osx/browser.css
@@ +3617,5 @@
>    border-top: none;
>    border-bottom: 1px solid #c4c4c4;
>    padding-top: 1px;
>    padding-bottom: 1px;
> +  min-height: 40px;

Let's make this 35px so that the padding at the top and bottom of the buttons is the same as the padding between the 2 buttons.

@@ +3632,5 @@
>    border-color: rgba(23, 51, 78, 0.15) rgba(23, 51, 78, 0.17) rgba(23, 51, 78, 0.2);
>    box-shadow: 0px 0px 2px rgba(255, 255, 255, 0.5) inset, 0px 1px 0px rgba(255, 255, 255, 0.2);
>    transition-property: background-color, border-color, box-shadow;
>    transition-duration: 150ms;
> +  min-height: 26px;

Let's revert this to 22px so that the buttons and the menulist have the same height.

Comment 7

[Florian Quèze [:florian]]

Attachment: Screenshot with values in comment 6

For future reference, this is what the infobar looked like when I tried the values I gave in comment 6. Sevaan looked at this screenshot on IRC and said "35 looks great. That height feels nicely balanced to me. [...] I like it. That feels good."

Comment 8

[Steven MacLeod [:smacleod]]

Attachment: Patch - Adjust infobar height to increase padding. v2

Updated patch. Confirmed that it looks just like the screenshot Florian provided.

Comment 9

[Florian Quèze [:florian]]

Comment on attachment 8450481 [details] [diff] [review]
Patch - Adjust infobar height to increase padding. v2

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

Thanks! We also want this on aurora.

Comment 10

[Steven MacLeod [:smacleod]]

Comment on attachment 8450481 [details] [diff] [review]
Patch - Adjust infobar height to increase padding. v2

Approval Request Comment
[Feature/regressing bug #]: This bug is part of the automatic translation feature, which we want to A/B with a subset of Aurora 32 users, and later Beta 32 users.
[User impact if declined]: The translation infobar will be short and cluttered on Mac.
[Describe test coverage new/current, TBPL]: none.
[Risks and why]: almost none, trivial CSS-only change.
[String/UUID change made/needed]: none.

Comment 11

[Steven MacLeod [:smacleod]]

https://hg.mozilla.org/integration/fx-team/rev/af3f3e97aff2

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/af3f3e97aff2

Comment 13

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

Comment on attachment 8450481 [details] [diff] [review]
Patch - Adjust infobar height to increase padding. v2

This is a trivial fix so Aurora+. However, my understanding is that the translation trial is already live and that translation will not ship with 32 so I don't know that users will get much benefit from this fix.

Comment 14

[Florian Quèze [:florian]]

(In reply to Lawrence Mandel [:lmandel] from comment #13)

> However, my understanding is that the
> translation trial is already live and that translation will not ship with 32
> so I don't know that users will get much benefit from this fix.

The pre-trial with German users on 32 aurora is live, but the real trial is expected to happen on 32 beta.

Comment 15

[Bogdan Maris, Desktop Test Engineering]

Verified as fixed on Mac OS X 10.9.4 and Mac OS X 10.8.5 using latest Nightly. Leaving [qa+] until this fix lands in Aurora as well.

Comment 16

[Steven MacLeod [:smacleod]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/cfae9fc4009f

Comment 17

[Bogdan Maris, Desktop Test Engineering]

Confirmed that this is verified on latest Aurora as well.