755793 - Allow for drawing in the window titlebar for Australis

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, defect)

Comment 1

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

Whoops, forgot description.

Basically, for Australis we want to be able to draw in the window titlebar, so that we can do away with the window icon / title, and move our tabs into the freed real-estate.

Comment 2

[Andreas Nilsson (:andreasn)]

Mental notes to self (trunk is building):
on window xul:
* chromemargin: 0,2,2,2;
* padding-top: 20px;

#messengerWindow {
  -moz-binding: url("chrome://global/content/bindings/toolbar.xml#toolbar-drag");
}

#tabs-toolbar {
  -moz-binding: url("chrome://global/content/bindings/toolbar.xml#toolbar-drag");
}

Comment 3

[Andreas Nilsson (:andreasn)]

Attachment: first try

Initial stab

Comment 4

[Andreas Nilsson (:andreasn)]

Known issues:
* Personas hides the min-max-close buttons
* Need to skip the top padding when window is maximized.

Comment 5

[Andreas Nilsson (:andreasn)]

If anyone runs this patch DON'T MAXIMIZE.
It's apparently quite hard to get back again.

Comment 6

[Andreas Nilsson (:andreasn)]

(In reply to Andreas Nilsson (:andreasn) from comment #5)
> If anyone runs this patch DON'T MAXIMIZE.
> It's apparently quite hard to get back again.

Actually, false alarm. That was with a newer version of this patch where I had introduced a bunch of randomly exploding things.

Comment 7

[Andreas Nilsson (:andreasn)]

Attachment: patch (v2)

Checkpoint before I start adding in exploding stuff.

Comment 8

[Andreas Nilsson (:andreasn)]

Attachment: patch (v2)

Comment 9

[Andreas Nilsson (:andreasn)]

Attachment: patch (v3)

This fixes the issues with the basic and classic themes so that we now get windows controls for that (only the minimize button works so far though).

Comment 10

[Andreas Nilsson (:andreasn)]

Attachment: patch (v4)

This allows for lightweight themes to work, for some reason currently incompatible with -moz-binding: url("chrome://global/content/bindings/toolbar.xml#toolbar-drag"); so that's commented out for now.

Comment 11

[Andreas Nilsson (:andreasn)]

Attachment: patch (v4.1)

Allow dragging in tabs toolbar and using lwtheme now, but currently don't work for Aero.

Comment 12

[Andreas Nilsson (:andreasn)]

Attachment: patch (v5)

All dragging works now, still some small quirks with the lwtheme window buttons though.

Comment 13

[Andreas Nilsson (:andreasn)]

Attachment: patch (v6)

Just a small update allowing the maximize/resize and close buttons work in Basic under Win7.

Comment 14

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

Paenglab mentioned that we'll want to port Firefox's TabsInTitleBar to Thunderbird, so I'm going to try to do that today.

Comment 15

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

Attachment: Checkpointing work

Checkpointing work, and stashing patch on Bugzilla so I can move it over to my Windows machine.

Comment 16

[Richard Marti (:Paenglab)]

Attachment: patch v7 (needs patch from bug 763308 first applied)

This is a patch with all needed XUL (I added the #ifdef CAN_DRAW_IN_TITLEBAR) and CSS changes. But still with a fixed margin-bottom of -12px for the titlebar. This needs to be dynamic for the different heights on different themes.

Not included in this patch is Mikes patch to calculate the captionButtonsBox width.

Mike, I saw you are also calculating the margin-bottom for the titlebar, right? In shorlander's mockups there is always a space of 16px between the window top and the tabs. Could you please attend this in this calculation? I would say margin-bottom= -(titlebar height - 16px) should do the trick.

Comment 17

[Richard Marti (:Paenglab)]

Attachment: screenshot of actual patch

I know, the tabbar buttons on the right collide with the caption buttons. This is due the missing width calculation of the captionButtonsBox.
Also the space between the window top and the tabs isn't correct.

Comment 18

[Richard Marti (:Paenglab)]

Mike, I forgot to write before, when you calculate the titlebar margin-bottom, could you set the margin when in #main-window[sizemode="maximized"] the gap is only 2px? When in maximized we don't need the gap of 16px for easier mouse grab to move the window.

Comment 19

[Richard Marti (:Paenglab)]

(In reply to Mike Conley (:mconley) from comment #15)
> Created attachment 633537 [details] [diff] [review]
> Checkpointing work
> 
> Checkpointing work, and stashing patch on Bugzilla so I can move it over to
> my Windows machine.

Mike, I tried your patch. When I start TB non-maximized then the calculations aren't done. When I maximize TB the calculations are done. The margin-bottom for the titlebar is only applied when maximied.

Comment 20

[Richard Marti (:Paenglab)]

Attachment: patch v8 (needs patch from bug 763308 first applied)

Fixed some margins to position the captionButtons on the right height.

Comment 21

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

Attachment: Patch v9

Ok, I think we've got a working port here.

Does this work for you, Richard?

Comment 22

[Richard Marti (:Paenglab)]

I still see the problem I wrote in comment 19. I tried also the pref mail.tabs.drawInTitlebar set to false. The only change I see is the spacer box for the windowCaptions has a width of 0px. I see also never a attribute of tabsintitlebar="true" on #messengerWindow.

Do we need a pref to choose between normal and draw in titlebar or should we hard code the draw in titlebar? When we hard code then we need no pref and all the calculations for the spacer box and the negative margin on titlebar should be done all the time.

Comment 23

[Richard Marti (:Paenglab)]

I asked Andreas on IRC and he thinks, no pref is needed.

Comment 24

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

(In reply to Richard Marti [:paenglab] from comment #22)
> I still see the problem I wrote in comment 19.

Ah yeah, I'm seeing that too. Investigating a fix...

Comment 25

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

(In reply to Richard Marti [:paenglab] from comment #23)
> I asked Andreas on IRC and he thinks, no pref is needed.

So you're suggesting we axe the mail.tabs.drawInTitlebar pref?

Comment 26

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

(In reply to Mike Conley (:mconley) from comment #24)
> (In reply to Richard Marti [:paenglab] from comment #22)
> > I still see the problem I wrote in comment 19.
> 
> Ah yeah, I'm seeing that too. Investigating a fix...

Richard:

So it looks like this issue is caused because there's a rule for #titlebar in mailWindow1.css that's causing margin-bottom to be -12px initially.

If you remove the margin-bottom rule, does this fix the issue for you?

-Mike

Comment 27

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

Hm, nevermind. That seems to break the maximized case. :/

Comment 28

[Richard Marti (:Paenglab)]

(In reply to Mike Conley (:mconley) from comment #25)
> (In reply to Richard Marti [:paenglab] from comment #23)
> > I asked Andreas on IRC and he thinks, no pref is needed.
> 
> So you're suggesting we axe the mail.tabs.drawInTitlebar pref?

Yes, the draw in titlebar would be always enabled. Australis has this also on non-maximized windows.

> So it looks like this issue is caused because there's a rule for #titlebar
> in mailWindow1.css that's causing margin-bottom to be -12px initially.

When the automatic calculation works also in non-maximized mode then this rule can be removed.

Comment 29

[Richard Marti (:Paenglab)]

Attachment: patch for review

Okay, let's go this patch through review.

Mike, please check my changes in JS carefully. You know I'm a noob in JS :)

Comment 30

[Andreas Nilsson (:andreasn)]

Thing are looking really good!

I've noticed that under both Aero and Basic there is a difference between the edges and the area for the tabs and window controls, but this can be addressed in a followup patch.

Richard: If I recall correctly you have a XP VM. Can you post a screenshot how the patch looks under XP? I've only tried it it compability mode.

Comment 31

[Richard Marti (:Paenglab)]

Attachment: screenshot

This is from a original XP. Thanks to my empoyer ;)

Comment 32

[Andreas Nilsson (:andreasn)]

Comment on attachment 636093 [details] [diff] [review]
patch for review

Thanks for the screenshots!
ui-review approved.

Comment 33

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

Comment on attachment 636093 [details] [diff] [review]
patch for review

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

This looks good - just one nit, and and a question. See below.

::: mail/base/content/msgMail3PaneWindow.js
@@ +1601,5 @@
> +      if (event.target != window)
> +        return;
> +      let sizemode = document.documentElement.getAttribute("sizemode");
> +      TabsInTitlebar.allowedBy("sizemode",
> +                                sizemode == "normal" ||

Hrm. I think sizemode can only have the values "normal", "maximized" and "fullscreen", so essentially, we're saying allowedBy("sizemode", true).

In Firefox's implementation of TabsInTitlebar, they don't have sizemode == "normal" in there...

What happens if we remove this? Do things still work as expected?

(And, before you say it - I know, I'm likely the person who added sizemode == "normal". But I'm not entirely sure it's necessary anymore).

::: mail/themes/qute/mail/mailWindow1.css
@@ +396,5 @@
> +    background-color: InactiveCaption;
> +  }
> +}
> +
> +/* Luna silver needs special colors. Default don't look good */

Grammar nit: should be "doesn't", not "don't".

Comment 34

[Richard Marti (:Paenglab)]

Attachment: Patch v11

(In reply to Mike Conley (:mconley) from comment #33)
> Comment on attachment 636093 [details] [diff] [review]
> Hrm. I think sizemode can only have the values "normal", "maximized" and
> "fullscreen", so essentially, we're saying allowedBy("sizemode", true).

It works with ("sizemode", true). Changed to this in new patch.

> > +/* Luna silver needs special colors. Default don't look good */
> 
> Grammar nit: should be "doesn't", not "don't".

fixed

Taking over ui-r+ from previous patch.

Comment 35

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

(In reply to Richard Marti [:paenglab] from comment #34)
> Created attachment 639362 [details] [diff] [review]
> Patch v11
> 
> (In reply to Mike Conley (:mconley) from comment #33)
> > Comment on attachment 636093 [details] [diff] [review]
> > Hrm. I think sizemode can only have the values "normal", "maximized" and
> > "fullscreen", so essentially, we're saying allowedBy("sizemode", true).
> 
> It works with ("sizemode", true). Changed to this in new patch.

Yeah, it works, but *should* it? Like, shouldn't we be doing what Firefox does and set allowedBy to only be when sizemode is maximized / fullscreen?

Does it work with just those two?

Comment 36

[Richard Marti (:Paenglab)]

(In reply to Mike Conley (:mconley) from comment #35)
> Yeah, it works, but *should* it? Like, shouldn't we be doing what Firefox
> does and set allowedBy to only be when sizemode is maximized / fullscreen?
> 
> Does it work with just those two?

I'd say it should also be in normal mode.

When you look at http://25.media.tumblr.com/tumblr_m0iwtyy6nh1qkoea4o1_1280.png you see it's a window in normal mode. Also on http://people.mozilla.com/~shorlander/files/australis-designSpecs/australis-designSpecs-windowsXP-lunaBlue-mainWindow.html and the other XP color pages or http://people.mozilla.com/~shorlander/files/australis-designSpecs/australis-designSpecs-windows7-mainWindow.html you can see the windows in draw in titlebar mode. On the Win7 page you can also see a image with "Tabs in a Maximized Window".

The only problem I'm seeing is on XP with menu enabled. I think the menu should be placed below the tabs like on Win7 and not on top in the titlebar. For this I'm planning to open a bug after this bug's landing.

Comment 37

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

Comment on attachment 639362 [details] [diff] [review]
Patch v11

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

(In reply to Richard Marti [:paenglab] from comment #36)
> (In reply to Mike Conley (:mconley) from comment #35)
> > Yeah, it works, but *should* it? Like, shouldn't we be doing what Firefox
> > does and set allowedBy to only be when sizemode is maximized / fullscreen?
> > 
> > Does it work with just those two?
> 
> I'd say it should also be in normal mode.
> 
> When you look at
> http://25.media.tumblr.com/tumblr_m0iwtyy6nh1qkoea4o1_1280.png you see it's
> a window in normal mode. Also on
> http://people.mozilla.com/~shorlander/files/australis-designSpecs/australis-
> designSpecs-windowsXP-lunaBlue-mainWindow.html and the other XP color pages
> or
> http://people.mozilla.com/~shorlander/files/australis-designSpecs/australis-
> designSpecs-windows7-mainWindow.html you can see the windows in draw in
> titlebar mode. On the Win7 page you can also see a image with "Tabs in a
> Maximized Window".
> 
> The only problem I'm seeing is on XP with menu enabled. I think the menu
> should be placed below the tabs like on Win7 and not on top in the titlebar.
> For this I'm planning to open a bug after this bug's landing.

Ah, fair enough, I was going off of what was currently in Firefox's implementation. 

Ok, on that note, just one final fix, and then r=me.

Great job Richard, as usual.

::: mail/base/content/msgMail3PaneWindow.js
@@ +1599,5 @@
> +    this.allowedBy("sizemode", false);
> +    window.addEventListener("resize", function (event) {
> +      if (event.target != window)
> +        return;
> +      let sizemode = document.documentElement.getAttribute("sizemode");

Since we're not checking the window's sizemode anymore, we can get rid of line 1603.

Comment 38

[Richard Marti (:Paenglab)]

Attachment: Patch for check-in

Patch addressing the last comment.

Taking the r+ and ui-r+ from previous patch.

Comment 39

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/106c1726c7d7

Comment 40

[Mark Banner (:standard8)]

Sorry, but I had to back this out due to test failures on Mac and Linux

https://hg.mozilla.org/comm-central/rev/a9ea38d7a17c

https://tbpl.mozilla.org/php/getParsedLog.php?id=13278934&tree=Thunderbird-Trunk

TEST-START | /home/cltbld/talos-slave/test/build/mozmill/account/test-account-port-setting.js | teardownModule
Test Failure: You need to specify a tab!
TEST-UNEXPECTED-FAIL | /home/cltbld/talos-slave/test/build/mozmill/account/test-account-port-setting.js | test-account-port-setting.js::teardownModule

Comment 41

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

Attachment: Fix preprocessor stuff for pinstripe / Linux

Grrr - I forgot that we were using preprocessor stuff here, and forgot to test on Linux and OSX (since this patch really doesn't affect them).

Anyhow, that's what broke us - in the case where CAN_DRAW_IN_TITLEBAR is false, we rendered busted Javascript.

Comment 42

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

Comment on attachment 639721 [details] [diff] [review]
Fix preprocessor stuff for pinstripe / Linux

Blake:

I already reviewed Paenglab's version of this, but I had to modify it by adding some more preprocessor stuff in TabsInTitlebar.

rs me?

-Mike

Comment 43

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

Comment on attachment 639721 [details] [diff] [review]
Fix preprocessor stuff for pinstripe / Linux

rs=bwinton on this.

I think we're OK to mark this as checkin-needed now.

Comment 44

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/855bb70eb2c7

Comment 45

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

Marking this as tracking TB 16, because we want to back it out once we hit the Aurora channel.

Comment 46

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

Attachment: Aurora backout patch

Paenglab:

Here's the backout patch - but it doesn't apply cleanly. It seems to conflict with changes in primaryToolbar-aero.css.

Can you resolve the conflict?

-Mike

Comment 47

[Richard Marti (:Paenglab)]

Attachment: Aurora backout patch

Bug 767162 made the problem. Patch adapted for this change.

Comment 48

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

Backed out of comm-aurora (TB 16): https://hg.mozilla.org/releases/comm-aurora/rev/6c2c3ee473c6

Comment 49

[Szabolcs Hubai (:xabolcs)]

Attachment: mail.tabs.drawInTitlebar enabled with Zune Desktop Theme

Just wanted to note that drawing in titlebar causes unreadable visuals
with the official Zune theme [1].


[1]: http://go.microsoft.com/fwlink/?LinkID=75078

Comment 50

[Szabolcs Hubai (:xabolcs)]

Attachment: mail.tabs.drawInTitlebar disabled with Zune Desktop Theme

Of course, it's all OK if mail.tabs.drawInTitlebar disabled.

Comment 51

[Richard Marti (:Paenglab)]

It looks I haven't the official Zune theme. Please can you open a new bug and CC me to it?

Comment 52

[Andreas Nilsson (:andreasn)]

Is the Zune theme specific only to Vista? I don't seem to see it in Win7.

Comment 53

[Richard Marti (:Paenglab)]

Zune is Win XP.

Comment 54

[jonrandy]

Fixed? This looks awful in Windows XP using any custom theme I have tried. The title bar simply is not being drawn - it is showing as the colour of the menu background. The work being done on Firefox to implement the Australis design looks to be working well with custom themes - I'm guessing similar has not been done here?

I can turn the 'draw in titlebar' off for now, but it would be nice if it worked as I would like the tabs up there. Also, turning this on by default without warning seemed a poor decision as it looks so bad on some systems

Comment 55

[Blake Winton (:bwinton) (:☕️)]

Hi Jonrandy,

what you describe sounds like a different issue to me, but one I would like to see fixed.  Could you please file a new bug, cc-ing Paneglab and I, listing the steps you take to see the issue, what you see, and what you expected to see?

(Also, if you could avoid phrases like "looks awful" in the new bug, that would make it easier for me to convince people to help fix it…  :)

Thank you,
Blake.