Thunderbird tabs in the title bar on OS X

RESOLVED FIXED in Thunderbird 32.0

Status

defect
RESOLVED FIXED
7 years ago
4 years ago

People

(Reporter: andreasn, Assigned: jsbruner)

Tracking

Trunk
Thunderbird 32.0
x86_64
macOS
Dependency tree / graph
Bug Flags:
in-testsuite -

Thunderbird Tracking Flags

(thunderbird31 fixed)

Details

(Whiteboard: [ux-feature-wanted-31])

Attachments

(2 attachments, 25 obsolete attachments)

429.31 KB, image/png
Details
7.78 KB, patch
jsbruner
: review+
Details | Diff | Splinter Review
Blocks: 733856
Depends on: 625989
Posted patch WIP (obsolete) β€” β€” Splinter Review
This patch implements the changes from bug 647216 and following.
Posted patch WIP v2 (obsolete) β€” β€” Splinter Review
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
Attachment #712098 - Attachment is obsolete: true
Posted image WIP v2 patch in action (obsolete) β€”
Posted patch Tabs In Titlebar V 1 (obsolete) β€” β€” Splinter Review
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.
Attachment #712993 - Attachment is obsolete: true
Posted patch Remove the title (obsolete) β€” β€” Splinter Review
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.
Assignee: nobody → josiah
Status: NEW → ASSIGNED
Posted patch Screenshot of the new style (obsolete) β€” β€” Splinter Review
Screenshot of all three patches applied.
Attachment #712994 - Attachment is obsolete: true
Posted image Screenshot of the new style (obsolete) β€”
Err... I'm assuming people don't want to compile that before seeing the change. :)
Attachment #736950 - Attachment is obsolete: true
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.
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
(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?
(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.
Never mind. I got it, you needed to do it in your object directory.
Posted file REm (obsolete) β€”
Posted patch Remove the title (obsolete) β€” β€” Splinter Review
Umm... The previous upload was an accident.

Updated for current trunk.

Builds uploading, should be available soon.
Attachment #736947 - Attachment is obsolete: true
Attachment #737291 - Attachment is obsolete: true
http://db.tt/gVGg8ByM

Link above links to the dmg. Sorry for the wait, it took forever to upload.
Posted patch Tabs in Titlebar V 2 (obsolete) β€” β€” Splinter Review
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.
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.
Attachment #738468 - Flags: feedback?(mconley)
Posted patch Tabs In Titlebar V 3 (obsolete) β€” β€” Splinter Review
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.
Attachment #736946 - Attachment is obsolete: true
Attachment #737486 - Attachment is obsolete: true
Attachment #736951 - Attachment is obsolete: true
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
Attachment #738468 - Flags: feedback?(mconley)
Posted patch Fix DrawInTitlebar for TB. (obsolete) β€” β€” Splinter Review
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.
Attachment #738471 - Attachment is obsolete: true
Attachment #751531 - Flags: feedback?(mconley)
Comment on attachment 738471 [details] [diff] [review]
Tabs In Titlebar V 3

Don't know why I obsoleted this patch, but it is necessary.
Attachment #738471 - Attachment is obsolete: false
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.
Attachment #751531 - Flags: feedback?(mconley) → feedback-
Posted patch Tabs In Titlebar V 3.1 (obsolete) β€” β€” Splinter Review
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.
Attachment #738468 - Attachment is obsolete: true
Attachment #738471 - Attachment is obsolete: true
Attachment #751531 - Attachment is obsolete: true
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.
Attachment #773046 - Flags: review?(mconley)
Summary: Allow to draw in the titlebar for Australis (Pinstripe) → Thunderbird tabs in the title bar on OS X
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...
Attachment #773046 - Flags: review?(mconley)
Posted patch Main Patch. (obsolete) β€” β€” Splinter Review
Got a fix for the lw-theme issue.
Attachment #760432 - Attachment is obsolete: true
Attachment #773046 - Attachment is obsolete: true
Posted patch Main Patch. (obsolete) β€” β€” Splinter Review
Fix for lw-themes is now Mac only, as it should be.
Attachment #773490 - Attachment is obsolete: true
Whiteboard: [ux-feature-wanted-31]
No longer depends on: AustralisBird
Posted patch Main Patch. (obsolete) β€” β€” Splinter Review
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).
Attachment #737292 - Attachment is obsolete: true
Attachment #773513 - Attachment is obsolete: true
Attachment #8337192 - Flags: review?(mconley)
Posted patch Main Patch. (obsolete) β€” β€” Splinter Review
margin-bottom should've been 0px, not none. Fixed.
Attachment #8337192 - Attachment is obsolete: true
Attachment #8337192 - Flags: review?(mconley)
Attachment #8337196 - Flags: review?(mconley)
Posted patch Main Patch. (obsolete) β€” β€” Splinter Review
Actually, to make this allowed to be disabled via an about:config pref, I fixed some CSS enabling that.
Attachment #8337196 - Attachment is obsolete: true
Attachment #8337196 - Flags: review?(mconley)
Attachment #8337197 - Flags: review?(mconley)
Posted patch Move Window Controls down. (obsolete) β€” β€” Splinter Review
Here's the patch to move the window controls downward.
Attachment #8337472 - Flags: review?(mconley)
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.
Posted patch Move Window Controls down. (obsolete) β€” β€” Splinter Review
Thanks Richard! Fixed.
Attachment #8337472 - Attachment is obsolete: true
Attachment #8337472 - Flags: review?(mconley)
Attachment #8337761 - Flags: review?(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 on attachment 8337197 [details] [diff] [review]
Main Patch.

Clearing review flags until after Australis re-lands.
Attachment #8337197 - Flags: review?(mconley)
Attachment #8337761 - Flags: review?(mconley)
Posted patch Main Patch. (obsolete) β€” β€” Splinter Review
Here's a simpler version of the patch that must be applied on top of the one in bug 953204.
Attachment #8337197 - Attachment is obsolete: true
Attachment #8337761 - Attachment is obsolete: true
Attachment #8398982 - Flags: review?(mconley)
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 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.
Attachment #8398982 - Flags: review?(mconley) → feedback+
Posted patch Patch. (obsolete) β€” β€” Splinter Review
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.
Attachment #8398982 - Attachment is obsolete: true
Attachment #8413527 - Flags: review?(mconley)
Posted patch patch (obsolete) β€” β€” Splinter Review
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.
Attachment #8413527 - Attachment is obsolete: true
Attachment #8413527 - Flags: review?(mconley)
Attachment #8413597 - Flags: review?(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.
Attachment #8413597 - Flags: review?(mconley) → review+
Posted patch Patch. β€” β€” Splinter Review
Addressed feedback. Thanks Mike!
Attachment #8413597 - Attachment is obsolete: true
Attachment #8414146 - Flags: review+
https://hg.mozilla.org/comm-central/rev/26c3baf7b556
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 32.0
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.
Attachment #8414146 - Flags: approval-comm-aurora?
Depends on: 1007225
Attachment #8414146 - Flags: approval-comm-aurora? → approval-comm-aurora+
Flags: in-testsuite-
You need to log in before you can comment on or make changes to this bug.