Closed Bug 1550721 Opened 6 years ago Closed 6 years ago

Firefox doesn't handle half-tiled windows in GNOME correctly

Categories

(Core :: Widget: Gtk, enhancement, P5)

Product:
Core
Component:
Widget: Gtk
Version:
66 Branch
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla71
Tracking Flags:
Tracking Status
firefox71 --- fixed

People

(Reporter: marius, Assigned: marius)

References

(Blocks 1 open bug)

Details

Attachments

(5 files)

screenshot of my dual-head setup, with the half-tiled Firefox on the left half of the upper screen
408.77 KB, image/png
Details
screenshot of my dual-head setup, with Firefox maximized on the upper screen
397.77 KB, image/png
Details
screenshot of my dual-head setup, with Firefox maximized on the upper screen (with the correct window title bar)
402.72 KB, image/png
Details
support-tiled-windows-on-gtk.patch
7.16 KB, patch
Details | Diff | Splinter Review
Bug 1550721 - Fix GTK title bar for tiled windows. r=stransky
47 bytes, text/x-phabricator-request
Details | Review

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Ubuntu Chromium/73.0.3683.103 Chrome/73.0.3683.103 Safari/537.36

Steps to reproduce:

Snapped the Firefox window to the left half of my GNOME desktop

Actual results:

The Firefox window title bar shows rounded corners

Expected results:

The Firefox window title bar should be straight, like it is when the Firefox window is fully maximized

This is most noticeable in the upper left corner of the screenshots. When I switch between maximized and half-tiled using GNOME's Super+Left and Super+Up keybindings (which might be a personal customization, I don't remember what the default keybindings are!), what's most noticeable is how the Firefox tab jumps from flush to the left edge of the screen to quite some distance from it.

Component: Untriaged → Widget: Gtk
Product: Firefox → Core

I took the second screenshot too soon! This is what the steady state looks like: tab title flush to the left edge of the screen.

(What happens when I maximize a half-tiled window is the window gets strectched over a fraction of a second by gnome-shell, and then another noticeable fraction of a second later Firefox updates the title bar with a jerky jump. I took the second screenshot too soon after the maximize action, without waiting for Firefox to update its title bar. Sorry! I wish I could replace the second screenshot instead of adding a third one, but I couldn't find a way. Maybe I can remove it, but I don't want to remove the comment attached to it. Maybe I could re-post the comment, but I don't want to send a flood of confusing emails to whoever gets Bugzilla notifications. Sorry!)

That's interesting, I didn't realize we may need that :) Thanks.

Blocks: gtktitlebar
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P5

So I'm poking around the source tree trying to see how hard this would be to fix. I found some code in widget/gtk/nsWindow.cpp that copies the GDK window state into mSizeState. I see it's currently handling things like GDK_WINDOW_STATE_MAXIMIZED and GDK_WINDOW_STATE_FULLSCREEN, but doesn't know about GDK_WINDOW_STATE_TILED nor the newer and more detailed GDK_WINDOW_STATE_TOP_TILED, GDK_WINDOW_STATE_BOTTOM_TILED, GDK_WINDOW_STATE_LEFT_TILED, GDK_WINDOW_STATE_RIGHT_TILED. (These are documented in https://developer.gnome.org/gdk3/stable/gdk3-Event-Structures.html#GdkWindowState.)

I think the internal window state enum (mSizeState == nsSizeMode_Maximized etc) is then used by the titlebar drawing code to determine whether the corners should be rounder or not, but I haven't found that code yet. (I am more or less completely unfamiliar with Firefox's source tree.) It seems to me there are two possible fixes:

  • when any of the tiled bits are set, set mSizeState to nsSizeMode_Maximized so the titlebar is drawn the same way as it is when the window is maximized (but this could cause undesired side effects elsewhere)
  • when any of the tiled bits are set, but GDK_WINDOW_STATE_FULLSCREEN/GDK_WINDOW_STATE_MAXIMIZED are not set, set mSizeState to a new nsSizeMode_Tiled (and then review all the places that check it and make them handle the new enum value correctly)

I will probably give up before I manage to prepare a patch, so if a real developer wants to fix this first, don't hold off on my account ;-)

I think I found how the maximized vs regular titlebar works!

browser/themes/linux/browser.css has this in it:

/* We draw to titlebar when Gkt+ CSD is available */                                                                                        
@media (-moz-gtk-csd-available) {                                                                                                           

...
  :root[tabsintitlebar] > #navigator-toolbox > #titlebar {                                                                                  
    -moz-appearance: -moz-window-titlebar-maximized;                                                                                        
  }                                                                                                                                         
  :root[tabsintitlebar][sizemode="normal"] > #navigator-toolbox > #titlebar {                                                               
    -moz-appearance: -moz-window-titlebar;                                                                                                  
  }                                                                                                                                         
...

This is then parsed (by some Rust magic) into a StyleAppearance::MozWindowTitlebar or a StyleAppearance::MozWindowTitlebarMaximized value, stored in the mAppearance of some nsStyleDisplay instance, then it gets converted into a MOZ_GTK_HEADER_BAR or MOZ_GTK_HEADER_BAR_MAXIMIZED widget type in widget/gtk/nsNativeThemeGTK.cpp, which is then used to pick (or create) the right GtkHeaderBar widget from the cache in widget/gtk/WidgetStyleCache.cpp.

Assuming nsWindow::mSizeState is exposed to CSS as "sizemode" (hey there's also a mSizeMode! one's for the current state, the other's for the desired state? further investigation required here), it looks like either of my two approaches suggested above might work. Of course finding all uses of the enum values gets more fun when you can't just grep for nsSizeMode_* and have to also inspect CSS.

Thanks for the investigation.

The question is how to transfer the GDK_WINDOW_STATE_TILED state to css. We may need a new media query for that (like -moz-gtk-csd-available) as I don't think it's suitable to extend the mSizeState as it's directly mapped to js sizemode attribute.

[1] https://dxr.mozilla.org/mozilla-central/rev/a42caa9f04fc41044437ac56eab6f6086c841d9f/widget/nsIWidgetListener.h#28

We also need to handle that state at nsWindow and don't crop the window corners in this case as well.

Feel free to go ahead with the patch and assign the bug to yourself to claim that you work on it. You can always step down if it's too difficult/complicated.

What about a new attribute, tilemode, as a sibling to sizemode? Possible values would be "normal" and "tiled". Then the CSS could say

/* We draw to titlebar when Gkt+ CSD is available */                                                                                        
@media (-moz-gtk-csd-available) {                                                                                                           

...
  :root[tabsintitlebar] > #navigator-toolbox > #titlebar {                                                                                  
    -moz-appearance: -moz-window-titlebar-maximized;                                                                                        
  }                                                                                                                                         
  :root[tabsintitlebar][sizemode="normal"][tilemode="normal"] > #navigator-toolbox > #titlebar {                                                               
    -moz-appearance: -moz-window-titlebar;                                                                                                  
  }                                                                                                                                         
...

Implementation-wise, I'm completely lost. I've added an enum nsTileMode to widget/nsIWidgetListener.h. I don't know if I need to copy that value from somewhere to somewhere else in browser/components/BrowserGlue.jsm? I don't know if I need to add tilemodechange events? I've no idea what's going on in xpfe/appshell/nsContentTreeOwner.cpp, with the persistString operations that do something about sizemode? Does xpfe/appshell/nsXULWindow.cpp need a tilemode attribute? What is XULPersist.cpp and what's it doing with sizemode? There are 30 files that mention 'sizemode' (not counting any with test in the pathname).

Help?

I've tested my proposed CSS change without actually hooking up the tilemode property and discovered that

  • no error is reported
  • setting -moz-appearance fixes part of my problem: the rounded corners become square
  • the other part of the problem is extra padding to the left of the first tab, and it remains

I think the extra padding gets removed by this bit in browser/base/content/browser.css:

%ifdef MOZ_WIDGET_GTK
@media (-moz-gtk-csd-reversed-placement: 0) {
  :root:not([sizemode=normal]) .titlebar-spacer[type="pre-tabs"] {
    display: none;
  }
}
@media (-moz-gtk-csd-reversed-placement) {
  :root:not([sizemode=normal]) .titlebar-spacer[type="post-tabs"] {
    display: none;
  }
}
%endif

but I tried to change those to :root:not([sizemode=normal][tilemode=normal]) and saw no difference.

Ctrl+Alt+Shift+I (https://developer.mozilla.org/en-US/docs/Tools/Browser_Toolbox) confirmed that the extra space to the left of the browser tab is the .titlebar-spacer.

I think I merely got the condition wrong: the inverse of [sizemode=normal][tilemode=normal] is not :not([sizemode=normal][tilemode=normal]), but rather :not([sizemode=normal]), :not([tilemode=normal]).

(Note to self: I need to run ./mach build after I edit the browser.css before ./mach run will noticed I made changes.)

I have a working patch (attached)!

This is my second-ever patch for Firefox, and the previous one was many years ago and only touched JavaScript code (and wasn't even working right, somebody else drove it to completion). I apologize if I'm not following the process correctly.

Pushed by cbrindusan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/44d3452f6a07
Fix GTK title bar for tiled windows. r=stransky,dao

Keywords: checkin-needed

GDK_WINDOW_STATE_TILED was added in GTK version 3.10, which was released in 2013. What is the lowest version of GTK that Firefox wants to support?

Adding

#ifndef GDK_WINDOW_STATE_TILED
#define GDK_WINDOW_STATE_TILED 0
#endif

ought to be enough to make it build (but of course that build wouldn't be able to detect tiled window states).

Where is a good place to put such compatibility defines? Near the top of nsWindow.cpp?

Flags: needinfo?(marius) → needinfo?(stransky)

(In reply to Marius Gedminas from comment #15)

#ifndef GDK_WINDOW_STATE_TILED
#define GDK_WINDOW_STATE_TILED 0
#endif

See https://searchfox.org/mozilla-central/rev/7ed8e2d3d1d7a1464ba42763a33fd2e60efcaedc/widget/gtk/gtk3drawing.cpp#40

so something like:

#ifndef GDK_WINDOW_STATE_TILED
#define GDK_WINDOW_STATE_TILED (1 << 8)
#endif

Please place it on top of nsWindow.cpp.

Flags: needinfo?(stransky)
Type: defect → enhancement
Assignee: nobody → marius

Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3258db2cb365
Fix GTK title bar for tiled windows. r=stransky,dao

Keywords: checkin-needed

https://hg.mozilla.org/mozilla-central/rev/3258db2cb365

Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox71: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla71

For some reason this patch doesn't work when I backport it to Ubuntu's Firefox 69 (which I do because I don't want to wait until December to get this fix on my machine).

Specifically, what I'm doing is (on Ubuntu 19.04) :

pull-lp-source firefox disco
cd firefox-69.0.1+build1
patch -p1 -i ~/src/firefox/support-tiled-windows-on-gtk-final.patch  # produced with hg export 5b5b21b7abc9
find -name '*.orig' -delete
dpkg-source --commit
dch -iU
debuild -S -k$GPGKEY
cd ..
dput ppa:mgedmin/ppa firefox_69.0.1+build1-0ubuntu0.19.04.1mg1_source.changes

The build takes about 4 hours on the PPA builder (and local builds fail with an obscure error, so I have to rely on the PPA), after which I can install the package, and when I do I observe the following:

  • normal Firefox windows have the normal title bar (round corners, extra padding in front of the first tab)
  • maximized Firefox windows have the maximized title bar (square corners, first tab flush with screen edge)
  • tiled Firefox windows have a mixture (round corners, first tab flush with screen edge)

Using the browser inspector (ctrl+alt+shift+i) I can see that the #titlebar element gets -moz-appearance: it should get:

  • -moz-window-titlebar for a regular window
  • -moz-window-titlebar-maximized for a maximized window
  • -moz-window-titlebar-maximized for a tiled window

(Ironically the browser inspector shows a /!\ icon and claims neither of the -moz-window-titlebar values are valid choices for this property, but I assume it's just using the validator for regular browser CSS and doesn't know about the extensions used for browser chrome.)

How come -moz-appearance does not take effect correctly?

Further experiments with unchecking the checkbox next to -moz-appearance in the inspector show that this property is important: when I uncheck the title bar becomes a solid bar of color (black for regular/tiled windows, white for maximized windows) instead of looking like a proper GTK headerbar.

I'm stumped and would welcome any ideas. (Also, note that local Firefox 71 builds that I used to produce and test the patch that was committed, work fine on my machine. It's just the backport that is giving me trouble.)

Firefox 71 finally percolated into Ubuntu, and I see the same problem with the official builds: tiled windows get rounded headerbar corners instead of square ones.

As a workaround, if I set MOZ_ENABLE_WAYLAND=1 before launching firefox to force it to run in native Wayland mode, the problem doesn't appear and headerbar corners correctly switch between round and square as I press Super+Left/Right to tile/untile.

Note that when I was testing earlier with ./mach run, I didn't see the problem with Firefox running via Xwayland.

(In reply to Marius Gedminas from comment #21)

Firefox 71 finally percolated into Ubuntu, and I see the same problem with the official builds: tiled windows get rounded headerbar corners instead of square ones.

Whoops, the version of Firefox in Ubuntu is actually 70.0.1. The build I'm running now doesn't have my patch applied, and yet tiling works fine under Wayland!

Whoops, the version of Firefox in Ubuntu is actually 70.0.1. The build I'm running now doesn't have my patch applied, and yet tiling works fine under Wayland!

I would like to request a brown paper bag to put over my head, because I accidentally ran the apt-cache policy command to verify the installed version on a different machine over SSH.

The behaviour about things working on Wayland but not on Xwayland applies to Ubuntu's 70.0.1 with my patch backported.

Firefox 71 finally percolated into Ubuntu, and I see the same problem with the official builds: tiled windows get rounded headerbar corners instead of square ones.

And now this is finally actually true (sadly).

For bonus points the MOZ_ENABLE_WAYLAND=1 workaround is not viable as it makes Firefox crash (for which I'll open a separate bug report).

(In reply to Marius Gedminas from comment #24)

Firefox 71 finally percolated into Ubuntu, and I see the same problem with the official builds: tiled windows get rounded headerbar corners instead of square ones.

And now this is finally actually true (sadly).

That can be easily fixed but it needs some work. There's some info:

this may be updated to:

if (mDrawInTitlebar && mSizeState == nsSizeMode_Normal && !mIsTiled)

and we also need to set/unset shape masks when mIsTiled is changed at nsWindow::OnWindowStateEvent().

Regressions: 1604921
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: