882009 - Fix browser.tabs.drawInTitlebar pref support on Windows

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[Matthew N. [:MattN]]

Prior to Australis, the pref browser.tabs.drawInTitlebar controls whether there is a gap between the tabs and the top of the screen for the titlebar/Firefox button when the browser is maximized and the menubar is hidden. This has no effect when the menubar is shown because the menubar was always shown below the titlebar.

With Australis, the menubar always appears above the titlebar but we still move the tabs down when drawInTitlebar = false. We currently don't show the Window title because we have always have @chromemargin set on the Window.

We should either:
A) Drop support for the pref. 
B) Fix Australis to show the title in the titlebar when it's false.

Neither of these are huge wins/costs but we're currently in a middle ground with brokenness for users who have the pref set to false. Is there a compelling reason to continue to support the pref set to false? An add-on would be able to get the system titlebar back even if we stop supporting the pref.

Note that this won't necessarily allow us to remove much, if any, of the TabsInTitlebar code in browser.js since we still have special cases such as Print Preview, TabView, and popup windows without a tabstrip where @TabsInTitlebar is not true. We would also need some CSS to keep checking the attribute for those special cases. Australis currently works fine in those cases AFAIK. This bug should focus on whether we should support users setting drawInTitlebar to false and how it should look when they do so.

Comment 1

[Firefox Portable user]

**** users, right?

Comment 2

[Mike Conley (:mconley) (:⚙️)]

(In reply to Firefox Portable user from comment #1)
> **** users, right?

Hello Firefox Portable user,

Please (re)familiarize yourself with the Bugzilla Rules of Etiquette[1] before posting. Thanks!

-Mike

[1]: https://bugzilla.mozilla.org/page.cgi?id=etiquette.html

Comment 3

[Firefox Portable user]

Hello Mike Conley (:mconley),

It wasn't me who suggested to **** users by removing function/feature of switching off drawing in the title bar. I just rephrased (pretty accurate, by the way) the initial post in 3 words.

So I don't think Etiquette would somehow help here, since the bug starter suggests[1] to actually **** users.
You are welcome!

-FPU

[1]: https://bugzilla.mozilla.org/show_bug.cgi?id=882009

Comment 4

[Matthew N. [:MattN]]

Note that the bug summary and comment 0 are about deciding one way or the the other. No decision has been made yet. If there are good reasons to keep supporting this pref from about:config (which most users would never find) which have not already been mentioned, now would be the time.

Comment 6

[al_9x]

Attachment: normal vs. hacked titlebar

on xp at least, tabs in title bar is a hack that does not look "right" even with the default style, and completely falls apart with custom styles

so the non hacked default normal titlebar fallback should remain available

Comment 7

[:Gijs (he/him)]

bug 893065 was just WONTFIXED (see bug 893065 comment 14).

Does that make this WONTFIX and/or "Fixed, the answer is 'no, we won't support it'" too? Or do we think the cost/benefits are different for Windows vs. OS X?

Comment 8

[dagger.bugzilla]

Personally I'd prefer to see it stay, since, on Windows Classic at least, the custom titlebar arrangement looks pretty horribly wrong. The pref completely fixes it (or almost does; it needs fixing to display the OS titlebar rather than the XUL titlebar, which just involves hooking the pref up to the existing code to do that, and which I can write the patch for).

Comment 9

[Justin Dolske [:Dolske]]

(In reply to :Gijs Kruitbosch from comment #7)
> bug 893065 was just WONTFIXED (see bug 893065 comment 14).

Right.

(In reply to Matthew N. [:MattN] from comment #0)

> We should either:
> A) Drop support for the pref. 
> B) Fix Australis to show the title in the titlebar when it's false.

We should go with (A) and drop it.

Comment 10

[Justin Dolske [:Dolske]]

Gah. I didn't actually mean to dupe this, since the work to-do here is to actually remove the code that checks this pref.

Comment 11

[al_9x]

displaying the native titlebar is trivial and does not require much support

[e-mail] already offered to fix the pref

does not seem like this can be addressed in an add-on

tabs in title bar do not look right in various non-default theme scenarios

for the above reasons the pref should remain, it's senseless to create artificial barriers to running Fx

Comment 12

[Dão Gottwald [:dao]]

Note that we could literally just remove the code that reads the pref, as tabs in the title bar still needs the ability to turn itself off e.g. for popup windows or print preview.

Comment 13

[:Gijs (he/him)]

(In reply to al_9x from comment #11)
> displaying the native titlebar is trivial and does not require much support

It does. It'll double the size of our testing matrix which is already big (OS versions + themes * default font size changes * LWT light/dark/off * window sizes * number of tabs open (overflowing or not, as well as color contrasts of buttons on gradient backgrounds in some of the themes)). Yes, it's easy to add another read of the pref. Supporting and testing it properly in all the other related configurations: not quite as easy.

> does not seem like this can be addressed in an add-on

I don't really see why it couldn't.

Comment 14

[al_9x]

(In reply to :Gijs Kruitbosch from comment #13)
> (In reply to al_9x from comment #11)
> > displaying the native titlebar is trivial and does not require much support
> 
> It does. It'll double the size of our testing matrix which is already big
> (OS versions + themes * default font size changes * LWT light/dark/off *
> window sizes * number of tabs open (overflowing or not, as well as color
> contrasts of buttons on gradient backgrounds in some of the themes)). Yes,
> it's easy to add another read of the pref. Supporting and testing it
> properly in all the other related configurations: not quite as easy.

For whose sake is it tested?  For the sake of those who would use it obviously.  Those who wouldn't, don't care if it's broken.  It should be self evident, that those who want it, will accept it without "full coverage" or whatever it is you claim is too expensive, rather than loose it altogether.  They will test it for you.  I reported that it was broken and would do so again.

> 
> > does not seem like this can be addressed in an add-on
> 
> I don't really see why it couldn't.

[e-mail], do you know if it's possible?

Comment 15

[dagger.bugzilla]

It should be possible, yes, but I think that native window decorations are something Firefox should be able to do itself (especially for Windows Classic, where drawing in the titlebar looks really bad), rather than rely on an extension for.

It's notable that, other than the @chromemargin issue this bug was filed over, the pref seems to work fine already, even though we haven't been paying any attention to it. I think we'd be ok fixing this specific issue, and then relying on user reports for any future issues, without needing to test any and every theme and UI change with it.

I'm also willing to go through and double-check my "seems to work" statement on the platforms I have access to (Windows XP and 7).

Comment 16

[al_9x]

Sounds good, but do you need to convince someone to let you fix this?

Comment 17

[:Gijs (he/him)]

Attachment: remove browser.tabs.drawInTitlebar pref support

AFAIK this should work.

Comment 18

[:Gijs (he/him)]

(In reply to al_9x from comment #6)
> Created attachment 768389 [details]
> normal vs. hacked titlebar
> 
> on xp at least, tabs in title bar is a hack that does not look "right" even
> with the default style, and completely falls apart with custom styles

At least the former part of what you were seeing could be a bug, see bug 885062. This should be fixed on the latest versions of UX. This should also help improve the custom theme situation, although I can't give you any guarantees there.

(In reply to al_9x from comment #14)
> (In reply to :Gijs Kruitbosch from comment #13)
> > (In reply to al_9x from comment #11)
> > > displaying the native titlebar is trivial and does not require much support
> > 
> > It does. It'll double the size of our testing matrix which is already big
> > (OS versions + themes * default font size changes * LWT light/dark/off *
> > window sizes * number of tabs open (overflowing or not, as well as color
> > contrasts of buttons on gradient backgrounds in some of the themes)). Yes,
> > it's easy to add another read of the pref. Supporting and testing it
> > properly in all the other related configurations: not quite as easy.
> 
> For whose sake is it tested?  For the sake of those who would use it
> obviously.  Those who wouldn't, don't care if it's broken.  It should be
> self evident, that those who want it, will accept it without "full coverage"
> or whatever it is you claim is too expensive, rather than loose it
> altogether.  They will test it for you.  I reported that it was broken and
> would do so again.

And then developers/QA have to reproduce it, fix it, test it, etc., all the while increasing code complexity for edge cases that few people care about. There is no such thing as a free lunch.

I also don't believe your claim that people will be happy without "full coverage", especially not if the brokenness is worse than just using tabs in titlebar. Finally, people living with bugs doesn't mean that somehow it's a good idea to have half-working code hanging around, especially if we think that the resulting experience (even in case of perfectly functional code) is inferior to the default one.

Comment 19

[al_9x]

(In reply to :Gijs Kruitbosch from comment #18)
> (In reply to al_9x from comment #6)
> > Created attachment 768389 [details]
> > normal vs. hacked titlebar
> > 
> > on xp at least, tabs in title bar is a hack that does not look "right" even
> > with the default style, and completely falls apart with custom styles
> 
> At least the former part of what you were seeing could be a bug, see bug
> 885062. This should be fixed on the latest versions of UX. This should also
> help improve the custom theme situation, although I can't give you any
> guarantees there.
> 

It's more of a mess, if anything. 

> 
> I also don't believe your claim that people will be happy without "full
> coverage"

If you're going to take away a feature that someone offered to fix and keep fixed, have the decency not to tell me it's for my own good.

Comment 20

[al_9x]

Attachment: 26.0a1 (2013-08-26)

Comment 21

[Mike Conley (:mconley) (:⚙️)]

al_9x:

I'm interested in how your screenshot came to be.

Do you have any add-ons installed?

-Mike

Comment 22

[Mike Conley (:mconley) (:⚙️)]

Also, al_9x, you may be interested in the add-on that a member of our community is working on:

http://aris-at-mozilla.blogspot.ca/

In the meantime, let's see if we can debug what's gone wrong with your tabs - because they look like hell.

-Mike

Comment 23

[al_9x]

Mike, there are countless 3rd party themes for xp, and the Fx title bar hack is likely broken in a good % of them.  But the default, native xp title bar works everywhere, so restoring that functionality in some way makes sense, whereas investigating the quirks with any given theme probably does not.
http://uxstyle.com/
http://wayback.archive.org/web/lunaelement.net/files/official1/LE4.exe
Luna Element 4 (Blue Compact color scheme)

Are there plans for that add-on to support the native titlebar?

Comment 24

[Mike Conley (:mconley) (:⚙️)]

(In reply to al_9x from comment #23)
> Mike, there are countless 3rd party themes for xp, and the Fx title bar hack
> is likely broken in a good % of them.  But the default, native xp title bar
> works everywhere, so restoring that functionality in some way makes sense,
> whereas investigating the quirks with any given theme probably does not.
> http://uxstyle.com/
> http://wayback.archive.org/web/lunaelement.net/files/official1/LE4.exe
> Luna Element 4 (Blue Compact color scheme)
> 
> Are there plans for that add-on to support the native titlebar?

Aris, the author of that add-on, seems responsive to feedback and suggestions. The removal of the pref is only a single entry point for putting the tabs in there - and Aris could certainly instruct the add-on to disallow tabs in titlebar. (Aris would simply need to call TabsInTitlebar.allowedBy("some-unique-key", false) in a browser context).

I can't speak for Aris, but that sounds pretty simple to implement. And if Aris doesn't feel like doing it, I hope (s)he takes pull requests so that someone like you can implement it.

Besides your XP theme, is your Firefox using other add-ons that might be disrupting tab drawing?

Comment 25

[al_9x]

(In reply to Mike Conley (:mconley) from comment #24)
> 
> Aris, the author of that add-on, seems responsive to feedback and
> suggestions. The removal of the pref is only a single entry point for
> putting the tabs in there - and Aris could certainly instruct the add-on to
> disallow tabs in titlebar. (Aris would simply need to call
> TabsInTitlebar.allowedBy("some-unique-key", false) in a browser context).
> 
> I can't speak for Aris, but that sounds pretty simple to implement. And if
> Aris doesn't feel like doing it, I hope (s)he takes pull requests so that
> someone like you can implement it.
> 

There were a few fixes needed to make the native title bar work properly, which [e-mail] indicated he might provide.  Perhaps he can submit them to the add-on.

> Besides your XP theme, is your Firefox using other add-ons that might be
> disrupting tab drawing?

no add-ons, new profile

Comment 26

[dagger.bugzilla]

> especially not if the brokenness is worse than just using tabs in titlebar

That may well be the case on Aero, where drawing in the titlebar is more expected and looks okay. On Windows Classic, however, it looks like this:

http://i.imgur.com/LbjRjDe.png

which looks terrible. Titlebars are not supposed to look like that. In the state we plan to ship in, we come across as "broken" rather than "good-looking".

I'm not familiar with the theme al_9x is using, but I assume it's in a similar situation.

Comment 27

[Aris]

I'm not sure why we shouldn't keep both states alive for 'browser.tabs.drawInTitlebar'. My add-on will handle both and an on/off checkbox could be added to the options panel for those, who dislike using about:config.

http://i.imgur.com/6vyk8wP.png
http://i.imgur.com/Et2a9cF.png

@[e-mail]: Does your theme look fine with 'browser.tabs.drawInTitlebar' set to false or is there a general problem with this theme and Firefox Australis UI?
Firefox can only check for Windows default themes and not for custom third party ones. There probably won't be a way to support every existing theme. See "-moz-windows-theme" here https://developer.mozilla.org/en-US/docs/Web/Guide/CSS/Media_queries

Comment 28

[al_9x]

(In reply to Aris from comment #27)
> I'm not sure why we shouldn't keep both states alive for
> 'browser.tabs.drawInTitlebar'. My add-on will handle both and an on/off
> checkbox could be added to the options panel for those, who dislike using
> about:config.
> 
> http://i.imgur.com/6vyk8wP.png
> http://i.imgur.com/Et2a9cF.png

Your add-on presumably uses the existing pref currently?  But mozilla is removing it, so hopefully you can preserve this functionality without the native pref.

> 
> @[e-mail]: Does your theme look fine with
> 'browser.tabs.drawInTitlebar' set to false or is there a general problem
> with this theme and Firefox Australis UI?

drawInTitlebar=false works perfectly in the latest non australis regular nightly, but is slightly broken in Australis (Bug 880545), due to the following, according to dagger.bugzilla:

(dagger.bugzilla from comment #8)
> it needs fixing to display the OS
> titlebar rather than the XUL titlebar, which just involves hooking the pref
> up to the existing code to do that, and which I can write the patch for 

Would you be able to do that in the add-on?

> Firefox can only check for Windows default themes and not for custom third
> party ones. There probably won't be a way to support every existing theme.
> See "-moz-windows-theme" here
> https://developer.mozilla.org/en-US/docs/Web/Guide/CSS/Media_queries

It should not be necessary to support specific themes, if drawInTitlebar=false functionality can be restored to the pre australis level (i.e. a true native, default title bar), it will work with all themes

Comment 29

[Mike Conley (:mconley) (:⚙️)]

(In reply to al_9x from comment #28)
> (In reply to Aris from comment #27)
> > I'm not sure why we shouldn't keep both states alive for
> > 'browser.tabs.drawInTitlebar'. My add-on will handle both and an on/off
> > checkbox could be added to the options panel for those, who dislike using
> > about:config.
> > 
> > http://i.imgur.com/6vyk8wP.png
> > http://i.imgur.com/Et2a9cF.png
> 
> Your add-on presumably uses the existing pref currently?  But mozilla is
> removing it, so hopefully you can preserve this functionality without the
> native pref.

Yes, this should be possible. The browser.tabs.drawInTitlebar pref (not to be confused with the drawintitlebar attribute - yet another reason to drop the pref!) is simply observed by the TabsInTitlebar object defined in browser/base/content/browser.js:

https://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#4577

That pref is observed - and when it's changed, the TabsInTitlebar.allowedBy method is called with the value.  TabsInTitlebar.allowedBy isn't going anywhere, and this is what I suggest Aris uses.

> 
> > 
> > @[e-mail]: Does your theme look fine with
> > 'browser.tabs.drawInTitlebar' set to false or is there a general problem
> > with this theme and Firefox Australis UI?
> 
> drawInTitlebar=false works perfectly in the latest non australis regular
> nightly, but is slightly broken in Australis (Bug 880545), due to the
> following, according to dagger.bugzilla:
> 
> (dagger.bugzilla from comment #8)
> > it needs fixing to display the OS
> > titlebar rather than the XUL titlebar, which just involves hooking the pref
> > up to the existing code to do that, and which I can write the patch for 
> 
> Would you be able to do that in the add-on?

That, I believe, involves 2 things:

1) Using TabsInTitlebar.allowedBy("some-unique-key", false), as above, and
2) Overriding these two -moz-appearance rules: https://mxr.mozilla.org/projects-central/source/ux/browser/themes/windows/browser.css#144

I *believe* that'll put the old titlebar back.

Comment 30

[Mike Conley (:mconley) (:⚙️)]

OH - and the chromemargin attribute on the window needs to be set to, I believe, "-1,-1,-1,-1".

https://developer.mozilla.org/en/docs/XUL/Attribute/chromemargin

Comment 31

[Aris]

Thanks Mike,

enabling/disabling tabs in titlebar through TabsInTitlebar.allowedBy("some-unique-key", false)/TabsInTitlebar.allowedBy("some-unique-key", true) works in my tests.

I will look more into it once the official setting gets removed from about:config.

Comment 32

[Dão Gottwald [:dao]]

Comment on attachment 795404 [details] [diff] [review]
remove browser.tabs.drawInTitlebar pref support

This patch yields no win for us if we're keeping the API and telling add-ons to use it. On the contrary, the hidden pref adds the benefit that we can use it for development. For instance, setting it to false shows that on Windows we're setting a margin on the tabs toolbar without respecting the tabsintitlebar attribute as we should.

Comment 33

[Mike Conley (:mconley) (:⚙️)]

(In reply to Aris from comment #31)
> Thanks Mike,
> 
> enabling/disabling tabs in titlebar through
> TabsInTitlebar.allowedBy("some-unique-key",
> false)/TabsInTitlebar.allowedBy("some-unique-key", true) works in my tests.
> 
> I will look more into it once the official setting gets removed from
> about:config.

Cool, glad it worked for you.

Comment 34

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #32)
> Comment on attachment 795404 [details] [diff] [review]
> remove browser.tabs.drawInTitlebar pref support
> 
> This patch yields no win for us if we're keeping the API and telling add-ons
> to use it. On the contrary, the hidden pref adds the benefit that we can use
> it for development.

There's a clear difference between having an add-on deal with this functionality and having it available as a hidden pref. The hidden pref implies some level of support and/or prioritization of bugs, and delegating to add-ons does not. I don't think choosing to support toggling this pref is the right thing to do. I don't understand why having add-ons using the allowedBy calls changes anything with regard to the rationale as articulated earlier in this bug.

> For instance, setting it to false shows that on Windows
> we're setting a margin on the tabs toolbar without respecting the
> tabsintitlebar attribute as we should.

This seems orthogonal, as presumably that would be an issue for popup windows, too. Can you file a separate bug?

Comment 35

[Dão Gottwald [:dao]]

(In reply to :Gijs Kruitbosch from comment #34)
> (In reply to Dão Gottwald [:dao] from comment #32)
> > Comment on attachment 795404 [details] [diff] [review]
> > remove browser.tabs.drawInTitlebar pref support
> > 
> > This patch yields no win for us if we're keeping the API and telling add-ons
> > to use it. On the contrary, the hidden pref adds the benefit that we can use
> > it for development.
> 
> There's a clear difference between having an add-on deal with this
> functionality and having it available as a hidden pref. The hidden pref
> implies some level of support and/or prioritization of bugs, and delegating
> to add-ons does not.

The API does imply a similar level of support, because it's us who explicitly provide and maintain the API. We can't delegate this to add-ons. With the hidden pref being just another API consumer, we end up maintaining exactly the same code paths. The idea that we could somehow let this code rot if add-ons called it but not if a hidden pref did seems bogus to me.

> > For instance, setting it to false shows that on Windows
> > we're setting a margin on the tabs toolbar without respecting the
> > tabsintitlebar attribute as we should.
> 
> This seems orthogonal, as presumably that would be an issue for popup
> windows, too. Can you file a separate bug?

It doesn't affect popup windows as the tabs toolbar is hidden there, unless you forcefully add a tab (we provide no UI for that).

Comment 36

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #35)
> (In reply to :Gijs Kruitbosch from comment #34)
> > (In reply to Dão Gottwald [:dao] from comment #32)
> > > Comment on attachment 795404 [details] [diff] [review]
> > > remove browser.tabs.drawInTitlebar pref support
> > > 
> > > This patch yields no win for us if we're keeping the API and telling add-ons
> > > to use it. On the contrary, the hidden pref adds the benefit that we can use
> > > it for development.
> > 
> > There's a clear difference between having an add-on deal with this
> > functionality and having it available as a hidden pref. The hidden pref
> > implies some level of support and/or prioritization of bugs, and delegating
> > to add-ons does not.
> 
> The API does imply a similar level of support, because it's us who
> explicitly provide and maintain the API. We can't delegate this to add-ons.
> With the hidden pref being just another API consumer, we end up maintaining
> exactly the same code paths. The idea that we could somehow let this code
> rot if add-ons called it but not if a hidden pref did seems bogus to me.

I'm confused. You're arguing that this removal is useless because it doesn't save us maintenance. On the other hand, you're saying the bug you pointed out only shows up when you use the pref, and not in other situations like a popup window or any other codepaths that set the attribute. The "API" is there because we use it ourselves: https://mxr.mozilla.org/projects-central/search?string=allowedBy%28&find=%2Fux%2F&findi=&filter=^[^\0]*%24&hitlimit=&tree=projects-central . We can't remove it until/unless we don't use it.

The cases where we use it are limited and known; we should not break those. Removing the pref removes a (very general!) usecase, which means there are fewer cases to worry about when we might not be drawing tabs in the titlebar, and when we may need to change a bunch of styling.

Yes, we could code the allowedBy code differently so that it can't be called with arbitrary new values and is less of an "API", but that seems like a waste of time to me.

I don't believe that add-ons offering the same functionality we're removing here means we then have to fix bugs that appear when combining the add-on with Firefox. If we don't provide the functionality ourselves, then it can also be the add-on's job to add CSS for it. I don't see how this should be different from any other add-on, and/or why that's a reason not to go ahead with the removal.

Comment 37

[Dão Gottwald [:dao]]

(In reply to :Gijs Kruitbosch from comment #36)
> (In reply to Dão Gottwald [:dao] from comment #35)
> > The API does imply a similar level of support, because it's us who
> > explicitly provide and maintain the API. We can't delegate this to add-ons.
> > With the hidden pref being just another API consumer, we end up maintaining
> > exactly the same code paths. The idea that we could somehow let this code
> > rot if add-ons called it but not if a hidden pref did seems bogus to me.
> 
> I'm confused. You're arguing that this removal is useless because it doesn't
> save us maintenance. On the other hand, you're saying the bug you pointed
> out only shows up when you use the pref, and not in other situations like a
> popup window or any other codepaths that set the attribute. The "API" is
> there because we use it ourselves:

You're missing here that we wanted add-ons to use the API in exchange for the pref removal (comment 24). Add-ons will trigger the same bug currently exposed by the pref.

> I don't believe that add-ons offering the same functionality we're removing
> here means we then have to fix bugs that appear when combining the add-on
> with Firefox. If we don't provide the functionality ourselves, then it can
> also be the add-on's job to add CSS for it. I don't see how this should be
> different from any other add-on, and/or why that's a reason not to go ahead
> with the removal.

If add-ons call /our/ code and /we/ set a general-purpose attribute such as "tabsintitlebar", it's unreasonable to expect add-ons to fix up our CSS where it ignores that attribute.

Comment 38

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #37)
> You're missing here that we wanted add-ons to use the API in exchange for
> the pref removal (comment 24). Add-ons will trigger the same bug currently
> exposed by the pref.

I didn't miss it, I just don't think about the consequences the same way you do.

> > I don't believe that add-ons offering the same functionality we're removing
> > here means we then have to fix bugs that appear when combining the add-on
> > with Firefox. If we don't provide the functionality ourselves, then it can
> > also be the add-on's job to add CSS for it. I don't see how this should be
> > different from any other add-on, and/or why that's a reason not to go ahead
> > with the removal.
> 
> If add-ons call /our/ code and /we/ set a general-purpose attribute such as
> "tabsintitlebar", it's unreasonable to expect add-ons to fix up our CSS
> where it ignores that attribute.

This is where we disagree. If we currently employ CSS wherever that attribute is set by us (disregarding the pref case), and there are no visual issues when we set it, then I don't think we need to fix anything for the hypothetical scenario where add-ons call our code in other cases, and I don't think expecting add-ons to add a few lines of CSS is a problem.

We don't seem to agree here; I'd like to get other opinions. Gavin is hard to reach at the moment, so I'd like to hear from Justin and Matt instead.

Comment 39

[Dão Gottwald [:dao]]

(In reply to :Gijs Kruitbosch from comment #38)
> > If add-ons call /our/ code and /we/ set a general-purpose attribute such as
> > "tabsintitlebar", it's unreasonable to expect add-ons to fix up our CSS
> > where it ignores that attribute.
> 
> This is where we disagree. If we currently employ CSS wherever that
> attribute is set by us (disregarding the pref case), and there are no visual
> issues when we set it, then I don't think we need to fix anything for the
> hypothetical scenario where add-ons call our code in other cases,

The attribute is always set by us. It's an integral part of the API we provide.

And the add-on scenario isn't hypothetical, it would be a direct consequence of add-ons using that API.

> and I don't think expecting add-ons to add a few lines of CSS is a problem.

It's fundamentally a bug in our theme code. Add-ons *can* work around it, but making that a requirement is bogus. (It's also not as straightforward as you make it sound when you say "add a few lines of CSS", e.g. because the theme code is not consistent across operating systems.)

Comment 40

[Dão Gottwald [:dao]]

Attachment: alternative untested WIP patch

Here's an alternative patch that attempts to address the issue this bug was filed for by making updateTitlebarDisplay depend on TabsInTitlebar rather than the other way around. It's untested and may have issues.

Comment 41

[al_9x]

(In reply to Dão Gottwald [:dao] from comment #40)
> Created attachment 800297 [details] [diff] [review]
> alternative untested WIP patch
> 
> Here's an alternative patch that attempts to address the issue this bug was
> filed for by making updateTitlebarDisplay depend on TabsInTitlebar rather
> than the other way around. It's untested and may have issues.

[e-mail] recommended the following to address Bug 880545

(dagger.bugzilla from comment #8)
> it needs fixing to display the OS
> titlebar rather than the XUL titlebar, which just involves hooking the pref
> up to the existing code to do that, and which I can write the patch for

Does your patch cover that? Could you make a build?

Comment 42

[Dão Gottwald [:dao]]

Attachment: invert updateTitlebarDisplay / TabsInTitlebar dependency

This seems to work well and I guess it's even a simplification in that it gets rid of updateTitlebarDisplay's custom conditions for when to draw in the title bar. I haven't run any tests yet... https://tbpl.mozilla.org/?tree=Try&rev=68333ce9e445

(In reply to al_9x from comment #41)
> > it needs fixing to display the OS
> > titlebar rather than the XUL titlebar, which just involves hooking the pref
> > up to the existing code to do that, and which I can write the patch for
> 
> Does your patch cover that? Could you make a build?

This patch solves the problem, but it doesn't do so by "hooking the pref up to the existing code", which is the approach I r-'d in bug 893065.

Comment 43

[al_9x]

(In reply to Dão Gottwald [:dao] from comment #42)
> Created attachment 800462 [details] [diff] [review]
> alternative patch
> 
> This seems to work well and I guess it's even a simplification in that it
> gets rid of updateTitlebarDisplay's custom conditions for when to draw in
> the title bar. I haven't run any tests yet...
> https://tbpl.mozilla.org/?tree=Try&rev=68333ce9e445
> 

fixed in this build, thanks

Comment 44

[Dão Gottwald [:dao]]

Attachment: make the tabs toolbar margin on Windows respect the tabsintitlebar attribute

This fixes the theme issue on Windows we were talking about, which shows up when flipping the pref, or when add-ons use the TabsInTitlebar.allowedBy API, or when add-ons call gBrowser.addTab in a popup.

Comment 45

[:Gijs (he/him)]

Comment on attachment 800627 [details] [diff] [review]
make the tabs toolbar margin on Windows respect the tabsintitlebar attribute

I'm still not convinced this is the right route.

Even if I were, in the patch, why only the 15px margin? Right above it is a similar rule for margins when the menubar is hidden, with a large comment about how it's designed to work to align the tabs to the top of the window. Presumably that would need to be updated, too.

Comment 46

[Dão Gottwald [:dao]]

(In reply to :Gijs Kruitbosch from comment #45)
> Even if I were, in the patch, why only the 15px margin? Right above it is a
> similar rule for margins when the menubar is hidden, with a large comment
> about how it's designed to work to align the tabs to the top of the window.
> Presumably that would need to be updated, too.

No, the rule with the 3px margin is about the menu bar being visible.

Comment 47

[Matthew N. [:MattN]]

Some quick comments:
* I think the priority of this bug should be lowered as the pref isn't and was never supported AFAIK so 
  this doesn't meet the P3 criteria of "the average user" running into it IMO.
**  We have more important bugs on supported configurations to worry about. I think I spent too much time 
    on this bug already.
* I do use the pref when testing patches affecting tabs in titlebar calculations like Dão mentioned in 
  comment 32. (Not that this alone is a reason to keep it).
* The benefit to removing the pref seems really low IMO.
* There will be an addon-compat cost from removing it (especially for add-ons whose purpose is to disable 
  this). There are a few add-ons which will end up having exceptions thrown from getBoolPref calls on a 
  non-existent pref.
* I definitely don't think we should add support for the pref on non-Windows platforms because 
  we shouldn't have add-ons or users already using it there.
**  This doesn't necessarily mean it can't exist in about:config though. It still won't be supported.
**  I guess this makes the development/testing argument from above weaker.
* Since I don't see a strong reason to remove it, my default stance would be to go with the current behaviour.
** If that means taking some minor regression fixes that Dão has provided, then that doesn't bother me. Any resolution is better than spending more time on this IMO but I don't feel strongly either way.

Unfortunately, I don't think I really helped here.

Comment 48

[Matthew N. [:MattN]]

Comment on attachment 800462 [details] [diff] [review]
invert updateTitlebarDisplay / TabsInTitlebar dependency

This seems reasonable if we go down this route. I'm glad this simplifies updateTitlebarDisplay.

Comment 49

[:Gijs (he/him)]

Comment on attachment 800627 [details] [diff] [review]
make the tabs toolbar margin on Windows respect the tabsintitlebar attribute

I'm still not convinced keeping the 3px thing is the right thing to do here, but it's probably not terribly important.

Comment 50

[:Gijs (he/him)]

Comment on attachment 800462 [details] [diff] [review]
invert updateTitlebarDisplay / TabsInTitlebar dependency

Review of attachment 800462 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/browser.js
@@ -4490,5 @@
>  };
>  
>  #ifdef CAN_DRAW_IN_TITLEBAR
>  function updateTitlebarDisplay() {
> -  let drawInTitlebar = !gInPrintPreviewMode && window.toolbar.visible;

You added an allowedBy call for printpreview, but I'm not seeing how this patch covers the window.toolbar.visible case. What am I missing? :-)

Comment 51

[Dão Gottwald [:dao]]

(In reply to :Gijs Kruitbosch from comment #50)
> >  #ifdef CAN_DRAW_IN_TITLEBAR
> >  function updateTitlebarDisplay() {
> > -  let drawInTitlebar = !gInPrintPreviewMode && window.toolbar.visible;
> 
> You added an allowedBy call for printpreview, but I'm not seeing how this
> patch covers the window.toolbar.visible case. What am I missing? :-)

It's covered by https://hg.mozilla.org/projects/ux/file/tip/browser/base/content/tabbrowser.xml#l3394

Comment 52

[:Gijs (he/him)]

(In reply to Matthew N. [:MattN] from comment #47)
> * Since I don't see a strong reason to remove it, my default stance would be
> to go with the current behaviour.

I agree with this, and the deprioritization / time constraints point. So I guess we should get this moving.

Comment 53

[:Gijs (he/him)]

Comment on attachment 800462 [details] [diff] [review]
invert updateTitlebarDisplay / TabsInTitlebar dependency

This looks good to me. When this lands, let's keep an eye on talos (esp. tpaint/ts_paint/tart) numbers. I believe this should be either 0-effect or positive, but I'd hate to regress things at this point in time.

Comment 54

[Dão Gottwald [:dao]]

https://hg.mozilla.org/projects/ux/rev/ed0e0347b778
https://hg.mozilla.org/projects/ux/rev/7f26161e7dd6

Comment 55

[Terrell Kelley]

> I also don't believe your claim that people will be happy without "full
> coverage", especially not if the brokenness is worse than just using tabs in
> titlebar. Finally, people living with bugs doesn't mean that somehow it's a
> good idea to have half-working code hanging around, especially if we think
> that the resulting experience (even in case of perfectly functional code) is
> inferior to the default one.

We've had to deal with quite a few for a long time, like the fact that you can't fix the spacing on the Bookmarks toolbar, or how you didn't test out of process flash on netbooks (where it slows everything to a crawl.) Or how completely broken acceleration is on cards that accelerate fine on Chrome or Internet Explorer. We just figure out a way around it and move on.

And it's not like you have to test all of it, any more than you test most other configurations that are borked. The people using nightly, aurora, or beta will test it for you. That's the entire point of those releases. If you are still testing every possible configuration, no wonder Mozilla dev time is so much slower than that of Chrome. If something breaks at any point in the cycle before release, back it out.

Heck, you have a volunteer right now who says they will test it for you. 

Plus, why in the world would you think an addon developer would have more computers and more time to test?

Comment 56

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/rev/ed0e0347b778
https://hg.mozilla.org/mozilla-central/rev/7f26161e7dd6