Closed Bug 1489581 Opened Last year Closed Last year

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

Categories

(Firefox :: Site Identity, defect, P1)

defect

Tracking

()

VERIFIED FIXED
Firefox 64
Tracking Status
firefox62 --- unaffected
firefox63 --- verified
firefox64 --- verified

People

(Reporter: dholbert, Assigned: johannh)

References

Details

(Keywords: regression, Whiteboard: [privacy-panel-64][triage])

Attachments

(3 files)

Attached image 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.
Summary: Content blocking "Disabled" badge is vertically misaligned (not quite centered, not quite aligned with icon) → Content Blocking "Disabled" badge's text is vertically misaligned (not quite centered, not quite aligned with icon)
Looks like this label was added in bug 1476218 -- adding dependency on that bug, and setting ni=johann for thoughts here.
Component: General → Site Identity and Permission Panels
Depends on: 1476218
Flags: needinfo?(jhofmann)
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;
>  }
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...
Assignee: nobody → jhofmann
Blocks: 1476218
Status: NEW → ASSIGNED
No longer depends on: 1476218
Flags: needinfo?(jhofmann)
Priority: -- → P1
Whiteboard: [privacy-panel-64][triage]
(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!
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 on attachment 9009624 [details]
Bug 1489581 - Improve vertical centering inside the content blocking "disabled" label. r=paolo

:Paolo Amadini has approved the revision.
Attachment #9009624 - Flags: review+
Pushed by jhofmann@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f8bc1d2ef0fc
Improve vertical centering inside the content blocking "disabled" label. r=paolo
https://hg.mozilla.org/mozilla-central/rev/f8bc1d2ef0fc
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 64
Daniel, would you mind checking your latest Nightly to see if it looks good now? I'd like to uplift this. Thanks!
Flags: needinfo?(dholbert)
Yeah, this looks better, I think (see screenshot).

Thanks!
Flags: needinfo?(dholbert) → needinfo?(jhofmann)
Attachment #9010306 - Attachment is patch: false
Attachment #9010306 - Attachment mime type: text/plain → image/png
Status: RESOLVED → VERIFIED
Great, thanks, filing uplift request now...
Flags: needinfo?(jhofmann)
Blocks: 1492536
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
Attachment #9009624 - Flags: approval-mozilla-beta?
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.
Attachment #9009624 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Verified the bug on Nightly 64.0a1 (2018-09-30) and 63.0b10 on Mac OS, Windows 10 and Ubuntu 16.04. Fixed.
You need to log in before you can comment on or make changes to this bug.