Closed Bug 1737754 Opened 7 months ago Closed 6 months ago

[MacOS] "Show you how" text is not centered inside the button

Categories

(Firefox :: General, defect)

Firefox 95
Desktop
macOS
defect

Tracking

()

VERIFIED FIXED
96 Branch
Tracking Status
firefox-esr78 --- unaffected
firefox-esr91 --- unaffected
firefox93 --- unaffected
firefox94 --- unaffected
firefox95 --- verified
firefox96 --- verified

People

(Reporter: sergiu.negritas, Assigned: sfoster)

References

(Regression)

Details

(Keywords: regression)

Attachments

(3 files)

Affected versions

  • 95.0a1 (2021-10-24)

Affected platforms

  • MacOS 10.15

Steps to reproduce

  1. Open Firefox with a new profile.
  2. Open multiple tabs.
  3. Quit firefox.
  4. Reopen firefox with the profile from step 1.

Expected result

  • Infobar is shown and the "Show you how" text is aligned in the center of the button.

Actual Result

  • Infobar is shown and the "Show you how" text is not aligned in the center of the button.

Regression range

Additional notes

  • Only the nightly version is affected by this issue.
Blocks: 1732445, 1724960
Has Regression Range: --- → no
Has STR: --- → yes
Has Regression Range: no → yes
Flags: needinfo?(emilio)

Are we sure this is about one of my patches? The pushlog in comment 0 has a lot of other patches that are more likely culprits...

ni?ing sfoster too since he might know what's going on right away, I'm not particularly familiar with this infobox.

Flags: needinfo?(sfoster)
Flags: needinfo?(lougenia)
Flags: needinfo?(emilio)

Well I think we can rule out Gijs' changes in Bug 1734246 as that patch literally just updated the string. I don't know off the top of my head what's going on here, and it seems odd that only this button would be affected if it was a layout change. Does this reproduce for any of the other infobars? I can investigate, but my first step would be to keep bisecting that pushlog down to a shorter list of candidates.

It looks like there's a (pre-existing) rule for that button that sets some padding at: https://searchfox.org/mozilla-central/source/toolkit/content/widgets/message-bar.css#232-242, I'm not sure if that has significance here?

:sergiu.negritas, since this bug is a regression, could you fill (if possible) the regressed_by field?
For more information, please visit auto_nag documentation.

Flags: needinfo?(sergiu.negritas)

Yeah if I remove this declaration then it looks much better, fwiw.

Apologies, :emilio, looks like I mistakenly pinged you.

Flags: needinfo?(lougenia)

I had a go at getting mozregression to winnow down that pushlog to a likely culprit, but without success yet. From the front-end point of view I'm happy to just land a patch with the changes :emilio pointed out. However it seems like it might be useful for the Layout team to know if there was a regression that caused this to show up, so if anyone is able to pinpoint the regressor that would be great.

Flags: needinfo?(sfoster)

Sergiu, could you try and get a narrower regression range? Thanks

Hey Pascal! Sorry for the long reply time but unfortunately I could not find a narrower regression range unfortunately.
After I place the dates of the regression it just freezes on my mac and it doesn't download any new build.

Flags: needinfo?(sergiu.negritas)

:sfoster what is the status of the patch you were planning to land?

Flags: needinfo?(sfoster)

I can make a patch from :emilio's suggestion in comment #4. That does seem to fix the symptom, but leaves an open question as to why this needing fixing. But answering that question doesn't need to block us here.

Assignee: nobody → sfoster
Status: NEW → ASSIGNED
Flags: needinfo?(sfoster)
Attachment #9251275 - Attachment description: Bug 1737754 - Remove padding override for notification buttons. r?jaws → Bug 1737754 - Remove padding override for notification buttons in the message-bar. r?jaws

This is from MacOS. Its the same on Linux (Ubuntu) and Windows. LGTM.

https://hg.mozilla.org/integration/autoland/rev/f3cf9c079ebb1ad2b5995bb4c99e299229b6e000 looks to be the cause when looking at that regression range.

Flags: needinfo?(pasulaav)
Flags: needinfo?(itiel_yn8)
Regressed by: 1698349

So the fix is correct because before that patch the selector changed in comment 13 had more specificity, right?

Flags: needinfo?(jaws)

(In reply to Emilio Cobos Álvarez (:emilio) from comment #14)

So the fix is correct because before that patch the selector changed in comment 13 had more specificity, right?

Yeah that's exactly right. Before Bug 1698349, the small button (button.small) rule had higher specificity so we got the block padding from it rather than the .notification-button rule in message-bar.css. So, I think the fix here is to add the button back to this selector in common.inc.css. There's a possibility this change has already been worked-around elsewhere but I'm not seeing anything yet.

I'll update my patch. Thanks :jaws for spotting this.

Flags: needinfo?(jaws)

I think that removing the padding is preferable? It never had an effect so it's just dead code.

Attachment #9251275 - Attachment description: Bug 1737754 - Remove padding override for notification buttons in the message-bar. r?jaws → Bug 1737754 - Restore specificity for small buttons rule to fix infobar notification call-to-action buttons. r?jaws!

Agree with emilio here :)

Flags: needinfo?(sfoster)
Flags: needinfo?(pasulaav)
Flags: needinfo?(itiel_yn8)

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

Pushed by sfoster@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ca306f7eb991
Restore specificity for small buttons rule to fix infobar notification call-to-action buttons. r=jaws,desktop-theme-reviewers,dao

(In reply to Tim Nguyen :ntim from comment #17)

Agree with emilio here :)

Gah, sorry. I did phabricator then bugmail so I didn't see this until I pushed.

Flags: needinfo?(sfoster)
Status: ASSIGNED → RESOLVED
Closed: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → 96 Branch

The patch landed in nightly and beta is affected.
:sfoster, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Flags: needinfo?(sfoster)

Comment on attachment 9251275 [details]
Bug 1737754 - Remove padding override for notification buttons. r?jaws

Beta/Release Uplift Approval Request

  • User impact if declined: Text is misaligned in the "Restore session" infobar button.
  • 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: * Reset the browser.startup.couldRestoreSession.count pref to 0 using either a new profile, or about:config.
  • Start Firefox, close it and start it again
  • You should get a infobar prompt indicating how to enable session restore.

ER: The "Show me how" text on the button should be correctly aligned (vertically centered .)

  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): The patch reverts a 1-line inadvertent CSS change which only applies to these infobar buttons
  • String changes made/needed: None
Attachment #9251275 - Attachment description: Bug 1737754 - Restore specificity for small buttons rule to fix infobar notification call-to-action buttons. r?jaws! → Bug 1737754 - Remove padding override for notification buttons. r?jaws
Flags: needinfo?(sfoster)
Attachment #9251275 - Flags: approval-mozilla-beta?
Flags: qe-verify+
QA Whiteboard: [qa-triaged]

Comment on attachment 9251275 [details]
Bug 1737754 - Remove padding override for notification buttons. r?jaws

Low risk, approved for our last 95 beta, thanks.

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

Hello! I have managed to reproduce the issue with firefox 95.0a1(2021-10-24) on MacOS 10.15, I can confirm that the issue is fixed in firefox 96.0a1(2021-11-24).

I will check the beta fix this friday when the next beta release is scheduled.

Thank you

Hello! Since firefox 95.0b12 has been released today I can confirm that the issue is fixed with this version on macOS 10.15.

Marking this issue as VERIFIED->FIXED and I will update the corresponding flags accordingly.

Thank you!

Status: RESOLVED → VERIFIED
QA Whiteboard: [qa-triaged]
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.