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

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[Negritas Sergiu, Desktop QA]

Attachment: textalign SHOWYOUHOW.png

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

• last good build: https://hg.mozilla.org/mozilla-central/rev/ef7b596bb385f46280eb1492009bf8fb541e17db

• first bad build: https://hg.mozilla.org/mozilla-central/rev/3cf700ab977a298e19a00f1ec66d14939766955b

• Here is the regression range: "https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=ef7b596bb385f46280eb1492009bf8fb541e17db&tochange=3cf700ab977a298e19a00f1ec66d14939766955b"

Additional notes

• Only the nightly version is affected by this issue.

Comment 1

[Emilio Cobos Álvarez [: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.

Comment 2

[Sam Foster [:sfoster] (he/him)]

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?

Comment 3

[BugBot [:suhaib / :marco/ :calixte]]

: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.

Comment 4

[Emilio Cobos Álvarez [:emilio]]

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

Comment 5

[Lougenia Bailey]

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

Comment 6

[Sam Foster [:sfoster] (he/him)]

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.

Comment 7

[Pascal Chevrel (relman team) -> :pascalc]

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

Comment 8

[Negritas Sergiu, Desktop QA]

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.

Comment 9

[Lougenia Bailey]

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

Comment 10

[Sam Foster [:sfoster] (he/him)]

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.

Comment 11

[Sam Foster [:sfoster] (he/him)]

Attachment: Bug 1737754 - Remove padding override for notification buttons. r?jaws

Comment 12

[Sam Foster [:sfoster] (he/him)]

Attachment: Screenshot of the notification with patch applied on MacOS

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

Comment 13

[Jared Wein [:jaws] (please needinfo? me)]

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

Comment 14

[Emilio Cobos Álvarez [:emilio]]

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

Comment 15

[Sam Foster [:sfoster] (he/him)]

(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.

Comment 16

[Emilio Cobos Álvarez [:emilio]]

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

Comment 17

[Tim Nguyen :ntim]

Agree with emilio here :)

Comment 18

[BugBot [:suhaib / :marco/ :calixte]]

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

Comment 19

[Pulsebot]

Pushed by [e-mail]:
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

Comment 20

[Sam Foster [:sfoster] (he/him)]

(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.

Comment 21

[Narcis Beleuzu [:NarcisB]]

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

Comment 22

[BugBot [:suhaib / :marco/ :calixte]]

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.

Comment 23

[Sam Foster [:sfoster] (he/him)]

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

Comment 24

[Pascal Chevrel (relman team) -> :pascalc]

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

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

Comment 25

[Pascal Chevrel (relman team) -> :pascalc]

https://hg.mozilla.org/releases/mozilla-beta/rev/d12b8ece56e8

Comment 26

[Negritas Sergiu, Desktop QA]

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

Comment 27

[Negritas Sergiu, Desktop QA]

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!