Closed Bug 755793 Opened 12 years ago Closed 12 years ago

Allow for drawing in the window titlebar for Australis

Categories

(Thunderbird :: Mail Window Front End, defect)

x86
Windows 7
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 17.0

People

(Reporter: mconley, Assigned: Paenglab)

References

Details

Attachments

(5 files, 17 obsolete files)

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
      No description provided.
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");
}
Attached patch first try (obsolete) — Splinter Review
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.
Attached patch patch (v2) (obsolete) — Splinter Review
Checkpoint before I start adding in exploding stuff.
Attachment #624433 - Attachment is obsolete: true
Attached patch patch (v2) (obsolete) — Splinter Review
Attachment #624753 - Attachment is obsolete: true
Attached patch patch (v3) (obsolete) — Splinter Review
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
Attached patch patch (v4) (obsolete) — Splinter Review
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
Attached patch patch (v4.1) (obsolete) — Splinter Review
Allow dragging in tabs toolbar and using lwtheme now, but currently don't work for Aero.
Attachment #625716 - Attachment is obsolete: true
Attached patch patch (v5) (obsolete) — Splinter Review
All dragging works now, still some small quirks with the lwtheme window buttons though.
Attachment #625719 - Attachment is obsolete: true
Attached patch patch (v6) (obsolete) — Splinter Review
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.
Attached patch Checkpointing work (obsolete) — Splinter Review
Checkpointing work, and stashing patch on Bugzilla so I can move it over to my Windows machine.
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
Attached image screenshot of actual patch (obsolete) —
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.
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.
(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.
Attachment #633799 - Attachment description: patch v7 → patch v7 (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
Attached patch Patch v9 (obsolete) — Splinter Review
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
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.
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. :/
(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.
Attached patch patch for review (obsolete) — Splinter 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)
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.
Attached image 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".
Attached patch Patch v11 (obsolete) — Splinter Review
(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?
(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+
Attached patch Patch for check-in (obsolete) — Splinter Review
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+
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/106c1726c7d7
Assignee: nisses.mail → richard.marti
Status: NEW → RESOLVED
Closed: 12 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 → ---
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
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/855bb70eb2c7
Status: REOPENED → RESOLVED
Closed: 12 years ago12 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.
Attached patch Aurora backout patch (obsolete) — Splinter Review
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
Depends on: 774167
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
Target Milestone: Thunderbird 16.0 → Thunderbird 17.0
Depends on: 771816
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
Of course, it's all OK if mail.tabs.drawInTitlebar disabled.
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.
Zune is Win XP.
Depends on: 787654
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.
You need to log in before you can comment on or make changes to this bug.