768516 - Thunderbird tabs in the title bar on OS X

Status: RESOLVED FIXED

(Thunderbird :: Theme, defect)

Description

[Andreas Nilsson (:andreasn)]

This is the pinstripe counterpart of bug #755793.

Mockup:
http://24.media.tumblr.com/tumblr_m0it9bMzM51qkoea4o1_1280.png

Comment 1

[Richard Marti (:Paenglab)]

Attachment: WIP

This patch implements the changes from bug 647216 and following.

Comment 2

[Richard Marti (:Paenglab)]

Attachment: WIP v2

WIP patch which draws the tabs in titlebar. Needs the patch from Bug 625989.

It has some issues:
- captions not positionable
- titlebar text always visible
- some problems in dimension calculation
- no sleek transition to fullscreen and back

Comment 3

[Richard Marti (:Paenglab)]

Attachment: WIP v2 patch in action

Comment 4

[Josiah Bruner [:jsbruner]]

Attachment: Tabs In Titlebar V 1

Thanks Richard! Your previous patch was a big help.

Patch adds tabs in titlebar ability on the current version of TB. Besides updating for the current trunk, some changes have been made:

A. The extra space that makes way for the caption buttons I extended to 60px, instead of the previous 55px. 60px makes it less cramped when the buttons are moved down.

B. The all-tabs button I moved up to align itself with the re-positioned fullscreen button. Also, the button's default opacity is .8 now, and on hover is is 1 to match the fullscreen button.

Note: The moved standardWindowButton position change is happening in bug 851652. However, since work is still progressing there to find a better way to accomplish that, I am using the old implementation at the bug.

Link to the attachment: https://bugzilla.mozilla.org/attachment.cgi?id=731984

Once that bug is finished, it will land in a disabled state until Australis has landed. Since that is unnecessary here, my goal is to land this in a disabled state as soon as possible in a disabled state. Then, when the standardWindowButton patch lands, I will push another patch that enables the new TB tabs.

This way we won't have to wait for Australis, because you know, we already are Australis.

There are still some issues with this patch that I will address in the coming days, namely:

A. Fullscreen entrance/exit is still choppy.

B. See the next patch I will upload momentarily.

Comment 5

[Josiah Bruner [:jsbruner]]

Attachment: Remove the title

This patch removes the title from the frame. But there is one potential issue. Right now, the "Daily" text shows up during launch. This is actually not too bad, and goes away pretty quickly, but may need to get fixed.

Comment 6

[Josiah Bruner [:jsbruner]]

Attachment: Screenshot of the new style

Screenshot of all three patches applied.

Comment 7

[Josiah Bruner [:jsbruner]]

Attachment: Screenshot of the new style

Err... I'm assuming people don't want to compile that before seeing the change. :)

Comment 8

[Josiah Bruner [:jsbruner]]

I was hoping to have Try builds up my tomorrow morning, but it appears that isn't happening. Having some build troubles...

Anyway, there should be Try Builds sometime Sunday for sure.

Comment 9

[Josiah Bruner [:jsbruner]]

On second thought: https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=196bbf3e791c

Building...

Comment 10

[Josiah Bruner [:jsbruner]]

Well, that didn't go over well...

For the sake of simplicity and in order to keep my head on, I have uploaded an OSX 64 build. Download is available here:

https://sites.google.com/site/josiahsbruner/files/Daily.app.zip?attredirects=0&d=1

Comment 11

[Richard Marti (:Paenglab)]

(In reply to Josiah Bruner [:JosiahOne] from comment #10)
> Well, that didn't go over well...
> 
> For the sake of simplicity and in order to keep my head on, I have uploaded
> an OSX 64 build. Download is available here:
> 
> https://sites.google.com/site/josiahsbruner/files/Daily.app.
> zip?attredirects=0&d=1

This doesn't work, where are a lot of files with link to your local filesystem. Please can you try to do a 'make package' and upload then this dmg file?

Comment 12

[Josiah Bruner [:jsbruner]]

(In reply to Richard Marti [:Paenglab] from comment #11)
> (In reply to Josiah Bruner [:JosiahOne] from comment #10)
> > Well, that didn't go over well...
> > 
> > For the sake of simplicity and in order to keep my head on, I have uploaded
> > an OSX 64 build. Download is available here:
> > 
> > https://sites.google.com/site/josiahsbruner/files/Daily.app.
> > zip?attredirects=0&d=1
> 
> This doesn't work, where are a lot of files with link to your local
> filesystem. Please can you try to do a 'make package' and upload then this
> dmg file?

Hmm... Alright. Sorry for my ignorance, but how does one go about doing a 'make package'?

I tried a whole bunch of things, but none of them have worked.

Comment 13

[Josiah Bruner [:jsbruner]]

Never mind. I got it, you needed to do it in your object directory.

Comment 14

[Josiah Bruner [:jsbruner]]

Attachment: REm

Comment 15

[Josiah Bruner [:jsbruner]]

Attachment: Remove the title

Umm... The previous upload was an accident.

Updated for current trunk.

Builds uploading, should be available soon.

Comment 16

[Josiah Bruner [:jsbruner]]

http://db.tt/gVGg8ByM

Link above links to the dmg. Sorry for the wait, it took forever to upload.

Comment 17

[Richard Marti (:Paenglab)]

Attachment: Tabs in Titlebar V 2

Josiah, thanks for the dmg, it's looking great. I made some slight changes:
- Removed the messengerWindow's padding-top when a lw.theme is shown.
- Moved also the tabbar-toolbar some pixels up to align with .tabs-alltabs-button.
- Instead of padding-bottom I used margin-bottom to move the .tabs-alltabs-button
  up. And it's only moved up when not in fullscreenmode.
- Instead of margin-right: 5px for .tabs-alltabs-button, I gave the tabs-toolbar
  5px more padding. Then in the future when the .tabs-alltabs-button is hidden
  with only one tab, the tabbar-toolbar icons are on the same position.

Comment 18

[Josiah Bruner [:jsbruner]]

Attachment: Allow OS X to draw in the title - mozilla code change

I had to make some changes to Mike Conley's Tabs in title patch. I changed the chrome margin code in the LightweightThemeConsumer. Also it is updated for the current trunk now.

Mike, is there something I did in this change that should not happen. Essentially I removed the ability to ever completely remove the chromemargin property. This fixes the TB error, and has no obvious side effects, but if this is incorrect let me know.

Comment 19

[Josiah Bruner [:jsbruner]]

Attachment: Tabs In Titlebar V 3

Updated Tabs in Titlebar patch. Fixes a bug with lwthemes and the address book. all-tabs button's opacity is now .7 to match the fullscreen button almost exactly.

All that is left is figuring out how to smooth the transition.

Comment 20

[Josiah Bruner [:jsbruner]]

Attachment: Screenshot of the new style

Comment 21

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

Comment on attachment 738468 [details] [diff] [review]
Allow OS X to draw in the title - mozilla code change

Hey Josiah,

What I need here is the difference you need to apply to UX in order to get the right result for TB. You said you had to make a change to LightweightConsumer - can I see the diff for that change?

-Mike

Comment 22

[Josiah Bruner [:jsbruner]]

Attachment: Fix DrawInTitlebar for TB.

Clearing Tabs In Titlebar patch.

Now only the patch from bug 625989 is necessary, in combination with the three patches attached to this bug. I updated the patch on bug 625989 for the current trunk thereby not needing the TB-specic Tabs in titlebar patch.

Instead, I have simply created a fix for TB. However, this probably is not what we want the final implementation to be for fixing this error. Although this fixes the issue, with no visible side effects, I am asking Mike Conley for feedback on this, as I really need more details on what chromemargin is suppose to do and what changes (if any) need to be made here to fix this problem so that it does not have any potential consequences.

Comment 23

[Josiah Bruner [:jsbruner]]

Comment on attachment 738471 [details] [diff] [review]
Tabs In Titlebar V 3

Don't know why I obsoleted this patch, but it is necessary.

Comment 24

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

Comment on attachment 751531 [details] [diff] [review]
Fix DrawInTitlebar for TB.

The chromemargin attribute is set on the root element of a window, and is used to set whether or not the OS chrome should be displayed on particular edges of the window.

Please see https://developer.mozilla.org/en-US/docs/XUL/Attribute/chromemargin

I don't think we want to remove these lines as you've indicated because in the event that no chromemargin was set on the window before a lw-theme was activated, we want to remove that attribute when the lw-theme is removed.

Comment 25

[Josiah Bruner [:jsbruner]]

Attachment: Tabs In Titlebar V 3.1

Updated patch. Now most other patches are obsolete. Now required to use:

Tabs in Titlebar V3
Remove the title
Move buttons down

The chromemargin error seems to have been fixed. Unfortunately there is now a new bug I must address that seems unrelated to that one.

Comment 26

[Josiah Bruner [:jsbruner]]

Attachment: Fix lightweight themes removing drawInTitlebar property.

Currently the lightweight theme manager clears the drawInTitlebar property of our window when lw-themes are deactivated (Since drawInTitlebar hasn't been used for anything else) Now however, we require drawInTitlebar to stay in-tact.

Mike, could you review this? I am also sort of curious on why this isn't part of the patch in bug 625989. Perhaps your patch there overrides this change. Either way it would be much cleaner and safer to always use drawInTitlebar here.

Comment 27

[Josiah Bruner [:jsbruner]]

Comment on attachment 773046 [details] [diff] [review]
Fix lightweight themes removing drawInTitlebar property.

I take it back, there are quite a few problems with this. I'm going to think about this some more...

Comment 28

[Josiah Bruner [:jsbruner]]

Attachment: Main Patch.

Got a fix for the lw-theme issue.

Comment 29

[Josiah Bruner [:jsbruner]]

Attachment: Main Patch.

Fix for lw-themes is now Mac only, as it should be.

Comment 30

[Josiah Bruner [:jsbruner]]

Attachment: Main Patch.

Here's the patch that implements the ability to move the tabs in the titlebar. Also setting the tabsintitlebar property on messengerWindow to false will allow disabling the feature. I will write a patch that sets that property to false by default until I have a patch that moves the caption buttons down.

Mike, could you please review this? And would you also move this up in your queue a little. This is currently causing a pretty big problem now (Bug 941740).

Comment 31

[Josiah Bruner [:jsbruner]]

Attachment: Main Patch.

margin-bottom should've been 0px, not none. Fixed.

Comment 32

[Josiah Bruner [:jsbruner]]

Attachment: Main Patch.

Actually, to make this allowed to be disabled via an about:config pref, I fixed some CSS enabling that.

Comment 33

[Josiah Bruner [:jsbruner]]

Attachment: Move Window Controls down.

Here's the patch to move the window controls downward.

Comment 34

[Richard Marti (:Paenglab)]

Comment on attachment 8337472 [details] [diff] [review]
Move Window Controls down.

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

Not tested yet but seen this.

::: mail/themes/osx/mail/messenger.css
@@ +210,5 @@
>  #messengerWindow[drawintitlebar="true"]:not(:-moz-lwtheme) > #titlebar {
>    -moz-appearance: -moz-window-titlebar;
>  }
> +
> +#messengerWindow:not([drawintitlebar=true]) > #titlebar > #titlebar-content >

nit: sometimes you have set quotes, sometime not.

@@ +230,5 @@
> +
> +#messengerWindow[drawintitlebar="true"] > #titlebar > #titlebar-content >
> +#titlebar-fullscreen-button {
> +  -moz-appearance: -moz-mac-fullscreen-button;
> +  margin-top 11px;

colon missing.

Comment 35

[Josiah Bruner [:jsbruner]]

Attachment: Move Window Controls down.

Thanks Richard! Fixed.

Comment 36

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

So I'm not so much worried about the code so much as I'm worried about timing.

Australis will not be riding the 28 train, which is why we're maintaining a backout branch (Holly) that we'll be merging in to Aurora instead of mozilla-central.

Which means that if we land this now, it's going to be busted for Earlybird users, unless we maintain our own backout branch, which probably isn't ideal.

So this is a little hairy.

What I suggest is that we hold off on putting the tabs into the titlebar until we have a high-degree of confidence that Australis is going to ride a particular train. Until then, I think we should produce minimal patches to fix any regressions Australis causes (such as jamming all of the toolbars into the titlebar), but then back those patches out after the uplift.

Any concerns with that?

Comment 37

[Josiah Bruner [:jsbruner]]

Comment on attachment 8337197 [details] [diff] [review]
Main Patch.

Clearing review flags until after Australis re-lands.

Comment 38

[Josiah Bruner [:jsbruner]]

Attachment: Main Patch.

Here's a simpler version of the patch that must be applied on top of the one in bug 953204.

Comment 39

[Josiah Bruner [:jsbruner]]

Oh, currently you must toggle drawInTitlebar to "true" in about:config to get this to work.

This should work with drawInTitlebar disabled as well for people who don't like it.

Comment 40

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

Comment on attachment 8398982 [details] [diff] [review]
Main Patch.

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

This is good stuff, and almost landable. Just a few issues that aren't too serious.

::: mail/base/content/msgMail3PaneWindow.js
@@ +1857,4 @@
>          extraMargin += tabmailMarginTop;
>          // On non-OSX, we can just use bottom margin:
>          titlebarContent.style.marginBottom = extraMargin + "px";
> +        // On OS X, we need to add 8 extra pixels to leave a top gap.

We do this a bit differently with Firefox - the extraMargin isn't applied to the marginBottom at all. Instead, line 1859 is wrapped in an #ifndef XP_MACOSX, and the gap is hardcoded into the CSS.

We might get a bit smarter with that if / when we find a solution for bug 967917. For now though, we probably want parity or better.

::: mail/themes/osx/mail/messenger.css
@@ +16,3 @@
>  }
>  
> +#messengerWindow:not([tabsintitlebar="true"]) > 

Nit - trailing ws.

@@ +135,5 @@
> +#messengerWindow[tabsintitlebar="true"]:not(:-moz-lwtheme) > #titlebar {
> +  -moz-appearance: -moz-window-titlebar;
> +}
> +
> +#messengerWindow[tabsintitlebar="true"]:not([sizemode="fullscreen"]) >

This isn't going to work for RTL because when using RTL on OS X, the window buttons stay in the same position instead of flipping around.

Instead of using these paddings, I'd suggest not hiding the titlebar-placeholders, and taking advantage of _sizePlaceholder in msgMail3PaneWindow.js's TabsInTitlebar instead. That'll sample the width of the caption and fullscreen buttons, and size the titlebar-placeholders instead. You'll need to modify msgMail3PaneWindow.js to size the fullscreen button placeholder for you behind an XP_MACOSX #ifdef, but it should be pretty straightforward. Ask me if you need help.

@@ +156,5 @@
> +  position: relative;
> +  margin-top: 11px;
> +  margin-bottom: 2px;
> +}
> +

We're also going to want to set pointer-events: none on the #titlebar-spacer, otherwise buttons customized into the right half of the tab toolbar won't be clickable in their upper-half.

::: mail/themes/osx/mail/tabmail.css
@@ +249,5 @@
>    margin: 0;
> +  opacity: .7;
> +}
> +
> +.tabs-alltabs-box:hover {

Wait... .tabs-alltabs-box doesn't exist in mail/'s tabmail-tabs binding. It exists in suite's though. This .tabs-alltabs-box can probably go.

Comment 41

[Josiah Bruner [:jsbruner]]

Attachment: Patch.

Wow, so actually I had to rework quite a bit more than I thought. Sadly after I applied your suggestions I noticed that it didn't work properly with lw-themes, and had to port even more code from Fx, plus more CSS to make up for that.

However, I think things work well now, so hopefully we can get this in TB 31.

Comment 42

[Richard Marti (:Paenglab)]

Attachment: patch

I fixed in bug 953204 the #ifndef XP_MACOSX to be sure it is in trunk. My only change in this patch is removing this part.

Comment 43

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

Comment on attachment 8413597 [details] [diff] [review]
patch

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

Tentative r=me, but I think the 0px sizePlaceholders can probably be removed. See my notes below. Thanks Richard!

::: mail/base/content/msgMail3PaneWindow.js
@@ +1927,5 @@
>        titlebarContent.style.marginBottom = "";
>        titlebar.style.marginBottom = "";
>        menubar.style.paddingBottom = "";
> +
> +#ifdef XP_MACOSX

No need to set these to 0 if they're already display: none'd from not having tabs in the titlebar... or is there a case I'm missing?

::: mail/themes/osx/mail/messenger.css
@@ +186,5 @@
> +#titlebar-buttonbox {
> +  -moz-appearance: -moz-window-button-box;
> +}
> +
> +#titlebar-fullscreen-button {

This should also probably be under the -moz-mac-lion-theme @media query.

Comment 44

[Josiah Bruner [:jsbruner]]

Attachment: Patch.

Addressed feedback. Thanks Mike!

Comment 45

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/26c3baf7b556

Comment 46

[Josiah Bruner [:jsbruner]]

Comment on attachment 8414146 [details] [diff] [review]
Patch.

[Approval Request Comment]
Regression caused by (bug #): None
User impact if declined: Users will still not be able to put the tabs in the titlebar for TB 31.
Testing completed (on c-c, etc.): On C-C, no test failures.
Risk to taking this patch (and alternatives if risky): Not terribly risky, however it is possible that some titlebar-related regressions might surface, but that is unlikely. Also such fixes should be able to be addressed with simple CSS.

Comment 47

[Mark Banner (:standard8)]

https://hg.mozilla.org/releases/comm-aurora/rev/01be4135cb03