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

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?

(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.

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.

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.

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

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.

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.

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.

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

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?

(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.

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.

Has anyone seen this on a non-Retina display?

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

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

(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.

(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.

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.

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

(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.

(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.

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.

Forgot to add: browser window should NOT be maximized.

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.

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 on attachment 8333954 [details] [diff] [review] 930094-v1.diff Gijs, what do you think of this?

Comment on attachment 8333954 [details] [diff] [review] 930094-v1.diff This broke browser.tabs.drawInTitlebar = false.

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 on attachment 8334174 [details] [diff] [review] Patch v1.1 Seems to work fine for OS X and Windows. What do you think, Gijs?

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?

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 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 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.

(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?

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

(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.

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

(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.

(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.

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?

(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

(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.

(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?

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

(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...

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.

(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?

(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?

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

(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.

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

(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.

This sounds like Bug 573974 -- Remember the presence of the drawintitlebar attribute in LightweightThemeConsumer.jsm should be forward-duped or something...?

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?

(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.

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.

Whoops, for real this time.

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 on attachment 8366731 [details] [diff] [review] Patch v2 Yikes - it looks like "" is not a valid value for chromemargin. Fixing...

Gotta go test this on Windows and Linux.

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 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 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...

(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?

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

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

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 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 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).

(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 on attachment 8368558 [details] [diff] [review] Patch v2.2 (r+'d by Gijs) How's this?

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

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

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.

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

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 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. :/

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 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 on attachment 8369536 [details] [diff] [review] Follow-up fix Clearing this while Mike looks at 10.6.

(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.

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

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.

Ok, thanks, folded.

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.

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

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...

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

(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.

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

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

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.

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

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.