1411290 - Text to block/allow pop-ups in identity manager is heavily truncated in localized builds

Status: VERIFIED FIXED

(Firefox :: Site Identity, defect, P1)

Description

[Francesco Lodolo [:flod]]

Attachment: Screenshot (Italian build)

Italian is not much longer than English (Blocca vs Block, Permetti vs Allow), and yet the controls are unusable, since the label is truncated and not readable.

Comment 1

[Francesco Lodolo [:flod]]

Sorry for the noise, clone the wrong bug (I meant to clone and block bug 1331931).

Comment 2

[:prathiksha]

Attachment: Bug 1411290 - Make menulist label text visible for popup permission in identity manager in localized builds.

Review commit: https://reviewboard.mozilla.org/r/192594/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/192594/

Comment 3

[Francesco Lodolo [:flod]]

Comment on attachment 8921576 [details]
Bug 1411290 - Make menulist label text visible for popup permission in identity manager in localized builds.

Sorry, not really entitled to give feedback on this kind of code changes ;-)

Comment 4

[Johann Hofmann [:johannh]]

Comment on attachment 8921576 [details]
Bug 1411290 - Make menulist label text visible for popup permission in identity manager in localized builds.

https://reviewboard.mozilla.org/r/192594/#review197980

I'm a bit hesitant about this, but I'd say it's generally ok with me. I'm a bit annoyed that the menulist changes size when selecting a different item, but I don't think we have to spend too much time or thought on this. :)

::: browser/themes/shared/notification-icons.inc.css
(Diff revision 1)
>    list-style-image: url("chrome://browser/skin/notification-icons/popup-subitem.svg");
>    margin-inline-start: 30px;
>  }
>  
> -.identity-popup-popup-menulist {
> -  width: 72px;

Maybe we should consider changing this to min-width? That would at least prevent jumping on some locales, e.g. English, and prevent the menulist from getting tiny when the word for allow/block is only a few letters.

Comment 5

[Dão Gottwald [:dao]]

(In reply to Johann Hofmann [:johannh] from comment #4)
> > -.identity-popup-popup-menulist {
> > -  width: 72px;
> 
> Maybe we should consider changing this to min-width? That would at least
> prevent jumping on some locales, e.g. English, and prevent the menulist from
> getting tiny when the word for allow/block is only a few letters.

If we want to keep a min-width here, we should use em rather than px for that.

Comment 6

[:prathiksha]

Comment on attachment 8921576 [details]
Bug 1411290 - Make menulist label text visible for popup permission in identity manager in localized builds.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/192594/diff/1-2/

Comment 7

[Johann Hofmann [:johannh]]

Comment on attachment 8921576 [details]
Bug 1411290 - Make menulist label text visible for popup permission in identity manager in localized builds.

https://reviewboard.mozilla.org/r/192594/#review198756

Seems reasonable, though I'd say we should give it a little (1em?) more min-width.

Thank you!

Comment 8

[:prathiksha]

Comment on attachment 8921576 [details]
Bug 1411290 - Make menulist label text visible for popup permission in identity manager in localized builds.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/192594/diff/2-3/

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d60b8971ecd9
Make menulist label text visible for popup permission in identity manager in localized builds. r=johannh

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 11

[roxleitan]

Reproduced this issue on Nightly 58.oa1 (2017-10-24).

Verified fixed using the latest Nightly 59.0a1 (2017-11-13)and FF Beta 58.0b3 on Windows 10 x64, Ubuntu 16.04 x64 and Mac OS X 10.12.