Use the Fx Australis titlebar styling

RESOLVED FIXED in Thunderbird 31.0

Status

Thunderbird
Theme
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Paenglab, Assigned: Paenglab)

Tracking

unspecified
Thunderbird 31.0
All
Windows 7

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 11 obsolete attachments)

(Assignee)

Description

4 years ago
This bug is to style the titlebar and also the tabbar background similar to Firefox. This includes a updated margin/height calculation in msgMail3PaneWindow.js.
(Assignee)

Comment 1

4 years ago
Created attachment 8351516 [details] [diff] [review]
patch

This patch needs bug 953011 and bug 952953 applied first.

Mike, please can you check if all is needed I added in msgMail3PaneWindow.js? I removed already the CustomizableUI etc. things.

I had to add a mutation observer for sizemode changes, please can you check if this can be shortened? I copied the _onMenuMutate: function (aMutations) { to _onSizeModeMutate: function (aMutations) { This uses only one attribute but all my tries to shorten this ended in not defined errors.
Assignee: nobody → richard.marti
Status: NEW → ASSIGNED
Attachment #8351516 - Flags: ui-review?(mconley)
Attachment #8351516 - Flags: review?(mconley)
(Assignee)

Comment 2

4 years ago
Comment on attachment 8351516 [details] [diff] [review]
patch

I'm cancelling after changes on bug 952953.
Attachment #8351516 - Flags: ui-review?(mconley)
Attachment #8351516 - Flags: review?(mconley)
(Assignee)

Comment 3

4 years ago
Created attachment 8355755 [details] [diff] [review]
patch v2

Unbitrotted after bug 876635 and updated after changes in bug 952953.
Still needs bug 952953 applied first.

Please can you still check, if _onSizeModeMutate: function (aMutations) can be shortened?
Attachment #8351516 - Attachment is obsolete: true
Attachment #8355755 - Flags: ui-review?(mconley)
Attachment #8355755 - Flags: review?(mconley)
(Assignee)

Comment 4

4 years ago
Created attachment 8355799 [details] [diff] [review]
Patch v3

Oops, found some trailing whitespace.
Attachment #8355755 - Attachment is obsolete: true
Attachment #8355755 - Flags: ui-review?(mconley)
Attachment #8355755 - Flags: review?(mconley)
Attachment #8355799 - Flags: ui-review?(mconley)
Attachment #8355799 - Flags: review?(mconley)
(Assignee)

Comment 5

4 years ago
Created attachment 8361297 [details] [diff] [review]
patch v4

Removed a CustomizableUI line I forgot to remove.
Attachment #8355799 - Attachment is obsolete: true
Attachment #8355799 - Flags: ui-review?(mconley)
Attachment #8355799 - Flags: review?(mconley)
Attachment #8361297 - Flags: ui-review?(mconley)
Attachment #8361297 - Flags: review?(mconley)
Comment on attachment 8361297 [details] [diff] [review]
patch v4

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

Hey Richard, sorry for the delay (again).

This looks pretty good, and great job porting all of the Javascript over. Some of the port, however, is unnecessary - particularly the menubar stuff. Mind wiping that stuff out? Also, I took some measurements, and it looks like (at least on Win 7), the space above the tabstrip is a few pixels short of what Firefox does. That might actually be related to the menubar, since TabsInTitlebar._update might be doing some subtractions there based on it.

Anyhow, I think it's starting to look pretty sharp. :)

::: mail/base/content/msgMail3PaneWindow.js
@@ +1680,5 @@
> +    // less settled before updating the titlebar. So instead of listening to
> +    // DOMMenuBarActive and DOMMenuBarInactive, we use a MutationObserver to
> +    // watch the "invalid" attribute directly.
> +    let menu = document.getElementById("mail-toolbar-menubar2");
> +    this._menuObserver = new MutationObserver(this._onMenuMutate);

I'm not entirely sure this is necessary. In Thunderbird, the menubar appears below the tabstrip, so it doesn't push the tabs down or necessitate making the titlebar taller.

@@ +1724,4 @@
>        this._readPref();
>    },
>  
> +  _onMenuMutate: function (aMutations) {

Yep, pretty sure we don't need this.

@@ +1795,5 @@
>  
> +      // Buttons first:
> +      let captionButtonsBoxWidth = rect($("titlebar-buttonbox")).width;
> +      // Get the height and margins separately for the menubar
> +      let menuHeight = rect(menubar).height;

Again, I don't think any of the menubar calculations need to come into play here, since it's not factored into the height of the titlebar.

@@ +1817,5 @@
> +
> +      // If the menubar is around (menuHeight is non-zero), try to adjust
> +      // its full height (i.e. including margins) to match the titlebar,
> +      // by changing the menubar's bottom padding
> +      if (menuHeight) {

This entire block can probably be removed.

@@ +1863,5 @@
> +      // Then we bring up the titlebar by the same amount, but we add any
> +      // negative margin:
> +      titlebar.style.marginBottom = "-" + titlebarContentHeight + "px";
> +
> +

This extra newline can go.
Attachment #8361297 - Flags: ui-review?(mconley)
Attachment #8361297 - Flags: review?(mconley)
Attachment #8361297 - Flags: feedback+
(Assignee)

Comment 7

4 years ago
Created attachment 8392354 [details] [diff] [review]
patch v5

(In reply to Mike Conley (:mconley) from comment #6)
> Comment on attachment 8361297 [details] [diff] [review]
> patch v4
> 
> Review of attachment 8361297 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Hey Richard, sorry for the delay (again).

No problem, I know you have a lot of work for Australis and also a private live.

> This looks pretty good, and great job porting all of the Javascript over.
> Some of the port, however, is unnecessary - particularly the menubar stuff.
> Mind wiping that stuff out? Also, I took some measurements, and it looks
> like (at least on Win 7), the space above the tabstrip is a few pixels short
> of what Firefox does. That might actually be related to the menubar, since
> TabsInTitlebar._update might be doing some subtractions there based on it.

The menubar calculations are needed for XP and Win7 Basic/Classic where the menubar is above the tabs. On Aero the menubar is below the tabs because a lot of users had problems with readability.

> @@ +1863,5 @@
> > +      // Then we bring up the titlebar by the same amount, but we add any
> > +      // negative margin:
> > +      titlebar.style.marginBottom = "-" + titlebarContentHeight + "px";
> > +
> > +
> 
> This extra newline can go.

Done.

I've also updated the patch with bug 930094. Now it should be easier to apply the TabsInTitlebar for OS X.

This patch doesn't have bug 978752 applied. I'll file a new bug when this bug is fixed.
Attachment #8361297 - Attachment is obsolete: true
Attachment #8392354 - Flags: ui-review?(mconley)
Attachment #8392354 - Flags: review?(mconley)
(Assignee)

Comment 8

4 years ago
Created attachment 8393497 [details] [diff] [review]
patch v6

V5 didn't apply cleanly because of a other patch in my queue.

I forgot also to mention I corrected the space between top border and tabs. It was 1px off because the menubar has a 1px margin which I hadn't calculated on Aero where the menubar is below the tabs.
Attachment #8392354 - Attachment is obsolete: true
Attachment #8392354 - Flags: ui-review?(mconley)
Attachment #8392354 - Flags: review?(mconley)
Attachment #8393497 - Flags: ui-review?(mconley)
Attachment #8393497 - Flags: review?(mconley)
(Assignee)

Comment 9

4 years ago
Created attachment 8405755 [details] [diff] [review]
patch v7

The patch was again bitrotted. When I was on it, I updated the patch with the latest Classic titlebar stylings from FX.

Philipp, this Classic titlebar changes needs also a small change for Lightning. The use of the position: relatve on the toolbars made the calendar-time-bar showing over the complete TB.
Attachment #8393497 - Attachment is obsolete: true
Attachment #8393497 - Flags: ui-review?(mconley)
Attachment #8393497 - Flags: review?(mconley)
Attachment #8405755 - Flags: ui-review?(mconley)
Attachment #8405755 - Flags: review?(philipp)
Attachment #8405755 - Flags: review?(mconley)
(Assignee)

Comment 10

4 years ago
Created attachment 8405780 [details] [diff] [review]
patch v7

I removed two rules which are already in messenger.css.
Attachment #8405755 - Attachment is obsolete: true
Attachment #8405755 - Flags: ui-review?(mconley)
Attachment #8405755 - Flags: review?(philipp)
Attachment #8405755 - Flags: review?(mconley)
Attachment #8405780 - Flags: ui-review?(mconley)
Attachment #8405780 - Flags: review?(philipp)
Attachment #8405780 - Flags: review?(mconley)
Does this change affect how it looks in Seamonkey? We should make sure we don't break them.
(Assignee)

Comment 12

4 years ago
I've tried a today Daily, which don't have this patch's changes, with a Lightning with this change and I see no difference in normal and rotated view. So I suppose it's also working in Seamonkey.
Comment on attachment 8405780 [details] [diff] [review]
patch v7

JosiahOne has graciously offered to help review this. Thanks JosiahOne! :D
Attachment #8405780 - Flags: review?(josiah)
Comment on attachment 8405780 [details] [diff] [review]
patch v7

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

ok, lets give it a try for calendar, r=philipp
Attachment #8405780 - Flags: review?(philipp) → review+
(Assignee)

Comment 15

4 years ago
Created attachment 8411121 [details] [diff] [review]
AustralisTitlebar.patch v8

Patch was bitrotted by bug 995747.
Attachment #8405780 - Attachment is obsolete: true
Attachment #8405780 - Flags: ui-review?(mconley)
Attachment #8405780 - Flags: review?(mconley)
Attachment #8405780 - Flags: review?(josiah)
Attachment #8411121 - Flags: ui-review?(mconley)
Attachment #8411121 - Flags: review?(mconley)
Attachment #8411121 - Flags: review?(josiah)
Comment on attachment 8411121 [details] [diff] [review]
AustralisTitlebar.patch v8

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

Well, it works and the code looks okay. So r+ for the CSS portion.
Attachment #8411121 - Flags: review?(josiah) → review+
(Assignee)

Comment 18

4 years ago
Created attachment 8413329 [details] [diff] [review]
AustralisTitlebar.patch v9

Unbitrot after bug 984978 landing.
Attachment #8411121 - Attachment is obsolete: true
Attachment #8411121 - Flags: ui-review?(mconley)
Attachment #8411121 - Flags: review?(mconley)
(Assignee)

Updated

4 years ago
Attachment #8413329 - Flags: ui-review+
Attachment #8413329 - Flags: review?(mconley)
Out of curiosity, where did the ui-review+ come from?
Flags: needinfo?(richard.marti)
Comment on attachment 8413329 [details] [diff] [review]
AustralisTitlebar.patch v9

Transferring that ui-review to me.
Attachment #8413329 - Flags: ui-review+ → ui-review?(josiah)
Flags: needinfo?(richard.marti)
Comment on attachment 8413329 [details] [diff] [review]
AustralisTitlebar.patch v9

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

Alright, tested on Windows 8, Windows 8 (High Contrast), Windows 7, Windows 7 (Classic), and Windows 7 (High Contrast) and they all look good.

So ui-r+.
Attachment #8413329 - Flags: ui-review?(josiah) → ui-review+
Comment on attachment 8413329 [details] [diff] [review]
AustralisTitlebar.patch v9

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

Alright, let's get this in there. Thanks for your hard work and patience, Richard.

::: mail/base/content/msgMail3PaneWindow.js
@@ +1881,5 @@
> +        // We need to reduce the height by the amount of navbar overlap
> +        // (this value is 0 or negative):
> +        extraMargin += tabmailMarginTop;
> +        // On non-OSX, we can just use bottom margin:
> +        titlebarContent.style.marginBottom = extraMargin + "px";

This is supposed to be behind an XP_MACOSX ifndef...

I suppose bug 768516 can do it. I'll make a note of that in my review for it.

::: mail/themes/windows/mail/primaryToolbar-aero.css
@@ +89,5 @@
>                                      transparent 1px);
>  }
>  
> +/**
> + * In the classic themes, the titlebar has a horizontal gradient, which is

It's a shame about the duplication in here. Can you please file a bug to try to re-use some of this stuff instead of duping it between primaryToolbar.css and primaryToolbar-aero.css?
Attachment #8413329 - Flags: review?(mconley) → review+
This patch doesn't apply cleanly on trunk, Richard... does it depend on something?
Flags: needinfo?(richard.marti)
Created attachment 8413464 [details] [diff] [review]
Australis Titlebar Patch V. 9

It looks like you had your Preferences stuff applied when you qnew'd. base's messenger.css has this context in your patch:

/* Preferences */
 
 preftab,
 preftab:root /* override :root from above */ {
   -moz-binding: url("chrome://messenger/content/preferences/preferences.xml#preftab");
   -moz-box-orient: vertical;
 }

Which isn't in the normal file. I updated that part for you in this patch (You still need to address mconley's feedback).
Attachment #8413464 - Flags: ui-review+
Attachment #8413464 - Flags: review+
(Assignee)

Comment 25

4 years ago
Created attachment 8413595 [details] [diff] [review]
Patch for check-in

Josiah replied and fixed the patch already. Thanks a lot for this. I fixed the #ifndef XP_MACOSX to be sure it's in trunk also when bug 768516 can't land together with this bug.
Attachment #8413329 - Attachment is obsolete: true
Attachment #8413464 - Attachment is obsolete: true
Attachment #8413595 - Flags: ui-review+
Attachment #8413595 - Flags: review+
Flags: needinfo?(richard.marti)
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
(Assignee)

Comment 26

4 years ago
(In reply to Mike Conley (:mconley) from comment #22)
> 
> It's a shame about the duplication in here. Can you please file a bug to try
> to re-use some of this stuff instead of duping it between primaryToolbar.css
> and primaryToolbar-aero.css?

I filed bug 1002370 for this.
https://hg.mozilla.org/comm-central/rev/11c8b396d726
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 31.0
You need to log in before you can comment on or make changes to this bug.