1489581 - Content Blocking "Disabled" badge's text is vertically misaligned (not quite centered, not quite aligned with icon)

Status: VERIFIED FIXED

(Firefox :: Site Identity, defect, P1)

Description

[Daniel Holbert [:dholbert]]

Attachment: screenshot

STR:
 1. In Firefox Preferences "Privacy & Security", toggle Content Blocking to "Off"
 2. Visit https://www.cnn.com/
 3. Click the site badge (e.g. the lock icon) at left of URL bar.

ACTUAL RESULTS:
The site-ID doorhanger "Content Blocking | Disabled" badge is awkwardly misaligned. In particular:
 - Within its red background, there's very little padding above the text, and lots of padding below the text. (not sure it's CSS padding; I'm just describing it visually as "padding"
 - The triangle-exclamation-point icon is slightly lower than the text.  It looks like it wants to be baseline-aligned, but isn't quite aligned.

EXPECTED RESULTS:
 - The "Disabled" text should probably be one or two pixels lower, to be vertically centered in the red area & to be baseline-aligned with the icon.

Comment 1

[Daniel Holbert [:dholbert]]

Looks like this label was added in bug 1476218 -- adding dependency on that bug, and setting ni=johann for thoughts here.

Comment 2

[Daniel Holbert [:dholbert]]

I poked around in Browser Toolbox, and found a few starters here:

 (1) the text is in a <label> which has an explicit |style="height:23px"| attribute on the element. That doesn't seem to have any effect, so I'm not sure why it's there. (Probably not relevant to this bug, but might be worth cleaning up as part of this if it's indeed unneeded.)

 (2) This label also has the following rule applied -- and if I turn off the hardcoded-pixel-value "line-height" decl, the text shifts down slightly to what looks like a better position:
>  #identity-popup-content-blocking-disabled-label > label {
>      margin: 0;
>      line-height: 18px;
>  }

Comment 3

[Johann Hofmann [:johannh]]

Thanks for filing this bug! I was initially only under the impression that this affected Linux, but it seems both Windows and Linux are affected...

We should really fix that for 63 (not sure why you marked it unaffected?).

(In reply to Daniel Holbert [:dholbert] from comment #2)
> I poked around in Browser Toolbox, and found a few starters here:
> 
>  (1) the text is in a <label> which has an explicit |style="height:23px"|
> attribute on the element. That doesn't seem to have any effect, so I'm not
> sure why it's there. (Probably not relevant to this bug, but might be worth
> cleaning up as part of this if it's indeed unneeded.)

Unfortunately it's not that easy. This height is added in https://searchfox.org/mozilla-central/source/browser/components/customizableui/PanelMultiView.jsm#1283, which is a bit delicate hack to work around resizing calculation in that popup.

In this case we might not actually need the override, we should probably just make sure that the label can't be multiline.

I think that removing this height is quite important for getting this fixed across platforms.

>  (2) This label also has the following rule applied -- and if I turn off the
> hardcoded-pixel-value "line-height" decl, the text shifts down slightly to
> what looks like a better position:
> >  #identity-popup-content-blocking-disabled-label > label {
> >      margin: 0;
> >      line-height: 18px;
> >  }

Yes, I think we can remove line-height (and add a better way to vertically center) when we got rid of the height...

Comment 4

[Daniel Holbert [:dholbert]]

(In reply to Johann Hofmann [:johannh] from comment #3)
> We should really fix that for 63 (not sure why you marked it unaffected?).

Not sure why either. Perhaps by mistake? In any case, thanks for taking!

Comment 5

[Johann Hofmann [:johannh]]

Attachment: Bug 1489581 - Improve vertical centering inside the content blocking "disabled" label. r=paolo

Labels in the identity popup that are potentially multiline get dynamic heights set
as part of the descriptionHeightWorkaround. This causes some cross-platform glitches
in vertically centering the icon and the label of the disabled indicator.

The disabled label doesn't really need to be multiline, so we avoid doing that. This
also means that we need to make some changes to handle long "Disabled" labels a little
more gracefully, but looking at existing translations of the word "Disabled" we won't
run into trouble: https://transvision.mozfr.org/string/?entity=browser/chrome/browser/browser.dtd:contentBlocking.disabled.label&repo=gecko_strings

Comment 6

[:Paolo Amadini]

Comment on attachment 9009624 [details]
Bug 1489581 - Improve vertical centering inside the content blocking "disabled" label. r=paolo

:Paolo Amadini has approved the revision.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f8bc1d2ef0fc
Improve vertical centering inside the content blocking "disabled" label. r=paolo

Comment 8

[Arthur Iakab [arthur_iakab]]

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

Comment 9

[Johann Hofmann [:johannh]]

Daniel, would you mind checking your latest Nightly to see if it looks good now? I'd like to uplift this. Thanks!

Comment 10

[Daniel Holbert [:dholbert]]

Attachment: screenshot in nightly after patch landed

Yeah, this looks better, I think (see screenshot).

Thanks!

Comment 11

[Johann Hofmann [:johannh]]

Great, thanks, filing uplift request now...

Comment 12

[Johann Hofmann [:johannh]]

Comment on attachment 9009624 [details]
Bug 1489581 - Improve vertical centering inside the content blocking "disabled" label. r=paolo

Approval Request Comment
[Feature/Bug causing the regression]: 1476218
[User impact if declined]: This fixes bug 1492536 and a slightly misaligned label in identity UI in all locales
[Is this code covered by automated tests?]: No, visual change only
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: Not really
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: Simplifying a bit of CSS in the identity popup
[String changes made/needed]: None

Comment 13

[Pascal Chevrel:pascalc]

Comment on attachment 9009624 [details]
Bug 1489581 - Improve vertical centering inside the content blocking "disabled" label. r=paolo

Low risk patch verified on Nightly, uplift approved for 63 beta 8, thanks.

Comment 14

[Dorel Luca [:dluca]]

https://hg.mozilla.org/releases/mozilla-beta/rev/84cbec409512

Comment 15

[David Olah]

Verified the bug on Nightly 64.0a1 (2018-09-30) and 63.0b10 on Mac OS, Windows 10 and Ubuntu 16.04. Fixed.