Allow for drawing in the window titlebar for Australis

RESOLVED FIXED in Thunderbird 17.0

Status

Thunderbird
Mail Window Front End
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mconley, Assigned: Paenglab)

Tracking

Trunk
Thunderbird 17.0
x86
Windows 7
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 17 obsolete attachments)

65.63 KB, image/png
Details
23.77 KB, patch
mconley
: review+
Details | Diff | Splinter Review
17.51 KB, patch
Details | Diff | Splinter Review
29.04 KB, image/png
Details
30.73 KB, image/png
Details
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.
Assignee: mconley → nisses.mail
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");
}
Created attachment 624433 [details] [diff] [review]
first try

Initial stab
Known issues:
* Personas hides the min-max-close buttons
* Need to skip the top padding when window is maximized.
If anyone runs this patch DON'T MAXIMIZE.
It's apparently quite hard to get back again.
(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.
Created attachment 624753 [details] [diff] [review]
patch (v2)

Checkpoint before I start adding in exploding stuff.
Attachment #624433 - Attachment is obsolete: true
Created attachment 624758 [details] [diff] [review]
patch (v2)
Attachment #624753 - Attachment is obsolete: true
Created attachment 625635 [details] [diff] [review]
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).
Attachment #624758 - Attachment is obsolete: true
Created attachment 625716 [details] [diff] [review]
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.
Attachment #625635 - Attachment is obsolete: true
Created attachment 625719 [details] [diff] [review]
patch (v4.1)

Allow dragging in tabs toolbar and using lwtheme now, but currently don't work for Aero.
Attachment #625716 - Attachment is obsolete: true
Created attachment 625979 [details] [diff] [review]
patch (v5)

All dragging works now, still some small quirks with the lwtheme window buttons though.
Attachment #625719 - Attachment is obsolete: true
Created attachment 626411 [details] [diff] [review]
patch (v6)

Just a small update allowing the maximize/resize and close buttons work in Basic under Win7.
Attachment #625979 - Attachment is obsolete: true
Paenglab mentioned that we'll want to port Firefox's TabsInTitleBar to Thunderbird, so I'm going to try to do that today.
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.
(Assignee)

Comment 16

5 years ago
Created attachment 633799 [details] [diff] [review]
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.
Attachment #626411 - Attachment is obsolete: true
(Assignee)

Comment 17

5 years ago
Created attachment 633801 [details]
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.
(Assignee)

Comment 18

5 years ago
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.
(Assignee)

Comment 19

5 years ago
(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.
(Assignee)

Updated

5 years ago
Attachment #633799 - Attachment description: patch v7 → patch v7 (needs patch from bug 763308 first applied)
(Assignee)

Comment 20

5 years ago
Created attachment 633898 [details] [diff] [review]
patch v8 (needs patch from bug 763308 first applied)

Fixed some margins to position the captionButtons on the right height.
Attachment #633799 - Attachment is obsolete: true
Depends on: 763308
Created attachment 634452 [details] [diff] [review]
Patch v9

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

Does this work for you, Richard?
Attachment #633537 - Attachment is obsolete: true
Attachment #633898 - Attachment is obsolete: true
(Assignee)

Comment 22

5 years ago
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.
(Assignee)

Comment 23

5 years ago
I asked Andreas on IRC and he thinks, no pref is needed.
(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...
(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?
(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
Hm, nevermind. That seems to break the maximized case. :/
(Assignee)

Comment 28

5 years ago
(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.
(Assignee)

Comment 29

5 years ago
Created attachment 636093 [details] [diff] [review]
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 :)
Attachment #634452 - Attachment is obsolete: true
Attachment #636093 - Flags: ui-review?(nisses.mail)
Attachment #636093 - Flags: review?(mconley)
(Assignee)

Updated

5 years ago
Attachment #633801 - Attachment is obsolete: true
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.
(Assignee)

Comment 31

5 years ago
Created attachment 636645 [details]
screenshot

This is from a original XP. Thanks to my empoyer ;)
Comment on attachment 636093 [details] [diff] [review]
patch for review

Thanks for the screenshots!
ui-review approved.
Attachment #636093 - Flags: ui-review?(nisses.mail) → ui-review+
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".
(Assignee)

Comment 34

5 years ago
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.

> > +/* 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.
Attachment #636093 - Attachment is obsolete: true
Attachment #636093 - Flags: review?(mconley)
Attachment #639362 - Flags: ui-review+
Attachment #639362 - Flags: review?(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?
(Assignee)

Comment 36

5 years ago
(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 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.
Attachment #639362 - Flags: review?(mconley) → review+
(Assignee)

Comment 38

5 years ago
Created attachment 639388 [details] [diff] [review]
Patch for check-in

Patch addressing the last comment.

Taking the r+ and ui-r+ from previous patch.
Attachment #639362 - Attachment is obsolete: true
Attachment #639388 - Flags: ui-review+
Attachment #639388 - Flags: review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/106c1726c7d7
Assignee: nisses.mail → richard.marti
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite-
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 16.0
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Created attachment 639721 [details] [diff] [review]
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 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
Attachment #639721 - Flags: review?(bwinton)
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.
Attachment #639721 - Flags: review?(bwinton) → review+
Attachment #639388 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/855bb70eb2c7
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Marking this as tracking TB 16, because we want to back it out once we hit the Aurora channel.
tracking-thunderbird16: --- → ?
Created attachment 642954 [details] [diff] [review]
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
(Assignee)

Updated

5 years ago
Depends on: 774167
(Assignee)

Comment 47

5 years ago
Created attachment 642999 [details] [diff] [review]
Aurora backout patch

Bug 767162 made the problem. Patch adapted for this change.
Attachment #642954 - Attachment is obsolete: true
Backed out of comm-aurora (TB 16): https://hg.mozilla.org/releases/comm-aurora/rev/6c2c3ee473c6
tracking-thunderbird16: ? → ---
Target Milestone: Thunderbird 16.0 → Thunderbird 17.0
(Assignee)

Updated

5 years ago
Depends on: 771816
Created attachment 657201 [details]
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
Created attachment 657202 [details]
mail.tabs.drawInTitlebar disabled with Zune Desktop Theme

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

Comment 51

5 years ago
It looks I haven't the official Zune theme. Please can you open a new bug and CC me to it?
Is the Zune theme specific only to Vista? I don't seem to see it in Win7.
(Assignee)

Comment 53

5 years ago
Zune is Win XP.
(Assignee)

Updated

5 years ago
Depends on: 787654

Comment 54

5 years ago
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
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.
Duplicate of this bug: 607628
You need to log in before you can comment on or make changes to this bug.