1600281 - Strange alignment of container buttons in preferences

Status: VERIFIED FIXED

(Firefox :: Settings UI, defect, P1)

Description

[Tim Nguyen :ntim]

"Preferences" and "Remove" buttons are weirdly aligned.

STR: visit about:preferences#containers

Comment 1

[Tim Nguyen :ntim]

Attachment: Screenshot

Comment 2

[Alice0775 White]

Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=a066931fc331c4511871812cffa4cab7de7169f6&tochange=c59883012b35833cca413bdd5f660b886d912072

Comment 3

[Tim Nguyen :ntim]

Removing https://searchfox.org/mozilla-central/source/browser/components/preferences/in-content/containers.js#67 would fix this, but it'd be nice to check if anything else is affected by this.

Comment 4

[Tim Nguyen :ntim]

Brian, could you please take a look ?

Comment 5

[Tim Nguyen :ntim]

Oh, I see what's wrong. The "Rewrite [align="left"] to [align="start"] and [align="right"] to [align="end"]" patch is wrong.

align="left" maps to pack="start" for horizontal boxes (-moz-box-orient: horizontal) and align="start" for vertical boxes.
Similarly, align="right" maps to pack="end" for horizontal boxes and align="end" for vertical boxes.

See: https://hg.mozilla.org/mozilla-central/rev/c58aa1b00988#l1.89

It would be nice to re-review all the files changed in https://hg.mozilla.org/mozilla-central/rev/c59883012b35 to make sure nothing else regressed.

Comment 6

[:Gijs (he/him)]

[Tracking Requested - why for this release]:
User-visible regression that we'll need to fix for release.

Comment 7

[Brian Grinstead [:bgrins]]

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

Oh, I see what's wrong. The "Rewrite [align="left"] to [align="start"] and [align="right"] to [align="end"]" patch is wrong.

align="left" maps to pack="start" for horizontal boxes (-moz-box-orient: horizontal) and align="start" for vertical boxes.
Similarly, align="right" maps to pack="end" for horizontal boxes and align="end" for vertical boxes.

See: https://hg.mozilla.org/mozilla-central/rev/c58aa1b00988#l1.89

It would be nice to re-review all the files changed in https://hg.mozilla.org/mozilla-central/rev/c59883012b35 to make sure nothing else regressed.

Thanks, I'll have a look.

I guess the same issue could have happened with the change from align="top" -> align="start" in Bug 1599283, but the only elements that got updated in https://hg.mozilla.org/integration/autoland/rev/ad75d8bb97ca are horizontally aligned so it uses the align attribute for valignment: https://hg.mozilla.org/mozilla-central/rev/c58aa1b00988#l1.89.

Comment 8

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #7)

I guess the same issue could have happened with the change from align="top" -> align="start" in Bug 1599283, but the only elements that got updated in https://hg.mozilla.org/integration/autoland/rev/ad75d8bb97ca are horizontally aligned so it uses the align attribute for valignment: https://hg.mozilla.org/mozilla-central/rev/c58aa1b00988#l1.89.

I think I misread this. align=top would have hit the early return at https://hg.mozilla.org/mozilla-central/rev/c58aa1b00988#l1.77 so pack would never be used.

Comment 9

[Brian Grinstead [:bgrins]]

Attachment: Bug 1600281 - Rewrite horizontal boxes that were incorrectly translated from align=right to align=end to use pack=end

• align="left" maps to pack="start" for horizontal boxes and align="start" for vertical boxes.
• align="right" maps to pack="end" for horizontal boxes and align="end" for vertical boxes.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9ed78435ef6d
Rewrite horizontal boxes that were incorrectly translated from align=right to align=end to use pack=end r=Gijs

Comment 11

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/9ed78435ef6d

Comment 12

[Hani Yacoub, Desktop QA]

Verified as fixed on Firefox 73.0a1 (2019-12-03) on Windows 10 x 64, Mac OS X 10.14 and on Ubuntu 18.04 x64.

Comment 13

[Tim Nguyen :ntim]

Comment on attachment 9112987 [details]
Bug 1600281 - Rewrite horizontal boxes that were incorrectly translated from align=right to align=end to use pack=end

Beta/Release Uplift Approval Request

• User impact if declined: Various elements are misaligned in the UI (attachment 9112522 [details], attachment 9111390 [details])
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?:
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Straightforward XUL patch
• String changes made/needed:

Comment 14

[Tim Nguyen :ntim]

This might be worth porting to TB, summary is in comment 5 :)

Comment 15

[Magnus Melin [:mkmelin]]

Thx, Richard already noticed and has a patch in bug 1600928.

Comment 16

[Julien Cristau [:jcristau]]

Comment on attachment 9112987 [details]
Bug 1600281 - Rewrite horizontal boxes that were incorrectly translated from align=right to align=end to use pack=end

fix a new regression in preferences, verified in 73, approved for 72.0b3

Comment 17

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/releases/mozilla-beta/rev/05a20bf0606f

Comment 18

[Hani Yacoub, Desktop QA]

Verified as fixed on Firefox 72.0b3 (Build ID: 20191204104712) on Windows 10 x 64, Mac OS X 10.14 and on Ubuntu 18.04 x64.

Comment 19

[Firefox Bug Husbandry Bot]

Please specify a root cause for this bug. See :tmaity for more information.