930094 - Browser windows sometimes have the nav-bar drawn in the titlebar

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Stephen Horlander [:shorlander]]

Attachment: Titlebar-less Pop-Up Widow

STR:

1) Open a pop-up window e.g. from a popular video site like Amazon Instant or Hulu
2) Enter Customize mode in another window
3) Titlebar disappears in pop-up window causing window widgets to shift to the navBar

Comment 2

[:Gijs (he/him)]

Annnnd I still can't reproduce this. Argh. Stephen, you marked this all/all - is it actually reproducible on Windows/Linux? If you've not tried, then what version of OS X are you running and what version of UX?

Comment 3

[Dão Gottwald [:dao]]

(In reply to :Gijs Kruitbosch from comment #2)
> Annnnd I still can't reproduce this. Argh. Stephen, you marked this all/all
> - is it actually reproducible on Windows/Linux?

Windows is possible, but we can't mess with the title bar like this on Linux.

Comment 4

[Stephen Horlander [:shorlander]]

I have only seen it on OS X, forgot to change platform. So… cannot reproduce on the latest UX nightly but it was happening every time up until today.

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

Stephen, now that it's been a week since the last comment, any luck reproducing it on latest nightly? If not, we should close this bug as RESO-WORKSFORME.

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

Bug 936543 shows another reproduction of this in a newer build.

Comment 8

[Stephen Horlander [:shorlander]]

I tried this again and for me I can only reproduce this if the window was created on a retina display. It doesn't have to end up on the retina display, it only has to start there.

Comment 9

[Chenxia Liu [:liuche]]

Attachment: Screeshot: gchat window (11/10 UX build)

Bug 888022 was duped to this, and I wanted to comment that I still see it present, on the current UX build. Still intermittent, as well, and I don't really have an STR, although I didn't enter/exit customize mode at all.

Comment 10

[Chenxia Liu [:liuche]]

This is on a Retina running 10.9, but I'd like to note that I also used to see it intermittently on my last machine, running 10.7.5.

Comment 11

[:Gijs (he/him)]

Sevaan, do you have STR for this, as you filed bug 936543? :-(

Comment 12

[Sevaan Franks [:sevaan]]

Hey Gijs, I can't seem to reproduce the bug now, no matter what I try. I updated the build last night... maybe it's been inadvertently fixed?

Comment 13

[:Gijs (he/him)]

(In reply to Sevaan Franks from comment #12)
> Hey Gijs, I can't seem to reproduce the bug now, no matter what I try. I
> updated the build last night... maybe it's been inadvertently fixed?

I would be extremely surprised. It's just been extremely hard to reproduce, we've already mistakenly closed this bug twice now, so let's leave it open.

If anyone sees it again, please check with one of Mike Conley, Matt Noorenberghe, Mike de Boer, Blair McBride, Jared Wein or me, and/or investigate yourself with DOM Inspector. I still have no idea what's causing this because I've never seen it, and I think it's the same with the rest of the dev team. It's really terrible, so we just need STR and a better idea of what's causing this, tbh.

Comment 14

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

I don't even need to enter customize mode to reproduce this. I just need to open a pop-up window on my Retina display.

I can reproduce this reliably.

Comment 15

[Stephen Horlander [:shorlander]]

Has anyone seen this on a non-Retina display?

Comment 16

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

Attachment: testcase.html

Open this page on a Retina display, and see the magical integration between the nav-bar and the window / full-screen buttons.

Comment 17

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

More facts - the bug disappears if the window is not fully zoomed.

Comment 18

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

(In reply to Mike Conley (:mconley) from comment #17)
> More facts - the bug disappears if the window is not fully zoomed.

Nope nope nope - flipped my signs there. The bug disappears if the window *is* fully zoomed.

Comment 19

[Sevaan Franks [:sevaan]]

(In reply to Stephen Horlander [:shorlander] from comment #15)
> Has anyone seen this on a non-Retina display?

I don't have a retina display and have seen it (bug 936543). Can't reproduce it now though.

Comment 20

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

For some reason, the chromemargin attribute is being set on the window when it opens, which is causing us to collapse into the titlebar.

I've been dealing with some of this stuff in bug 933933, so I'll look at this too.

Comment 21

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

I have traced this down to the chromemargin being set in LightweightThemeConsumer.jsm.

Comment 22

[:Gijs (he/him)]

(In reply to Mike Conley (:mconley) from comment #16)
> Created attachment 833082 [details]
> testcase.html
> 
> Open this page on a Retina display, and see the magical integration between
> the nav-bar and the window / full-screen buttons.

I'm so confused. I tried the testcase on stock UX nightly from Saturday (there's no newer ones yet). Still don't see it. Retina mbp, 10.9.

(In reply to Mike Conley (:mconley) from comment #18)
> (In reply to Mike Conley (:mconley) from comment #17)
> > More facts - the bug disappears if the window is not fully zoomed.
> 
> Nope nope nope - flipped my signs there. The bug disappears if the window
> *is* fully zoomed.

What does this mean? I've been using cmd-+/- to decrease page zoom on the opener tab, but that doesn't seem to change anything. As you're opening about:blank, I can't change the page zoom on the opened popup window. I've also tried changing the OS X zoom thing (ctrl+scroll) but that doesn't seem to affect anything either.

Comment 23

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

(In reply to :Gijs Kruitbosch from comment #22)
> What does this mean?

Sorry, I should have been more clear. By "zoomed", I meant "maximized", via the green button in the titlebar. This button's always been called "zoom", at least as far back as I can remember.

Comment 24

[Sevaan Franks [:sevaan]]

I can reproduce this bug on my Macbook Air now by:

* Connecting external monitor
* Opening Australis on external monitor
* Launching chromeless window from external monitor

Will not reproduce is browser is on Macbook.

Comment 25

[Sevaan Franks [:sevaan]]

Forgot to add: browser window should NOT be maximized.

Comment 26

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

Ok, let me summarize my findings here:

* The LightweightThemeConsumer.jsm is a toolkit module that's used by other XUL apps like Thunderbird and Instantbird to make lightweight themes possible.
* In Bug 893682, the chromemargin was added to the main-window by default for performance reasons.
* For OS X, the LightweightThemeConsumer takes a reading of the chromemargin attribute when it is being constructed for the window and caches the value. When the lw-theme is disabled, the old chromemargin value (if one existed) is restored.

On pre-Australis m-c, OS X is a bit of a special-case for lw-themes. In particular, when we enable a lw-theme on OS X, we start drawing in the titlebar to get the lw-theme up there. When we disable, we stop drawing in there.

On Windows, if drawing in the titlebar is disabled (like on Windows XP if the menubar is enabled), then we *don't* draw in the titlebar with lw-themes.

UX changed things for OS X - we *always* draw in the titlebar now, whether or not we're using lightweight themes, or even drawing tabs in the titlebar. This is unique for UX / Firefox.

So here's how things go down:

* When the popup window is constructed, the root-element binding is attached and a LightweightThemeConsumer instance is created. The LightweightThemeConsumer constructor caches the default value of chromemargin for OS X ("0,-1,-1,-1").
* browser.js runs updateTitlebarDisplay, which removes the chromemargin attribute, since TabsInTitlebar.enabled is false.
* The window is fully spawned, and due to OS X / Gecko strangeness, fires a resize event sometimes - this is the part that we seem to have inconsistent reproduction on. When the resize event fires, the LightweightThemeConsumer notices, and then also notices that we're not using a lw-theme, and tries to restore the chromemargin to the default value (which we cached in the first step). So it sets the chromemargin attribute, and BLAM, we're drawing in the titlebar, and the nav-bar gets put up there.

Comment 27

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

Attachment: 930094-v1.diff

Gijs had a good point - since we're *always* drawing in the titlebar on OS X, the updateTitlebarDisplay really has no business removing the chromemargin attribute for it.

Comment 28

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

Comment on attachment 8333954 [details] [diff] [review]
930094-v1.diff

Gijs, what do you think of this?

Comment 29

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

Comment on attachment 8333954 [details] [diff] [review]
930094-v1.diff

This broke browser.tabs.drawInTitlebar = false.

Comment 30

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

Attachment: Patch v1.1

So this one is a little more rigorous. I'm using the same methodology before (not removing chromemargin if we're OS X), but updating the browser.css for OS X so that we don't look like ass for pop-up windows or when not drawing the tabs in the titlebar.

This has, however, exposed a new bug - we don't display the window titles if we're drawing in the titlebar (because this would overlap the tabs if we're drawing the tabs up there). This, however, means no window titles for pop-up windows, or when browser.tabs.drawInTitlebar is false.

I think we should file a separate bug for that and get mstange or smichaud to give us an attribute that lets us tell Cocoa when to draw the window title.

Comment 31

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

Comment on attachment 8334174 [details] [diff] [review]
Patch v1.1

Seems to work fine for OS X and Windows.

What do you think, Gijs?

Comment 32

[:Gijs (he/him)]

Comment on attachment 8334174 [details] [diff] [review]
Patch v1.1

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

::: browser/themes/osx/browser.css
@@ +81,3 @@
>  #main-window[chromehidden~="toolbar"] > #titlebar {
> +  padding-top: 0;
> +  min-height: 24px;

Why +2px?

Comment 33

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

Attachment: Patch v1.2

Good point - that was a leftover from me being frustrated with the height of things. It turns out I just needed to remove the negative margin from the nav-bar.

Comment 34

[:Gijs (he/him)]

Comment on attachment 8334198 [details] [diff] [review]
Patch v1.2

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

It looks OK to me but I'd like Matt to take a look, sorry.

Comment 35

[Matthew N. [:MattN]]

Comment on attachment 8334198 [details] [diff] [review]
Patch v1.2

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

I think this can be simplified somewhat to reduce new CSS blocks and thus help maintainability.

::: browser/themes/osx/browser.css
@@ +60,5 @@
>  }
>  
> +#main-window:not([tabsintitlebar]) #titlebar-buttonbox,
> +#main-window:not([tabsintitlebar]) #titlebar-fullscreen-button {
> +  -moz-appearance: none;

I think we should instead make the selector in the content stylesheet (where we add -moz-appearance) more specific to only apply with [tabsintitlebar] instead of overriding it here (unless there is a good reason for it).

@@ +74,5 @@
>    -moz-box-ordinal-group: 0;
>  }
>  
> +#main-window:not([tabsintitlebar]) > #titlebar {
> +  min-height: 22px;

Is this supposed to represent the titlebar height? If so, make a variable for it.

@@ +82,5 @@
> +  padding-top: 0;
> +}
> +
> +#main-window[chromehidden~="toolbar"] #nav-bar {
> +  margin-top: 0;

Without looking too deeply, we can probably fix the "margin-top: -1px;" on #nav-bar to only apply when appropriate.

@@ +2594,5 @@
> + * We have to draw the bottom border of the titlebar if we're hiding the
> + * tabs toolbar, otherwise, there's no deliniation between the titlebar
> + * and the nav-bar.
> + */
> +#main-window[chromehidden~="toolbar"] #nav-bar::before {

Can we get the … #navigator-toolbox:not(:-moz-lwtheme)::before styles applied instead of adding a new pseudo-element?

@@ +2601,5 @@
> +   * and position those with ordinal attributes, and because our layout code
> +   * expects :before/:after nodes to come first/last in the frame list,
> +   * we have to reorder this element to come last, hence the
> +   * ordinal group value (see bug 853415). */
> +  -moz-box-ordinal-group: 1001;

Are you sure we want to position at the end of the nav-bar for a ::before? I think you want 0. 
Also, with bug 877890 fixed, do we even need this stuff anymore? We need to make sure we don't regress bug 853415 though.

@@ +2606,5 @@
> +  position: absolute;
> +  top: 0;
> +  left: 0;
> +  right: 0;
> +  z-index: 0;

The set of properties above this are duplicated from earlier in the file which isn't great for maintainability. We can add this rule to the existing ruleset and have a seperate ruleset for the differences.

Comment 36

[Dão Gottwald [:dao]]

(In reply to Mike Conley (:mconley) from comment #30)
> This has, however, exposed a new bug - we don't display the window titles if
> we're drawing in the titlebar (because this would overlap the tabs if we're
> drawing the tabs up there). This, however, means no window titles for pop-up
> windows, or when browser.tabs.drawInTitlebar is false.

That's actually the same bug as this one. We erroneously draw stuff in the title bar and therefore the window title gets hidden.

> I think we should file a separate bug for that and get mstange or smichaud
> to give us an attribute that lets us tell Cocoa when to draw the window
> title.

chromemargin is supposed to be that attribute. Why would we want to have a separate one?

Comment 37

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

Clearing mstange's needinfo, because I think I flagged him by accident.

Comment 38

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

(In reply to Dão Gottwald [:dao] from comment #36)
> chromemargin is supposed to be that attribute. Why would we want to have a
> separate one?

Because there's a case where we do want to draw in the titlebar, but we do want to show the title - when browser.tabs.drawInTitlebar is false, and a lightweight theme is being used.

Comment 39

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

I think we can reduce the priority of this one a bit.

Comment 40

[Dão Gottwald [:dao]]

(In reply to Mike Conley (:mconley) from comment #38)
> (In reply to Dão Gottwald [:dao] from comment #36)
> > chromemargin is supposed to be that attribute. Why would we want to have a
> > separate one?
> 
> Because there's a case where we do want to draw in the titlebar, but we do
> want to show the title - when browser.tabs.drawInTitlebar is false, and a
> lightweight theme is being used.

On Windows, we don't and never did draw in the title bar in that case.

Comment 41

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

(In reply to Dão Gottwald [:dao] from comment #40)
> (In reply to Mike Conley (:mconley) from comment #38)
> > (In reply to Dão Gottwald [:dao] from comment #36)
> > > chromemargin is supposed to be that attribute. Why would we want to have a
> > > separate one?
> > 
> > Because there's a case where we do want to draw in the titlebar, but we do
> > want to show the title - when browser.tabs.drawInTitlebar is false, and a
> > lightweight theme is being used.
> 
> On Windows, we don't and never did draw in the title bar in that case.

You're absolutely correct, except that this bug is not about Windows - it's about OS X. On OS X, we historically did draw in the titlebar in this case so that the lw-theme extended into the titlebar.

Comment 42

[Dão Gottwald [:dao]]

Historically, we never drew any content such as tabs in the title bar on OS X. Now that OS X is closer to Windows, why should it be different here?

Comment 43

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

(In reply to Dão Gottwald [:dao] from comment #42)
> Historically, we never drew any content such as tabs in the title bar on OS
> X. Now that OS X is closer to Windows, why should it be different here?

Because the spec calls for it:

http://people.mozilla.org/~shorlander/files/australis-designSpecs/australis-designSpecs-osx-mainWindow.html

Comment 44

[Dão Gottwald [:dao]]

(In reply to Mike Conley (:mconley) from comment #43)
> (In reply to Dão Gottwald [:dao] from comment #42)
> > Historically, we never drew any content such as tabs in the title bar on OS
> > X. Now that OS X is closer to Windows, why should it be different here?
> 
> Because the spec calls for it:
> 
> http://people.mozilla.org/~shorlander/files/australis-designSpecs/australis-
> designSpecs-osx-mainWindow.html

That page doesn't mention lightweight themes and popups or browser.tabs.drawInTitlebar = false, so I have no idea what you mean.

Comment 45

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

(In reply to Dão Gottwald [:dao] from comment #44)
> (In reply to Mike Conley (:mconley) from comment #43)
> > (In reply to Dão Gottwald [:dao] from comment #42)
> > > Historically, we never drew any content such as tabs in the title bar on OS
> > > X. Now that OS X is closer to Windows, why should it be different here?
> > 
> > Because the spec calls for it:
> > 
> > http://people.mozilla.org/~shorlander/files/australis-designSpecs/australis-
> > designSpecs-osx-mainWindow.html
> 
> That page doesn't mention lightweight themes and popups or
> browser.tabs.drawInTitlebar = false, so I have no idea what you mean.

Ok, I'll try to be more clear:

1) By default, OS X will draw its tabs in the titlebar, meaning we're drawing in the titlebar.
2) So as to not regress our old behaviour, when we use lightweight themes, we will also draw in the titlebar so that the texture extends to it.
3) We need to support the preference that users do not want to draw their tabs in the titlebar, which is toggled via browser.tabs.drawInTitlebar.
4) In the event that browser.tabs.drawInTitlebar is set to "false", we still need to support the case where we draw lightweight themes in the titlebar. This is the case I was referring to in comment 38 - this is the case where we draw in the titlebar but still want to see the title.
5) Popups without toolbars should have the same behaviour as before the Australis merge - meaning that we show the titlebar and the titlebar title, but that we also support lightweight themes in them.

Is there any part of that that's not clear?

Comment 46

[Dão Gottwald [:dao]]

Yes, the need for 4) and 5) is unclear, given that we don't behave like that on Windows.

Comment 47

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

(In reply to Dão Gottwald [:dao] from comment #46)
> Yes, the need for 4) and 5) is unclear, given that we don't behave like that
> on Windows.

But we do behave that way on OS X, which is the platform that this bug is concerned with...

Comment 48

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

I think there's some confusion here.

OS X and Windows have up until now *always* behaved differently (OS X draws in the titlebar when using lw-themes even when not drawing tabs in titlebar, whereas Windows does not). I'm trying to preserve that difference.

Comment 49

[Dão Gottwald [:dao]]

(In reply to Mike Conley (:mconley) from comment #48)
> I think there's some confusion here.
> 
> OS X and Windows have up until now *always* behaved differently (OS X draws
> in the titlebar when using lw-themes even when not drawing tabs in titlebar,
> whereas Windows does not).

OS X and Windows used to be completely different beasts. The UI and underlying platform capabilities mostly converged now.

> I'm trying to preserve that difference.

Why?

Comment 50

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

(In reply to Dão Gottwald [:dao] from comment #49)
> Why?

I imagine we extended the lw-theme texture into the titlebar pre-Australis for aesthetic reasons. Maybe for other reasons - I wasn't here when it happened.

Why would we not persist this?

Comment 51

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

To put my mind at ease, I've just confirmed with shorlander that we do indeed want to persist this behaviour.

Comment 52

[Dão Gottwald [:dao]]

(In reply to Mike Conley (:mconley) from comment #50)
> (In reply to Dão Gottwald [:dao] from comment #49)
> > Why?
> 
> I imagine we extended the lw-theme texture into the titlebar pre-Australis
> for aesthetic reasons. Maybe for other reasons - I wasn't here when it
> happened.
> 
> Why would we not persist this?

Because:

1) we don't do it on Windows. Other than historic baggage, what differentiates the two?

2) you're doing it at the expense of the window title, which I think is getting the priorities wrong. We should focus on getting this right and on par with Windows, and then we can have a followup bug for drawing lightweight themes behind the window title in popups.

Comment 54

[Richard Newman [:rnewman]]

Worth morphing the title to indicate that this affects all windows, not just popups?

Comment 55

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

(In reply to Richard Newman [:rnewman] from comment #54)
> Worth morphing the title to indicate that this affects all windows, not just
> popups?

Yep, good call.

Comment 56

[Richard Newman [:rnewman]]

This sounds like 

Bug 573974 -- Remember the presence of the drawintitlebar attribute in LightweightThemeConsumer.jsm 

should be forward-duped or something...?

Comment 57

[:Gijs (he/him)]

So, AFAICT the underlying issue here is also causing bug 941831. Seeing as the current plan I've heard of for bug 940093 is to add a UI pref in customize mode, that is kind of a big deal, because the breakage will be extremely visible.

I'm not really sure what's required to move forward here. Mike, can you help get this unstuck?

Comment 58

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

(In reply to :Gijs Kruitbosch from comment #57)
> I'm not really sure what's required to move forward here. Mike, can you help
> get this unstuck?

I can sure try, though that's going to take my attention from customize mode transition smoothness.

Since this affects usability though, I suppose this trumps the smoothness.

I'll see where I can get with this over the next few hours and re-evaluate.

Comment 59

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

I think I'm making good forward progress on this, but I think I'm going to try to change how we cache the chromemargin attribute for LightweightThemeConsumer. Currently, we cache it in the Consumer object itself - but I think I'm going to stash the value in an attribute on the root element so that the XUL application has the ability to invalidate the cache.

Patch soon.

Comment 60

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

Attachment: Patch v2

Comment 61

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

Attachment: Patch v2

Whoops, for real this time.

Comment 62

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

Comment on attachment 8366731 [details] [diff] [review]
Patch v2

So this fixes the stated problem in the bug for the non-lightweight theme case, and sets us up nicely for follow-up bugs that can solve the lightweight theme case.

The inline documentation already lays this out, but essentially, the first time a window _changes LWT state_ is now the point where we attempt to cache / change the chromemargin attribute in LightweightThemeConsumer. Also, instead of caching the chromemargin attribute internally, we jam it into an attribute on the root element so that XUL apps can invalidate this cache if necessary (if, for example, TabsInTitlebar is disabled while using a lightweight theme, this would be necessary so that if the lightweight theme is enabled, LightweightThemeConsumer knows to stop drawing in the titlebar).

I'm interested in feedback on this approach.

Comment 63

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

Comment on attachment 8366731 [details] [diff] [review]
Patch v2

Yikes - it looks like "" is not a valid value for chromemargin. Fixing...

Comment 64

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

Attachment: WIP Patch

Gotta go test this on Windows and Linux.

Comment 65

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

Attachment: WIP Patch

Comment 66

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

Attachment: Patch v2

Ok, I believe this works.

Tested the following states:
* Default, in and out of customize mode
* Tabs in Titlebar disabled, in and out of customize mode (and toggled while in customize mode)
* LWT applied, in and out of customize mode
* LWT applied, Tabs in Titlebar disabled, in and out of customize mode (toggled while in customize mode).


For follow-ups:
* The height of the titlebar might need some adjustment for when using a lightweight theme and tabs in titlebar is disabled - it looks a little short.
* When tabs in titlebar is disabled and a lightweight theme is applied, entering customize mode is really rough - there's a moment where it appears as if the menu panel is anchored to the far left of the screen.

Otherwise, I believe this works as advertised - and, oh yeah, fixes the original problem this bug was filed for. :)

Comment 67

[:Gijs (he/him)]

Comment on attachment 8366942 [details] [diff] [review]
Patch v2

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

Thanks for the discussion. This makes sense to me. Nice work!

::: browser/base/content/browser.js
@@ +4602,5 @@
>        document.documentElement.removeAttribute("tabsintitlebar");
>        updateTitlebarDisplay();
>  
>        // Reset the margins and padding that might have been modified:
> +      titlebarContent.style.marginTop = "";

Oops. Good catch.

@@ +4628,5 @@
>  
>  #ifdef CAN_DRAW_IN_TITLEBAR
>  function updateTitlebarDisplay() {
> +
> +#ifdef XP_MACOSX

A comment about what is going on here would not go amiss.

@@ +4645,5 @@
>  
>    if (TabsInTitlebar.enabled)
>  #ifdef XP_WIN
>      document.documentElement.setAttribute("chromemargin", "0,2,2,2");
>  #else

Because CAN_DRAW_IN_TITLEBAR is false on !XP_WIN && !XP_MACOSX, I think you can remove this else.

::: browser/themes/osx/browser.css
@@ +2586,5 @@
>  .tabbrowser-tab:focus > .tab-stack > .tab-content > .tab-label {
>    box-shadow: @focusRingShadow@;
>  }
>  
> +#main-window[tabsintitlebar]:not(:-moz-any([chromehidden~="toolbar"],[customize-entered])) > #titlebar {

Nit: Considering there's just two of these, I think I'd prefer: :not(a):not(b)

::: toolkit/modules/LightweightThemeConsumer.jsm
@@ +143,2 @@
>  
> +      if (active)

Nit: braces because the else has braces

Comment 68

[Dão Gottwald [:dao]]

Comment on attachment 8366942 [details] [diff] [review]
Patch v2

> function updateTitlebarDisplay() {
>-  document.getElementById("titlebar").hidden = !TabsInTitlebar.enabled;
>+
>+#ifdef XP_MACOSX
>+  if (TabsInTitlebar.enabled) {
>+    document.documentElement.setAttribute("chromemargin-default", "0,-1,-1,-1");
>+    document.documentElement.setAttribute("chromemargin", "0,-1,-1,-1");
>+  } else {
>+    document.documentElement.setAttribute("chromemargin-default", "");
>+    let isCustomizing = document.documentElement.hasAttribute("customizing");
>+    if (!LightweightThemeManager.currentTheme || isCustomizing) {
>+      document.documentElement.removeAttribute("chromemargin");
>+    }
>+  }

If I read this part correctly, you're still removing the window title in popups with a lightweight theme applied, which is not an acceptable state to be in as explained in comment 52. We need to get the basics right. In this case this means we need to expose the content title somewhere. If we need to make a tradeoff here, then lightweight themes being drawn in the title bar in popup windows loses since it's not basic functionality, but fine for to a followup bug.

If I didn't read the above code correctly, then it needs some documentation...

Comment 69

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

(In reply to Dão Gottwald [:dao] from comment #68)
> Comment on attachment 8366942 [details] [diff] [review]
> Patch v2
> 
> > function updateTitlebarDisplay() {
> >-  document.getElementById("titlebar").hidden = !TabsInTitlebar.enabled;
> >+
> >+#ifdef XP_MACOSX
> >+  if (TabsInTitlebar.enabled) {
> >+    document.documentElement.setAttribute("chromemargin-default", "0,-1,-1,-1");
> >+    document.documentElement.setAttribute("chromemargin", "0,-1,-1,-1");
> >+  } else {
> >+    document.documentElement.setAttribute("chromemargin-default", "");
> >+    let isCustomizing = document.documentElement.hasAttribute("customizing");
> >+    if (!LightweightThemeManager.currentTheme || isCustomizing) {
> >+      document.documentElement.removeAttribute("chromemargin");
> >+    }
> >+  }
> 
> If I read this part correctly, you're still removing the window title in
> popups with a lightweight theme applied, which is not an acceptable state to
> be in as explained in comment 52. We need to get the basics right. In this
> case this means we need to expose the content title somewhere. If we need to
> make a tradeoff here, then lightweight themes being drawn in the title bar
> in popup windows loses since it's not basic functionality, but fine for to a
> followup bug.

Ah, good eye.

Yes, with this patch, we fail to draw the title in the titlebar with lightweight themes. This was something I was going to fix in a follow-up (with bug 888615 landed, this should be pretty straight-forward), but would you prefer to see the fix included in this bug instead of a follow-up?

Comment 70

[Dão Gottwald [:dao]]

Yes, I'd prefer that fix to be included here.

Comment 71

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

Attachment: Patch v2.1 (r+'d by Gijs)

Ok, this puts the title up into the titlebar if we're not drawing the tabs up there.

Comment 72

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

Comment on attachment 8367407 [details] [diff] [review]
Patch v2.1 (r+'d by Gijs)

Adding Unfocused as well. Either or - just need someone from the toolkit module to thumbs up.

Comment 73

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8367407 [details] [diff] [review]
Patch v2.1 (r+'d by Gijs)

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

I'm not so familiar with this aspect of LightwightThemeConsumer - I'd feel far better to have Dao look at this.

Comment 74

[Dão Gottwald [:dao]]

Comment on attachment 8367407 [details] [diff] [review]
Patch v2.1 (r+'d by Gijs)

>+#ifdef XP_MACOSX
>+XPCOMUtils.defineLazyModuleGetter(this, "LightweightThemeManager",
>+  "resource://gre/modules/LightweightThemeManager.jsm");
>+#endif
>+

remove this ...

>+    document.documentElement.setAttribute("chromemargin-default", "");
>+    let isCustomizing = document.documentElement.hasAttribute("customizing");
>+    if (!LightweightThemeManager.currentTheme || isCustomizing) {

... and just replace LightweightThemeManager.currentTheme with document.documentElement.hasAttribute("lwtheme")).

Please add a comment explaining why you're setting chromemargin-default to "" rather than removing it (or even better, remove the need for that hack).

>--- a/browser/themes/osx/browser.css
>+++ b/browser/themes/osx/browser.css
>@@ -6,16 +6,17 @@
> 
> %include shared.inc
> %filter substitution
> %define forwardTransitionLength 150ms
> %define conditionalForwardWithUrlbar window:not([chromehidden~="toolbar"]) #urlbar-container
> %define conditionalForwardWithUrlbarWidth 27
> %define spaceAboveTabbar 9px
> %define toolbarButtonPressed :hover:active:not([disabled="true"]):not([cui-areatype="menu-panel"])
>+%define titlebarHeight 22px

> #main-window[chromehidden~="toolbar"] > #titlebar {
>-  padding-top: 22px;
>+  min-height: @titlebarHeight@;
>+}

Since you use this value only once, you don't need to %define it.

>+#main-window:not([tabsintitlebar]):not(:-moz-lwtheme) > #titlebar {
>+  display: none;
> }

Can this (and the equivalent rule you're adding in browser/themes/windows/browser.css) move to browser/base/content/browser.css with #main-window:not([chromemargin]) > #titlebar as the selector?

>+#main-window[tabsintitlebar]:not([chromehidden~="toolbar"]):not([customize-entered]) > #titlebar {
>   padding-top: @spaceAboveTabbar@;
>   min-height: @tabHeight@;
> }

:not([chromehidden~="toolbar"]) seems redundant here, since it's implied by [tabsintitlebar] (except when it's not, in which case :not([chromehidden~="toolbar"]) is actually wrong here).

Comment 75

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

Attachment: Patch v2.2 (r+'d by Gijs)

(In reply to Dão Gottwald [:dao] from comment #74)
> Comment on attachment 8367407 [details] [diff] [review]
> Patch v2.1 (r+'d by Gijs)
> 
> >+#ifdef XP_MACOSX
> >+XPCOMUtils.defineLazyModuleGetter(this, "LightweightThemeManager",
> >+  "resource://gre/modules/LightweightThemeManager.jsm");
> >+#endif
> >+
> 
> remove this ...
> 
> >+    document.documentElement.setAttribute("chromemargin-default", "");
> >+    let isCustomizing = document.documentElement.hasAttribute("customizing");
> >+    if (!LightweightThemeManager.currentTheme || isCustomizing) {
> 
> ... and just replace LightweightThemeManager.currentTheme with
> document.documentElement.hasAttribute("lwtheme")).
> 

Oh yes, that's much better, thank you!


> Please add a comment explaining why you're setting chromemargin-default to
> "" rather than removing it (or even better, remove the need for that hack).
> 

Yeah, it turns out that hack isn't necessary. Removing the attribute works just fine.

> >--- a/browser/themes/osx/browser.css
> >+++ b/browser/themes/osx/browser.css
> >@@ -6,16 +6,17 @@
> > 
> > %include shared.inc
> > %filter substitution
> > %define forwardTransitionLength 150ms
> > %define conditionalForwardWithUrlbar window:not([chromehidden~="toolbar"]) #urlbar-container
> > %define conditionalForwardWithUrlbarWidth 27
> > %define spaceAboveTabbar 9px
> > %define toolbarButtonPressed :hover:active:not([disabled="true"]):not([cui-areatype="menu-panel"])
> >+%define titlebarHeight 22px
> 
> > #main-window[chromehidden~="toolbar"] > #titlebar {
> >-  padding-top: 22px;
> >+  min-height: @titlebarHeight@;
> >+}
> 
> Since you use this value only once, you don't need to %define it.
> 

Ok, removed.

> >+#main-window:not([tabsintitlebar]):not(:-moz-lwtheme) > #titlebar {
> >+  display: none;
> > }
> 
> Can this (and the equivalent rule you're adding in
> browser/themes/windows/browser.css) move to browser/base/content/browser.css
> with #main-window:not([chromemargin]) > #titlebar as the selector?
> 

Ah, yes, that works quite well, thanks for that.

> >+#main-window[tabsintitlebar]:not([chromehidden~="toolbar"]):not([customize-entered]) > #titlebar {
> >   padding-top: @spaceAboveTabbar@;
> >   min-height: @tabHeight@;
> > }
> 
> :not([chromehidden~="toolbar"]) seems redundant here, since it's implied by
> [tabsintitlebar] (except when it's not, in which case
> :not([chromehidden~="toolbar"]) is actually wrong here).

Done.

Comment 76

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

Comment on attachment 8368558 [details] [diff] [review]
Patch v2.2 (r+'d by Gijs)

How's this?

Comment 77

[Dão Gottwald [:dao]]

Comment on attachment 8368558 [details] [diff] [review]
Patch v2.2 (r+'d by Gijs)

>     var root = this._doc.documentElement;
>     var active = !!aData.headerURL;
> 
>+    let stateChanging = (active != this._active);

nit: remove the empty line and use either var or let for all three declarations.

Please rename chromemargin-default to chromemargin-nonlwtheme or some such to better express its purpose.

r=me with that

Comment 78

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

Attachment: Patch v2.3 (r+'d by Gijs, dao)

Done! I went with "let" because var can die in a fire, and I went with chromemargin-nonlwtheme as per your suggestion.

Thanks Dao!

Comment 79

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

Bah, so I just landed bug 962677, and that patch is interfering slightly with this one - it's causing the titlebar to go wonky if we toggle drawInTitlebar while in customize mode.

Give me a few minutes to sort that out.

Comment 80

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

Ugh, and by a few minutes, I mean a few hours. Our TabsInTitlebar stuff is maddening and super-touchy. :(

Comment 81

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

Attachment: Follow-up fix

This patch is to be applied on top of Patch v2.3.

With this patch, disabling and re-enabling tabs-in-titlebar in customize mode now works as expected, with or without a lightweight theme.

The trick here was to realize that there's some stuff we were calculating that's really unnecessary to calculate - specifically, the OS X specific margins for the titlebar-content element. This can be done in CSS alone, since there's no built-in way for OS X users to increase the font / size of their titlebars.

I'm just going to make sure this didn't regress anything on Windows or Linux and then I'll request review.

Comment 82

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

Comment on attachment 8369536 [details] [diff] [review]
Follow-up fix

One thing that I've noticed is that pop-up windows with lightweight themes have a slightly smaller titlebar than we probably want, but I think that's something we could fix in a follow-up.

This appears to fix things on OS X and doesn't regress Windows. Unable to test on Ubuntu Linux due to build issues. :/

Comment 83

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

Was able to resolve my build issues, and things seem to work properly on Ubuntu (as expected, since we don't draw in the titlebar there).

Comment 84

[:Gijs (he/him)]

Comment on attachment 8369536 [details] [diff] [review]
Follow-up fix

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

::: browser/themes/osx/browser.css
@@ +4019,5 @@
>    padding-top: 0;
>  }
>  
> +#main-window[tabsintitlebar]:not([customizing]):not(:-moz-lwtheme) > #titlebar > #titlebar-content,
> +#main-window[tabsintitlebar]:-moz-any([customize-entering],[customize-exiting]) > #titlebar > #titlebar-content {

These confuse me. For one, I guess they depend on the size of the titlebar elements, which I believe aren't the same across different versions of OS X. :-(

For another, it seems like these could be unified:

#main-window[tabsintitlebar]:-moz-any(:not([customizing]):not(:-moz-lwtheme), [customize-entering],[customize-exiting]) > #titlebar > #titlebar-content

Or, alternatively, split up (to avoid the :-moz-any). I'm not sure I understand the current split. :-)

@@ +4030,5 @@
> +  margin-top: 11px;
> +  margin-bottom: 0px;
> +}
> +
> +#main-window[tabsintitlebar]:-moz-lwtheme > #titlebar > #titlebar-content {

As far as I can tell there are no rules here about non-tabsintitlebar-non-customizing + lwtheme. How does that work?

Comment 85

[:Gijs (he/him)]

Comment on attachment 8369536 [details] [diff] [review]
Follow-up fix

Clearing this while Mike looks at 10.6.

Comment 86

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

(In reply to :Gijs Kruitbosch from comment #84)
> Comment on attachment 8369536 [details] [diff] [review]
> Follow-up fix
> 
> Review of attachment 8369536 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/themes/osx/browser.css
> @@ +4019,5 @@
> >    padding-top: 0;
> >  }
> >  
> > +#main-window[tabsintitlebar]:not([customizing]):not(:-moz-lwtheme) > #titlebar > #titlebar-content,
> > +#main-window[tabsintitlebar]:-moz-any([customize-entering],[customize-exiting]) > #titlebar > #titlebar-content {
> 
> These confuse me. For one, I guess they depend on the size of the titlebar
> elements, which I believe aren't the same across different versions of OS X.
> :-(

Tested on Mountain Lion, and we're still in the green. While it's true that the standard window buttons changed in size, the titlebar itself remained the same, so I think we're operating correctly here.

> 
> For another, it seems like these could be unified:
> 
> #main-window[tabsintitlebar]:-moz-any(:not([customizing]):not(:-moz-lwtheme),
> [customize-entering],[customize-exiting]) > #titlebar > #titlebar-content
> 
> Or, alternatively, split up (to avoid the :-moz-any). I'm not sure I
> understand the current split. :-)
> 

I'll take a look to see if I can combine those.

> @@ +4030,5 @@
> > +  margin-top: 11px;
> > +  margin-bottom: 0px;
> > +}
> > +
> > +#main-window[tabsintitlebar]:-moz-lwtheme > #titlebar > #titlebar-content {
> 
> As far as I can tell there are no rules here about
> non-tabsintitlebar-non-customizing + lwtheme. How does that work?

Correct. That case is still a bit buggy - for example, I believe the tabs are too close to the standard window buttons and full screen button in that configuration. Pop up windows make this more evident.

I would, however, like to deal with lw-theme issues in a separate follow-up if at all possible.

Comment 87

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

Attachment: Follow-up fix v2

Ok, split up that rule into 3. Tested pretty thoroughly - I feel reasonably confident about this.

Comment 88

[:Gijs (he/him)]

Comment on attachment 8370309 [details] [diff] [review]
Follow-up fix v2

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

Alright. Please file a followup for the lwtheme stuff, which should hopefully be pretty self-contained.

Comment 89

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

Attachment: Patch v2.4 (r+'d by Gijs, dao)

Ok, thanks, folded.

Comment 90

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

Comment on attachment 8370317 [details] [diff] [review]
Patch v2.4 (r+'d by Gijs, dao)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 

Australis.


User impact if declined: 

Users who attempt to disable the tabs in titlebar while customizing might find their nav-bar jammed into their tab strip, or other titlebar glitches. Popup windows might have the nav-bar in the titlebar.


Testing completed (on m-c, etc.): 

Tested the following scenarios on m-c:

-Tabs in titlebar enabled (with lw-theme, without lw-theme, entered and exited customize mode)
-Tabs in titlebar disabled (with lw-theme, without lw-theme, entered and exited customize mode)
-Popup window (with lw-theme, without lw-theme, with tabs in titlebar, without tabs in titlebar)


Risk to taking this patch (and alternatives if risky): 

Medium risk of introducing other titlebar regressions, but as it currently stands, landing this is probably way better than keeping what we've currently got.


String or IDL/UUID changes made by this patch:

None.

Comment 91

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

remote:   https://hg.mozilla.org/integration/fx-team/rev/cbe26a93c79b

Comment 92

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

Comment on attachment 8370317 [details] [diff] [review]
Patch v2.4 (r+'d by Gijs, dao)

Spoke too soon (Gijs was right) - I seem to have introduced a non-customize mode orange here, at least on 10.6:

14:59:56  WARNING -  TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/general/browser_tabbar_big_widgets.js | Titlebar should have grown
14:59:56     INFO -  Stack trace:
14:59:56     INFO -      JS frame :: chrome://mochitests/content/browser/browser/base/content/test/general/browser_tabbar_big_widgets.js :: test :: line 17
14:59:56     INFO -      JS frame :: chrome://mochikit/content/browser-test.js :: Tester_execTest :: line 583
14:59:56     INFO -      JS frame :: chrome://mochikit/content/browser-test.js :: Tester_nextTest/< :: line 481
14:59:56     INFO -      native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0

Backing out...

Comment 93

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

Backed out:

remote:   https://hg.mozilla.org/integration/fx-team/rev/a77d5593a3fc

Comment 94

[:Gijs (he/him)]

(In reply to Mike Conley (:mconley) from comment #93)
> Backed out:
> 
> remote:   https://hg.mozilla.org/integration/fx-team/rev/a77d5593a3fc

This issue is much more prominent than the issues caused by having high widgets not alter the titlebar size. r=me to reland with the test disabled.

Comment 95

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

Attachment: Patch v2.5 (r+'d by Gijs, dao)

Thanks Gijs - disabling the test on OS X, and filed bug 967917.

Comment 96

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

Relanded:

remote:   https://hg.mozilla.org/integration/fx-team/rev/8a56937c985b

Comment 97

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8a56937c985b

Comment 98

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

Comment on attachment 8370431 [details] [diff] [review]
Patch v2.5 (r+'d by Gijs, dao)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 

Australis.


User impact if declined: 

Users who attempt to disable the tabs in titlebar while customizing might find their nav-bar jammed into their tab strip, or other titlebar glitches. Popup windows might have the nav-bar in the titlebar.


Testing completed (on m-c, etc.): 

Tested the following scenarios on m-c:

-Tabs in titlebar enabled (with lw-theme, without lw-theme, entered and exited customize mode)
-Tabs in titlebar disabled (with lw-theme, without lw-theme, entered and exited customize mode)
-Popup window (with lw-theme, without lw-theme, with tabs in titlebar, without tabs in titlebar)


Risk to taking this patch (and alternatives if risky): 

Medium risk of introducing other titlebar regressions, but as it currently stands, landing this is probably way better than keeping what we've currently got.


String or IDL/UUID changes made by this patch:

None.

Comment 99

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

remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/6b8cf58d44c7

Comment 100

[Cornel Ionce [:noni] [Hubs QA]]

Tested the scenarios from comment 98 and issue is no longer reproducing on latest Nightly (build ID:20140306030201) and latest Aurora (build ID: 20140306004001).
More exploratory testing will be performed and any regressions found will be logged.
Marking as verified.