1521312 - Titlebar window controls are too close to window edge after overriding reversed placement when ui.GtkCSDReversedPlacement is set

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect)

Description

[Kestrel]

Attachment: window-controls-margin.png

After correcting Bug 1521310 on Ubuntu 18.10 by creating ui.GtkCSDReversedPlacement = 0 and restarting, the window controls are too close to the edge of the window. The titlebar-buttonbox has a 7px border on the left side when it should be on the right side.

Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=529a105310435244ef67c078a810ad92b22d9327&tochange=7d151a27de64cce8b0ed96443c1888656341e420

Regressed by: Bug 1433667

Comment 2

[Kestrel]

Problem still exists after Bug 1519606 when using ui.GtkCSDReversedPlacement to override placement.

Comment 3

[Kestrel]

Attachment: border-wrong-side.png

Comment 4

[Tim Spurway [:tspurway]]

:jimm, do you think this will need to be fixed for beta 66?

Comment 5

[Jim Mathies [:jimm]]

(In reply to Tim Spurway [:tspurway] from comment #4)

:jimm, do you think this will need to be fixed for beta 66?

More of a UX decision, feels like polish but it does seem pretty jaring. Maybe Martin can give us an opinion here.

Comment 6

[Martin Stránský [:stransky] (ni? me)]

Can you provide a full reproduction steps for latest nightly? Because I can't reproduce that. Setting ui.GtkCSDReversedPlacement has no effect and switching left/right button placement works correctly (after browser restart).

ui.GtkCSDReversedPlacement toggle without browser restart does not trigger on-theme-change event so the button borders are not regenerated for the reversed placement. But I don't think ui.GtkCSDReversedPlacement should be even used for that.

Comment 7

[Kestrel]

STR1:

1. Ubuntu 18.10 Yaru theme (controls on right by default), go to about:config and create ui.GtkCSDReversedPlacement = 1.
2. Restart browser.
3. Placement is reversed (left side) but 7px border padding is on right side.

STR2:

1. Ubuntu 18.10 Yaru theme, use Tweaks to put controls on left, go to about:config and create ui.GtkCSDReversedPlacement = 0.
2. Restart browser.
3. Placement is reversed (right side) but 7px border padding is on left side.

Comment 8

[Martin Stránský [:stransky] (ni? me)]

I think the ui.GtkCSDReversedPlacement should be removed as the preference is loaded from system settings.

When ui.GtkCSDReversedPlacement is set only ans system used a different icont placement the buttons borders are calculated incorrectly as code at toolkit level is not aware of the ui.GtkCSDReversedPlacement variable.

Is the ui.GtkCSDReversedPlacement needed anyway?

Comment 9

[Martin Stránský [:stransky] (ni? me)]

Attachment: Bug 1521312 - [Linux/Gtk+] Remove ui.GtkCSDReversedPlacement as it's broken and useless r=dao

Comment 10

[Kestrel]

The hidden ui prefs are intended to override system settings which can be useful for customization and troubleshooting. They are not expected to work flawlessly so I would be more inclined to keep the pref and WONTFIX the cosmetic issue.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/de00632953fa
[Linux/Gtk+] Remove ui.GtkCSDReversedPlacement as it's broken and useless r=dao

Comment 12

[Martin Stránský [:stransky] (ni? me)]

(In reply to Kestrel from comment #10)

The hidden ui prefs are intended to override system settings which can be
useful for customization and troubleshooting. They are not expected to work
flawlessly so I would be more inclined to keep the pref and WONTFIX the
cosmetic issue.

Toggle ui.GtkCSDReversedPlacement without EnsureToolbarMetrics() from gtk3drawing.cpp re-call is generally wrong as the titlebar layout is not updated and just the css styles are shifted so IMHO it's better to remove it.

Comment 13

[Razvan Maries]

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

Comment 14

[Julien Cristau [:jcristau]]

Should we get this on 66?

Comment 15

[Martin Stránský [:stransky] (ni? me)]

(In reply to Julien Cristau [:jcristau] from comment #14)

Should we get this on 66?

No, the ui.GtkCSDReversedPlacement is not exposed anywhere and was included for testing purposes. Thanks.

Comment 16

[Julien Cristau [:jcristau]]

Thanks Martin!