1708882 - (Proton) (regression) back/forward an reload icons are no longer correctly aligned in toolbar

Status: RESOLVED FIXED

(Firefox :: Theme, defect, P2)

Description

[Sören Hentzschel]

Attachment: screenshot.png

The back/forward an reload icons are no longer correctly aligned in toolbar. This is a noticeable visual regression in the primary UI. I attached a (zoomed) screenshot that shows both Firefox 89 Beta (top) and Firefox 88 (bottom).

Comment 1

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

Attachment: Bug 1708882 - Center the reload/stop button vertically with respect to the other toolbarbutton icons.

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/260f7bc5d6f6
Center the reload/stop button vertically with respect to the other toolbarbutton icons. r=desktop-theme-reviewers,harry

Comment 3

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/260f7bc5d6f6

Comment 4

[Sören Hentzschel]

This is not fixed (macOS).

Comment 5

[Sören Hentzschel]

Attachment: screenshot

Comment 6

[Sören Hentzschel]

:jaws, please see the attached screenshot. While I am not sure if the position of the icon can be furthed improved (maybe a pixel rounding thing?) it's very noticeable that the background of the icon has a different position on hover.

Comment 7

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

I think we should back this out. I'm on PTO this week. Harry, what do you think?

Comment 8

[Harry Twyford]

If we back this out, the icons will be always be misaligned like in comment 0. IMO, that's worse than the button hover backgrounds being misaligned. I'm in favour of not backing this out. The hover state should definitely be fixed, but that will require changing the reload animation. It's possible that won't get done for 90. If I had to pick one, I'd rather 90 ship with misaligned hover backgrounds rather than the icons always being misaligned.

Seeing as Jared and I disagree here, I think we should get another opinion to break the tie. Dao, thoughts?

Comment 9

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

(In reply to Harry Twyford [:harry] from comment #8)

The hover state should definitely be fixed, but that will require changing the reload animation. It's possible that won't get done for 90. If I had to pick one, I'd rather 90 ship with misaligned hover backgrounds rather than the icons always being misaligned.

I'm happy to fix this but I don't yet see where the problem is being introduced. The hover state simply colors the background of image.toolbarbutton-icon, so a misalignment on hover is just revealing a misalignment of the image or a parentNode in all states. It does appear to be a pixel too low, but all the paddings and margins match up.

Comment 10

[Mathew Hodson]

Since a patch for this bug landed in Firefox 90, I think this bug should be closed so the status is tracked correctly.

The issue in comment 5 may have been fixed by bug 1710328. If there are still issues, I think new bugs should be created.