Closed Bug 1781922 Opened 2 months ago Closed 2 months ago

Icon is clipped on the bottom at SSL Network-Error pages

Categories

(Firefox :: Security, defect)

defect

Tracking

()

VERIFIED FIXED
105 Branch
Tracking Status
firefox-esr91 --- unaffected
firefox-esr102 --- unaffected
firefox103 --- unaffected
firefox104 + verified
firefox105 --- verified

People

(Reporter: dholbert, Assigned: jteow)

References

(Regression)

Details

(Keywords: regression)

Attachments

(3 files)

STR:

  1. Load https://wrong.host.badssl.com/
  2. Look at the icon to the left of the text (yellow-triangle-sign with exclamation point).

ACTUAL RESULTS: The icon is clipped at the bottom.
EXPECTED RESULTS: No such clipping.

mozregression says this was regressed by bug 1737043 -- regression range:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=00c4c63dc51ffbc60674c92e91d0212daa3de7da&tochange=c6cafb2781ca86d06b843adfea89934503f995e9

(And indeed, it looks like that commit changed various bits of neterror markup/styling.)

Flags: needinfo?(gijskruitbosch+bugs)
Summary: Icon is clipped on the bottom at SSL Error pages → Icon is clipped on the bottom at SSL Network-Error pages

[Tracking Requested - why for this release]: unintended visual regression in our UI, which made it to beta but which we have the opportunity to fix (even via backout if needed, worst-case?) before release

Looks like this behavior-change was specifically caused by the last line in this rule:

.title-text {
  font-weight: 300;
  line-height: 1.2;
  margin-bottom: 0;
  padding-bottom: 0;
}

https://searchfox.org/mozilla-central/rev/2be8794dd98b299fcfc5ce66e794e8b85ca14510/browser/themes/shared/aboutNetError.css#242

If I inspect the page in devtools and un-check that padding-bottom decl, then the we give the image a bit more space and don't clip it (i.e. the bug goes away) -- we use the padding-bottom: 0.4em value specified in info-pages.css instead:

.title-text {
  font-size: inherit;
  padding-bottom: 0.4em;
}

https://searchfox.org/mozilla-central/rev/2be8794dd98b299fcfc5ce66e794e8b85ca14510/toolkit/themes/shared/in-content/info-pages.css#62
(That's the value that we're using in old builds, before this regressed.)

We need this padding because the image is drawn using background-image, with background-size: 1.6em; making it draw the image as being nearly as tall as 2 lines of text. So we either need to reduce this background-size decl, or add back the padding-bottom, or otherwise ensure that we're giving the image enough space.

Set release status flags based on info from the regressing bug 1737043

The bug is marked as tracked for firefox104 (beta). However, the bug still isn't assigned.

:ckerschb, could you please find an assignee for this tracked bug? Given that it is a regression and we know the cause, we could also simply backout the regressor. If you disagree with the tracking decision, please talk with the release managers.

For more information, please visit auto_nag documentation.

Flags: needinfo?(ckerschb)

James very courteously kept me as patch author but it wasn't really my patch anymore when it landed, so passing needinfo to James here.

Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(jteow)

(In reply to :Gijs (he/him) from comment #7)

James very courteously kept me as patch author but it wasn't really my patch anymore when it landed, so passing needinfo to James here.

I guess no need for my ni? here ... since James most likely will tackle this one.

Flags: needinfo?(ckerschb)
Assignee: nobody → jteow
Status: NEW → ASSIGNED

Thanks for flagging :dholbert

I was trying to mimic the Figma file that was provided to me but it might be better to keep the spacing consistent across the error pages especially seeing as how there was an unintended consequence such as this case and do more of a holistic tweak if spacing needs to change.

I've submitted a patch and will try uplifting it after it's approved since my change caused a visual regression but I feel should be a low-risk for uplifting.

Flags: needinfo?(jteow)

:jteow any update on landing this patch before early beta is over this week?

Flags: needinfo?(jteow)

:diannaS I was awaiting for the patch to be approved. It was approved so I'm in the midst of landing it and will request an uplift when its landed.

Flags: needinfo?(jteow)
Pushed by jteow@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/62a4c33b5d15
Fix icon from being clipped on network-error pages - r=dao

Comment on attachment 9287506 [details]
Bug 1781922 - Fix icon from being clipped on network-error pages - r?dao

Beta/Release Uplift Approval Request

  • User impact if declined: Users will see a cut-off icon near the title text on certain error states in aboutNetError.xhtml
  • Is this code covered by automated tests?: No
  • Has the fix been verified in Nightly?: No
  • Needs manual test from QE?: Yes
  • If yes, steps to reproduce: Please see https://bugzilla.mozilla.org/show_bug.cgi?id=1781922#c0
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): It's low risk because it's a CSS change and the removal of the overrides (that were added in the first place via the patch that caused the regression) will result in .title-text re-using the CSS it was using before the regression.
  • String changes made/needed:
  • Is Android affected?: No
Attachment #9287506 - Flags: approval-mozilla-beta?
Flags: qe-verify+
Status: ASSIGNED → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → 105 Branch
QA Whiteboard: [qa-triaged]

Reproduced the initial issue using old Nightly from 2022-07-27, verified that the icon is not cut anymore on the latest Nightly 105.0a1 across platforms (macOS 11.6, Windows 10 64bit and Ubuntu 22.04).

I also confirmed that this is fixed for me in latest Nightly, 105.0a1 (2022-08-03).

Thanks!

Status: RESOLVED → VERIFIED

Comment on attachment 9287506 [details]
Bug 1781922 - Fix icon from being clipped on network-error pages - r?dao

Approved for 106.0b6

Attachment #9287506 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Also verified fixed using latest Beta build from treeherder mozilla-beta branch on all platforms (Windows 10, macOS 11.6 and Ubuntu 18.04).

Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.