893065 - browser.tabs.drawInTitlebar = false doesn't work on OS X

Status: RESOLVED WONTFIX

(Firefox :: General, defect)

Description

[Adam Roach [:abr]]

With the new Australis in UX Nightly, the browser.tabs.drawInTitlebar pref is still present, but setting it to "false" causes unexpected behavior. On OS X, the leftmost tab is moved under the window controls, and no title bar is rendered.

It would be preferable if setting this pref to "false" actually returned the titlebar to the top of the window. If we elect not to do that, we should at least remove the pref so that it doesn't cause UI issues.

Comment 1

[Adam Roach [:abr]]

Attachment: Screenshot of window controls overlapping left tab

Comment 2

[Adam Roach [:abr]]

Some IRC notes from JosiahOne:

No, there isn't really a way I can find to accomplish this easily. How you _would_ accomplish this is by setting the #main-window attribute "chromemargin" to "1,1,1,1". Then, set #titlebar's css property "margin-bottom" to 40px. I can do this in DOMi and it will work.

Unfortunately I can't find a way to modify XUL element attributes permanently, so this doesn't really help. Maybe there is some way to accomplish it. If there is a way to modify XUL element attributes, then I could see this happening.

But after some DOMi tweaking, I got this: (see attachment)

Comment 3

[Adam Roach [:abr]]

Attachment: Expected appearance when browser.tabs.drawInTitlebar == false

Comment 4

[Josiah Bruner [:jsbruner]]

I should be able to fix this in a couple days...

Comment 5

[Josiah Bruner [:jsbruner]]

Attachment: Fix.

Here's a fix. Mike, could you review this? Wasn't positive if the #ifdefs were all necessary, but I believe they are.

I believe a ui-review will be needed as well.

Comment 6

[Josiah Bruner [:jsbruner]]

Comment on attachment 774901 [details] [diff] [review]
Fix.

># HG changeset patch
># User JosiahOne <[e-mail]>
># Date 1373662680 14400
># Node ID 83e95c8fbe165b48c7b5c1ae070bb0c738eac9eb
># Parent  56af4ca8e542d5c5e25ee317622502921751036a
>Bug 893065 - Fix drawInTitlebar pref
>
>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js
>--- a/browser/base/content/browser.js
>+++ b/browser/base/content/browser.js
>@@ -4483,17 +4483,23 @@
>         };
> 
>         this._draghandles.tabsToolbar = new tmp.WindowDraggingElement(tabsToolbar);
>         this._draghandles.tabsToolbar.mouseDownCheck = mouseDownCheck;
> 
>         this._draghandles.navToolbox = new tmp.WindowDraggingElement(gNavToolbox);
>         this._draghandles.navToolbox.mouseDownCheck = mouseDownCheck;
>       }
>+#ifdef CAN_DRAW_IN_TITLEBAR
>+      updateTitlebarDisplay()
>+#endif
>     } else {
>+#ifdef CAN_DRAW_IN_TITLEBAR
>+      updateTitlebarDisplay()
>+#endif
>       document.documentElement.removeAttribute("tabsintitlebar");
>     }
>   },
> 
>   _sizePlaceholder: function (type, width) {
>     Array.forEach(document.querySelectorAll(".titlebar-placeholder[type='"+ type +"']"),
>                   function (node) { node.width = width; });
>   },
>@@ -4530,16 +4536,21 @@
>   let drawInTitlebar = !gInPrintPreviewMode && window.toolbar.visible;
>   document.getElementById("titlebar").hidden = !drawInTitlebar;
> 
>   if (drawInTitlebar)
>     document.documentElement.setAttribute("chromemargin", "0,2,2,2");
>   else
>     document.documentElement.removeAttribute("chromemargin");
> 
>+#ifdef XP_MACOSX
>+  if (Services.prefs.getBoolPref("browser.tabs.drawInTitlebar") == false)
>+    document.documentElement.setAttribute("chromemargin", "1,1,1,1");
>+#endif
>+
>   TabsInTitlebar.allowedBy("drawing-in-titlebar", drawInTitlebar);
> }
> #endif
> 
> #ifdef CAN_DRAW_IN_TITLEBAR
> function onTitlebarMaxClick() {
>   if (window.windowState == window.STATE_MAXIMIZED)
>     window.restore();
>diff --git a/browser/themes/osx/browser.css b/browser/themes/osx/browser.css
>--- a/browser/themes/osx/browser.css
>+++ b/browser/themes/osx/browser.css
>@@ -52,16 +52,20 @@
> .titlebar-placeholder[type="fullscreen-button"]:-moz-locale-dir(rtl) {
>   -moz-box-ordinal-group: 0;
> }
> 
> #titlebar {
>   padding-top: 9px;
> }
> 
>+#titlebar:not([drawintitlebar="true"]) {
>+  margin-bottom: -10px;
>+}
>+
> #main-window[chromehidden~="toolbar"] > #titlebar {
>   padding-top: 22px;
> }
> 
> #main-window:not(:-moz-lwtheme):not([privatebrowsingmode=temporary]) > #titlebar {
>   -moz-appearance: -moz-window-titlebar;
> }
>

Comment 7

[Matthew N. [:MattN]]

Comment on attachment 774901 [details] [diff] [review]
Fix.

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

We weren't planning on supporting the pref set to false on OS X as it's added maintenance cost for only a handful of users who find the pref. If this patch ends up being very straightforward we can consider it. An extension could make these changes instead.

::: browser/base/content/browser.js
@@ +4494,3 @@
>      } else {
> +#ifdef CAN_DRAW_IN_TITLEBAR
> +      updateTitlebarDisplay()

I believe this works fine on Windows without these additions so why are they necessary on OS X?

@@ +4542,5 @@
>      document.documentElement.removeAttribute("chromemargin");
>  
> +#ifdef XP_MACOSX
> +  if (Services.prefs.getBoolPref("browser.tabs.drawInTitlebar") == false)
> +    document.documentElement.setAttribute("chromemargin", "1,1,1,1");

It seems like this can just go in the else right above this.

Comment 8

[Josiah Bruner [:jsbruner]]

(In reply to Matthew N. [:MattN] from comment #7)
> Comment on attachment 774901 [details] [diff] [review]
> Fix.
> 
> Review of attachment 774901 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> We weren't planning on supporting the pref set to false on OS X as it's
> added maintenance cost for only a handful of users who find the pref. If
> this patch ends up being very straightforward we can consider it. An
> extension could make these changes instead.

Few things.

This patch is very straight forward and I would say that there really isn't any maintenance cost at all. (Or if there is, very little) For example, on the semi-unstable UX branch, where no real effort has been made to make drawInTitlebar work while false, only two changes are needed to add back the titlebar, and it works flawlessly. We only need to modify the chromemargin (Which is used on other windows, so that will stay supported) and the #titlebar's position, which isn't really hard to manage. I can't really think of how adding back a native titlebar will any significant decrease in development.

As a compromise, we could land this, fixing the pref, but if it ever becomes a problem in the future, then we don't need to worry about it. You don't necessarily have to "support" the pref to land this. At least for now, this will make *many* users happy. We don't need to make everything look pretty. (For example Private Browsing windows with this patch draw a native titlebar, but who cares, that's what people want)

I'm just saying to consider this with an open mind. Australis is a big change for people, and many will be upset by the lack of a title bar. This is a very simple fix for that.

> 
> ::: browser/base/content/browser.js
> @@ +4494,3 @@
> >      } else {
> > +#ifdef CAN_DRAW_IN_TITLEBAR
> > +      updateTitlebarDisplay()
> 
> I believe this works fine on Windows without these additions so why are they
> necessary on OS X?

On OS X we don't call updateTitlebarDisplay() enough to update the chromemargin as needed here. Without this call, changing the pref will update the css, but will do nothing to the chromemargin until this function is called again (For example by adding/hiding the bookmarks bar)

Windows probably doesn't need this because updateTitlebarDisplay() most likely gets called on maximize and minimize, whereas OS X doesn't have such a feature.

> 
> @@ +4542,5 @@
> >      document.documentElement.removeAttribute("chromemargin");
> >  
> > +#ifdef XP_MACOSX
> > +  if (Services.prefs.getBoolPref("browser.tabs.drawInTitlebar") == false)
> > +    document.documentElement.setAttribute("chromemargin", "1,1,1,1");
> 
> It seems like this can just go in the else right above this.

It can not. I tried that, but the else is for Windows only and is used for when drawInTitlebar is not needed (by un-maximizing  the window). We still need to check the preference on OS X.

Comment 9

[Josiah Bruner [:jsbruner]]

Comment on attachment 774901 [details] [diff] [review]
Fix.

Because of my previous statement I am resetting the review flag.

Comment 10

[Dão Gottwald [:dao]]

Comment on attachment 774901 [details] [diff] [review]
Fix.

>--- a/browser/base/content/browser.js
>+++ b/browser/base/content/browser.js
>@@ -4483,17 +4483,23 @@
>         };
> 
>         this._draghandles.tabsToolbar = new tmp.WindowDraggingElement(tabsToolbar);
>         this._draghandles.tabsToolbar.mouseDownCheck = mouseDownCheck;
> 
>         this._draghandles.navToolbox = new tmp.WindowDraggingElement(gNavToolbox);
>         this._draghandles.navToolbox.mouseDownCheck = mouseDownCheck;
>       }
>+#ifdef CAN_DRAW_IN_TITLEBAR
>+      updateTitlebarDisplay()
>+#endif
>     } else {
>+#ifdef CAN_DRAW_IN_TITLEBAR
>+      updateTitlebarDisplay()
>+#endif
>       document.documentElement.removeAttribute("tabsintitlebar");
>     }
>   },

As Matt indicated, this doesn't make sense. updateTitlebarDisplay calls TabsInTitlebar, not the other way around. No part of updateTitlebarDisplay depends on what TabsInTitlebar is doing.

Comment 11

[Josiah Bruner [:jsbruner]]

(In reply to Dão Gottwald [:dao] from comment #10)
> Comment on attachment 774901 [details] [diff] [review]
> Fix.
> 
> >--- a/browser/base/content/browser.js
> >+++ b/browser/base/content/browser.js
> >@@ -4483,17 +4483,23 @@
> >         };
> > 
> >         this._draghandles.tabsToolbar = new tmp.WindowDraggingElement(tabsToolbar);
> >         this._draghandles.tabsToolbar.mouseDownCheck = mouseDownCheck;
> > 
> >         this._draghandles.navToolbox = new tmp.WindowDraggingElement(gNavToolbox);
> >         this._draghandles.navToolbox.mouseDownCheck = mouseDownCheck;
> >       }
> >+#ifdef CAN_DRAW_IN_TITLEBAR
> >+      updateTitlebarDisplay()
> >+#endif
> >     } else {
> >+#ifdef CAN_DRAW_IN_TITLEBAR
> >+      updateTitlebarDisplay()
> >+#endif
> >       document.documentElement.removeAttribute("tabsintitlebar");
> >     }
> >   },
> 
> As Matt indicated, this doesn't make sense. updateTitlebarDisplay calls
> TabsInTitlebar, not the other way around. No part of updateTitlebarDisplay
> depends on what TabsInTitlebar is doing.

Well, apparently this is not true. I did a test, and here's what I discovered.

Setup: I added a log statement to the beginning of _update and a log statement at the beginning of updateTitlebarDisplay.

Process: First, *without* this patch applied, I changed drawInTitlebar via about:config. This resulted in a single log statement in the Browser Console:

"_update called"

^ So update then was called by itself without drawInTitlebar.

       Second, I repeated the experiment with my patch applied, did the same thing and got:

"_update called"
"updateTitlebarDisplay called"
"_update called"

And that was it. No infinite loops, and no real negative side effects.

If you think it would be better, I could create a new function that updates the chromemargin only and add that to _update. That would save us a updateTitlebarDisplay and another _update I believe.

Would that be a better approach?

Comment 12

[Josiah Bruner [:jsbruner]]

Wait, on second thought creating a new function wouldn't help either, as we would still need to use _update to call the function, and this function would need to call _update. So there's no difference.

So I guess there are two approaches. Keep it as is. Or, send updateTitlebarDisplay when a pref is changed.

Is there really a problem with the first approach?

Comment 13

[Dão Gottwald [:dao]]

(In reply to Josiah Bruner [:JosiahOne] from comment #12)
> Is there really a problem with the first approach?

Yes, even though there's no infinite loop (because TabsInTitlebar.allowedBy is smart enough to prevent it), it turns dependencies around in a messy way. The underlying problem is that you want browser.tabs.drawInTitlebar=false to completely disable drawing in the title bar, but it never did that ever since that pref got introduced on Windows.

Comment 14

[Justin Dolske [:Dolske]]

We should be removing this pref, not trying to keep it around. It's not about the cost of any one patch, but the holistic host. Here's a couple recent articles on the subject:

http://nacin.com/2013/07/01/firefox-makes-a-decision-removes-an-option/
http://firstround.com/article/The-one-cost-engineers-and-product-managers-dont-consider

An add-on is the proper replacement way to let users alter this UI.

Comment 15

[Justin Dolske [:Dolske]]

* holistic cost, not host. (Unintentional alliteration typo)

Comment 16

[Josiah Bruner [:jsbruner]]

Comment on attachment 774901 [details] [diff] [review]
Fix.

Fair enough, in that case I can stop working on optimizing the previous patch. So removing the entire pref is the plan now? I'll start working on that then.

Comment 17

[Josiah Bruner [:jsbruner]]

Whoops, didn't see the WONTFIX. Ignore the last comment.