Closed Bug 1418829 Opened 6 years ago Closed 6 years ago

browser.tabs.drawInTitlebar;true causes tabbar to have empty space to left and right

Categories

(Core :: Widget: Gtk, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox-esr52 --- unaffected
firefox57 --- unaffected
firefox58 --- unaffected
firefox59 --- fixed

People

(Reporter: yoasif, Assigned: stransky)

References

(Blocks 1 open bug)

Details

(Keywords: nightly-community, regression)

Attachments

(8 files)

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:59.0) Gecko/20100101 Firefox/59.0
Build ID: 20171119100329

Steps to reproduce:

1. Start Firefox on Linux
2. Install https://addons.mozilla.org/en-us/firefox/addon/tab-counter-webext/ - which places an icon to the right of the tab bar by default. 


Actual results:

Empty space to the right of the icon. 


Expected results:

As when browser.tabs.drawInTitlebar is set to false.
 8:19.47 INFO: Narrowed inbound regression window from [a9cf5391, 63224d6e] (4 builds) to [9f8b177e, 63224d6e] (2 builds) (~1 steps left)
 8:19.47 INFO: No more inbound revisions, bisection finished.
 8:19.47 INFO: Last good revision: 9f8b177edf71215098643f5a705fe8730734c346
 8:19.47 INFO: First bad revision: 63224d6e543cc3399f419dd4ac500873db02b021
 8:19.47 INFO: Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=9f8b177edf71215098643f5a705fe8730734c346&tochange=63224d6e543cc3399f419dd4ac500873db02b021
Blocks: 1415481
Has Regression Range: --- → yes
Has STR: --- → yes
Component: Untriaged → Widget: Gtk
Product: Firefox → Core
Also reported on  Comment 30 on bug 1415481.
I think we should remove "widget.allow-client-side-decoration" and use "browser.tabs.drawInTitlebar" only to disable/enable titlebar drawing.
Assignee: nobody → stransky
(In reply to Johann Hofmann [:johannh] from comment #31)
> Screenshots showing the added drag space:
> 
> https://screenshots.mattn.ca/compare/?oldProject=mozilla-
> central&oldRev=e070277ec199fa96fa490ed52d33646a376d0d80&newProject=mozilla-
> central&newRev=60b9fa15e4272bb1d2b876235f346587be6f2424&filter=linux
> 
> I guess that's intended to be shown by default.
Attachment #8930074 - Flags: review?(jhorak) → review?(dao+bmo)
Comment on attachment 8930074 [details]
Bug 1418829 - set browser.tabs.drawInTitlebar to false on UNIX_BUT_NOT_MAC again,

https://reviewboard.mozilla.org/r/201254/#review206848
Attachment #8930074 - Flags: review?(dao+bmo) → review+
Comment on attachment 8930075 [details]
Bug 1418829 - remove widget.allow-client-side-decoration preference,

https://reviewboard.mozilla.org/r/201256/#review206920
Attachment #8930075 - Flags: review?(jhorak) → review+
Comment on attachment 8930077 [details]
Bug 1418829 - nsLookAndFeel enables rendering to the titlebar for supported window managers only,

https://reviewboard.mozilla.org/r/201260/#review206356

::: widget/gtk/nsLookAndFeel.cpp:1081
(Diff revision 1)
>  
>      gtk_widget_destroy(window);
>      g_object_unref(labelWidget);
>  
> -    // Require GTK 3.10 for GtkHeaderBar support.
> -    mCSDAvailable = gtk_check_version(3, 10, 0) == nullptr;
> +    // Require GTK 3.10 for GtkHeaderBar support and compatible window manager.
> +    mCSDAvailable = (gtk_check_version(3, 10, 0) == nullptr &&

Remove the call of GetCSDSupportLevel() from 
https://searchfox.org/mozilla-central/source/widget/gtk/nsWindow.cpp#3758
and let this implementation of nsLookAndFeel::EnsureInit to decide whenever the CSD is available on not by checking GetCSDSupportLevel.

Consider the need of caching mIsCSDAvailable in nsWindow, is it worth? Calling: LookAndFeel::GetInt(LookAndFeel::eIntID_GTKCSDAvailable, &isCSDAvailable) looks cheap to me, or is the nsWindow::SetDrawsInTitlebar called so frequently?
Attachment #8930077 - Flags: review?(jhorak) → review-
Comment on attachment 8930076 [details]
Bug 1418829 - Export GetCSDSupportLevel() as public from nsWindow,

https://reviewboard.mozilla.org/r/201258/#review206340
Attachment #8930076 - Flags: review?(jhorak) → review+
Comment on attachment 8930078 [details]
Bug 1418829 - Add MATE entry to GetCSDSupportLevel(), use GDK_DECOR_BORDER with CSD_SUPPORT_FULL only,

https://reviewboard.mozilla.org/r/201262/#review206930
Attachment #8930078 - Flags: review?(jhorak) → review+
Comment on attachment 8930077 [details]
Bug 1418829 - nsLookAndFeel enables rendering to the titlebar for supported window managers only,

https://reviewboard.mozilla.org/r/201260/#review206932

::: widget/gtk/nsLookAndFeel.cpp:1081
(Diff revision 1)
>  
>      gtk_widget_destroy(window);
>      g_object_unref(labelWidget);
>  
> -    // Require GTK 3.10 for GtkHeaderBar support.
> -    mCSDAvailable = gtk_check_version(3, 10, 0) == nullptr;
> +    // Require GTK 3.10 for GtkHeaderBar support and compatible window manager.
> +    mCSDAvailable = (gtk_check_version(3, 10, 0) == nullptr &&

I can't remove it from nsWindow class, it's going to be used for subsequent patches to get FULL/FLAT transparency type, set RGBA visual accordingly and fiddle with resizers.

So the code CSD at nsWindow::Create() is going to be extended for cases at Bug 1417933.
Comment on attachment 8930077 [details]
Bug 1418829 - nsLookAndFeel enables rendering to the titlebar for supported window managers only,

https://reviewboard.mozilla.org/r/201260/#review206932

> I can't remove it from nsWindow class, it's going to be used for subsequent patches to get FULL/FLAT transparency type, set RGBA visual accordingly and fiddle with resizers.
> 
> So the code CSD at nsWindow::Create() is going to be extended for cases at Bug 1417933.

Okay, I'll do that but I'd rather leave that for another patch as nsWindow::Create needs to be redesigned for FLAT/FULL modes and also round courners rendering.
Comment on attachment 8930077 [details]
Bug 1418829 - nsLookAndFeel enables rendering to the titlebar for supported window managers only,

https://reviewboard.mozilla.org/r/201260/#review206938

::: widget/gtk/nsLookAndFeel.cpp:1081
(Diff revision 1)
>  
>      gtk_widget_destroy(window);
>      g_object_unref(labelWidget);
>  
> -    // Require GTK 3.10 for GtkHeaderBar support.
> -    mCSDAvailable = gtk_check_version(3, 10, 0) == nullptr;
> +    // Require GTK 3.10 for GtkHeaderBar support and compatible window manager.
> +    mCSDAvailable = (gtk_check_version(3, 10, 0) == nullptr &&

Let's track that at Bug 1419448.
Comment on attachment 8930077 [details]
Bug 1418829 - nsLookAndFeel enables rendering to the titlebar for supported window managers only,

https://reviewboard.mozilla.org/r/201260/#review206940

Okay, we'll address the nsWindow issues in bug 1419448. Please fix the typo in the commit message, r+ with that fixed.
Attachment #8930077 - Flags: review- → review+
Pushed by stransky@redhat.com:
https://hg.mozilla.org/integration/autoland/rev/686eee770f7f
set browser.tabs.drawInTitlebar to false on UNIX_BUT_NOT_MAC again, r=dao
https://hg.mozilla.org/integration/autoland/rev/b757a8721ea2
remove widget.allow-client-side-decoration preference, r=jhorak
https://hg.mozilla.org/integration/autoland/rev/b055cf0f0b05
Export GetCSDSupportLevel() as public from nsWindow, r=jhorak
https://hg.mozilla.org/integration/autoland/rev/ff5a8099c8e3
nsLookAndFeel enables rendering to the titlebar for supported window managers only, r=jhorak
https://hg.mozilla.org/integration/autoland/rev/6918d1218831
Add MATE entry to GetCSDSupportLevel(), use GDK_DECOR_BORDER with CSD_SUPPORT_FULL only, r=jhorak
Martin, first, thanks for your great work to implement this also on Linux. On https://hg.mozilla.org/mozilla-central/rev/ff5a8099c8e3#l1.38 you are checking for "browser.tabs.drawInTitlebar". Thunderbird can now also draw in title bar. But we are using "mail.tabs.drawInTitlebar" to enable this feature. With your check, TB can't draw in title bar. Would it be possible to somehow change this check to work with TB too? Or do we need to change our pref to browser... which isn't very logical for a mail program?
Flags: needinfo?(stransky)
(In reply to Richard Marti (:Paenglab) from comment #29)
> Martin, first, thanks for your great work to implement this also on Linux.
> On https://hg.mozilla.org/mozilla-central/rev/ff5a8099c8e3#l1.38 you are
> checking for "browser.tabs.drawInTitlebar". Thunderbird can now also draw in
> title bar. But we are using "mail.tabs.drawInTitlebar" to enable this
> feature. With your check, TB can't draw in title bar. Would it be possible
> to somehow change this check to work with TB too? Or do we need to change
> our pref to browser... which isn't very logical for a mail program?

Good point, I'll investigate that. You may need to implement variant of Bug 1414222 which enables/disables titlebar according to system configuration and also provides info about active titlebar buttons. Then we can work out the "browser.tabs.drawInTitlebar" issue for TB.
Flags: needinfo?(stransky)
(In reply to Martin Stránský [:stransky] from comment #30)
> Good point, I'll investigate that. You may need to implement variant of Bug
> 1414222 which enables/disables titlebar according to system configuration
> and also provides info about active titlebar buttons. Then we can work out
> the "browser.tabs.drawInTitlebar" issue for TB.

Filed as Bug 1420110
(In reply to Martin Stránský [:stransky] from comment #31)
> (In reply to Martin Stránský [:stransky] from comment #30)
> > Good point, I'll investigate that. You may need to implement variant of Bug
> > 1414222 which enables/disables titlebar according to system configuration
> > and also provides info about active titlebar buttons. Then we can work out
> > the "browser.tabs.drawInTitlebar" issue for TB.

This is all already implemented in TB. :)

> Filed as Bug 1420110

Thanks.
This is marked as fixed, but the problem still persists on my end:

Screenshots - https://imgur.com/a/FnYUt

Build-ID 	20171216100407
OS 	        Antergos (Gnome 3.26.2; GTK3 3.22.26)
Like said in comment 34 this isn't fixed for us as well!

When the browser is maximized everything is ok, BUT if the browser is resized then suddenly there is an empty space left to the tabs. When maximizing the browser again the space disappears again!

Added a screenshot which shows that bug.

I am using GNOME-Flashback under Ubuntu 17.10.

This is my environment:

~$ env | grep "XDG_"
XDG_MENU_PREFIX=gnome-flashback-
XDG_GREETER_DATA_DIR=/var/lib/lightdm-data/mkurz
XDG_SESSION_TYPE=x11
XDG_DATA_DIRS=/usr/share/gnome-flashback-compiz:/usr/share/gnome-flashback-compiz:/usr/local/share:/usr/share:/var/lib/snapd/desktop:/var/lib/snapd/desktop
XDG_SESSION_DESKTOP=gnome-flashback-compiz
GNOME_SESSION_XDG_SESSION_PATH=/org/freedesktop/DisplayManager/Session0
XDG_SEAT_PATH=/org/freedesktop/DisplayManager/Seat0
XDG_CURRENT_DESKTOP=GNOME-Flashback:GNOME
XDG_RUNTIME_DIR=/run/user/1000
XDG_SESSION_PATH=/org/freedesktop/DisplayManager/Session0
XDG_CONFIG_DIRS=/etc/xdg/xdg-gnome-flashback-compiz:/etc/xdg/xdg-gnome-flashback-compiz:/etc/xdg
I've seen this on Linux/GNOME/Wayland. I can't reproduce it right now, but at some point it was triggered by switching a YouTube player to full-screen and back. Perhaps the 3ship and m.kurz can test this.
@Laurentiu Nicola I already gave a description how to reproduce that. Just resize the window, that's it, nothing more.
In maximized state everything is OK, in un-maximized state the space on the left appears. Just testet with todays nightly.
@m.kurz Confirmed.
(In reply to m.kurz from comment #35)
> Empty space on the left next to tabs when browser is resized

That's an intended part of the Photon design spec with titlebar disabled and unrelated to this bug.

https://mozilla.invisionapp.com/share/ZKBC94BPQ#/screens/237801528
(In reply to Kestrel from comment #39)
> That's an intended part of the Photon design spec with titlebar disabled and
> unrelated to this bug.
> 
> https://mozilla.invisionapp.com/share/ZKBC94BPQ#/screens/237801528

Hmm.. Alright. I find it a bit weird because the UI seems "jumping" around when switching between maximized and un-maximized.
(In reply to Kestrel from comment #39)
> (In reply to m.kurz from comment #35)
> > Empty space on the left next to tabs when browser is resized
> 
> That's an intended part of the Photon design spec with titlebar disabled and
> unrelated to this bug.
> 
> https://mozilla.invisionapp.com/share/ZKBC94BPQ#/screens/237801528

Is the empty space on the right of the tab bar supposed to be there, too?

As you can see from my screenshot in comment #34, when I drag an icon to the right of the tab bar, it leaves some space between that icon and the window control buttons, like described in comment #0. This empty space - unlike the one left of the tab bar - doesn't disappear, when I maximize the window.
(In reply to 3ship from comment #41)
> Is the empty space on the right of the tab bar supposed to be there, too?

Yes, you can see these referred to as "Persistent Drag Spaces" in the design spec.

https://mozilla.invisionapp.com/share/ZKBC94BPQ#/screens/229786513
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: