670304 - Buttons on the tab bar should match Firefox

Status: RESOLVED FIXED

(Thunderbird :: Theme, defect)

Description

[Jim Porter (:squib)]

Attachment: The all tabs button in Firefox

In Firefox under Aero, the buttons on the tab bar (usually "All Tabs" and the tab scroll arrows, but anything can go there) have a gradient background instead of a button background. We should do this. It would also provide a nice alternative for the quick filter bar, which is styled like a tab right now (that's bug 667244).

Comment 1

[Jim Porter (:squib)]

And here's where that style is set in Firefox: http://mxr.mozilla.org/comm-central/source/mozilla/browser/themes/winstripe/browser/browser.css#732

Comment 2

[Richard Marti (:Paenglab)]

Attachment: Same appearance as Firefox

I had to put the definitions in messenger-aero.css instead of tabmail-aero.css because the Lightning buttons wheren't affected in the latter.

Comment 3

[Jim Porter (:squib)]

(In reply to comment #2)
> Created attachment 544966 [details] [diff] [review] [review]
> Same appearance as Firefox
> 
> I had to put the definitions in messenger-aero.css instead of
> tabmail-aero.css because the Lightning buttons wheren't affected in the
> latter.

Would it be possible to simplify the selector to ".tabmail-tabs toolbarbutton"? It seems like all the affected elements are toolbarbuttons inside .tabmail-tabs, and there aren't any toolbarbuttons in there that should *not* have this styling.

Comment 4

[Richard Marti (:Paenglab)]

Attachment: Same appearance as Firefox v2

(In reply to comment #3)
> Would it be possible to simplify the selector to ".tabmail-tabs
> toolbarbutton"? It seems like all the affected elements are toolbarbuttons
> inside .tabmail-tabs, and there aren't any toolbarbuttons in there that
> should *not* have this styling.

Unfortunately the close buttons are also toolbarbuttons. I had to give the !important rule to background and padding.

Comment 5

[Jim Porter (:squib)]

(In reply to comment #4)
> Unfortunately the close buttons are also toolbarbuttons. I had to give the
> !important rule to background and padding.

Oh, in that case, maybe it's best to go with the original way. I'll leave it up to bwinton, though.

Comment 6

[Richard Marti (:Paenglab)]

But when the tabbar is/has a toolbar the rules should apply directly to all toolbarbuttons with the second patch.

Comment 7

[Jim Porter (:squib)]

(In reply to comment #6)
> But when the tabbar is/has a toolbar the rules should apply directly to all
> toolbarbuttons with the second patch.

Or add #tabbar-toolbar > toolbarbutton to the definitions. But either way is fine with me, I suppose.

Comment 8

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

Comment on attachment 549168 [details] [diff] [review]
Same appearance as Firefox v2

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

This seems nicer, although I'm curious as to how well it will work with the ability to drag toolbar buttons into the tab bar (bug 582801), and the ability to hide the menu bar (bug 581661).  Turns out, not so well...  :)

http://dl.dropbox.com/u/2301433/Glass/ShadedToolbarButtons.png

The extra fun thing is that while the AllTabs button is nicely shaded, the QuickFilter button has a different highlight, and the dragged-down buttons all do the weird glow-y thing, except for the Tag Messages button, which doesn't highlight itself at all.  :)

So, having said all that, I think I'm going to have to give this a ui-r-.

Other than those, though, I don't see any problems with the patch, so at least it gets an r+.  (I suspect there will be some pretty large changes to get it all working, so you'll probably need to re-request review when you fix all the stuff.)

Thanks,
Blake.

Comment 9

[Richard Marti (:Paenglab)]

(In reply to comment #8)
> The extra fun thing is that while the AllTabs button is nicely shaded, the
> QuickFilter button has a different highlight, 

The QFB button is covered by Bug 667244

> and the dragged-down buttons all do the weird glow-y thing, except for the
> Tag Messages button, which doesn't highlight itself at all.  :)

This should be covered by the bug introducing this functionality, but I'm now on it to do this in this bug ;)

And PS: on my machine the Tag button in tab toolbar is glowing when a message is selected ;)

Comment 10

[Richard Marti (:Paenglab)]

Attachment: Toolbar buttons in tab bar

Blake is this how the buttons should look on the tab bar?

If yes, then we should also have special colored icons like FX now has.

Comment 11

[Jim Porter (:squib)]

One thing that seems like it's missing compared to Firefox is that Firefox puts a drop shadow (in white, so it's really more of a glow) around the icons when they're on the tab bar. That would probably help us for contrast.

Comment 12

[Richard Marti (:Paenglab)]

(In reply to Jim Porter (:squib) from comment #11)
> One thing that seems like it's missing compared to Firefox is that Firefox
> puts a drop shadow (in white, so it's really more of a glow) around the
> icons when they're on the tab bar. That would probably help us for contrast.

Firefox has now also inverted icons (white with black border) for buttons on the tab bar.

Comment 13

[Richard Marti (:Paenglab)]

Attachment: Same appearance as Firefox v3

Now with the same appearance also for the main toolbarbuttons, when in tab bar (see attachment 550876 [details]).

I'm requesting again r=? because I've added primaryToolbar-aero.css and want to know if the indenting is correct for the long selectors.

Comment 14

[Richard Marti (:Paenglab)]

Attachment: Same appearance as Firefox v4

I cleaned this bug and bug 667244 and took the tab bar things (except QFB button) for XP to this patch.

Comment 15

[Richard Marti (:Paenglab)]

Attachment: Same appearance as Firefox v5

Sorry to bug spam but I found an error I made: border-radius: none; instead of border-radius: 0;

Comment 16

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

Comment on attachment 551313 [details] [diff] [review]
Same appearance as Firefox v5

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

I'm not sure where you want to handle this, but when we're on a tab that doesn't support the quick filter, we have a missing space, outlined in pink in <http://dl.dropbox.com/u/2301433/Glass/MissingQFB.png>.

As you can also see in the screenshot, the icons are really hard to see, so I'ld like us to get a different set for the toolbar, a la Firefox, before we commit this patch, so ui-r-.

And, but for the things I mention below, I'm happy with the code, so r=me.

Thanks,
Blake.

::: mail/themes/qute/mail/primaryToolbar-aero.css
@@ +256,5 @@
> +}
> +
> +#tabbar-toolbar
> +      toolbarbutton[type="menu-button"]:not([open="true"]):not(:active):hover >
> +                   .toolbarbutton-menubutton-dropmarker:not([disabled="true"]),

I think I would use four-space indents here.  And I don't think you need to doubly-indent the second line.

@@ -242,0 +242,32 @@
> > +/* ::::: toolbar buttons on tabbar toolbar ::::: */
> > +
> > +#tabbar-toolbar .toolbarbutton-1,
> > +#tabbar-toolbar .toolbarbutton-menubutton-button,
NaN more ...

Here, too, we should use four-space indents, instead of three.

@@ -242,0 +242,42 @@
> > +/* ::::: toolbar buttons on tabbar toolbar ::::: */
> > +
> > +#tabbar-toolbar .toolbarbutton-1,
> > +#tabbar-toolbar .toolbarbutton-menubutton-button,
NaN more ...

And here.  :)

Comment 17

[Richard Marti (:Paenglab)]

(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #16)
> Comment on attachment 551313 [details] [diff] [review]
> Same appearance as Firefox v5
> 
> Review of attachment 551313 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm not sure where you want to handle this, but when we're on a tab that
> doesn't support the quick filter, we have a missing space, outlined in pink
> in <http://dl.dropbox.com/u/2301433/Glass/MissingQFB.png>.

This is also before this patch the case (also the menuitem in View/Toolbars is empty on such a tab. What do you mean, should I leave the button visible but with a lesser opacity? But this should be handled in Bug 667244.

> As you can also see in the screenshot, the icons are really hard to see, so
> I'ld like us to get a different set for the toolbar, a la Firefox, before we
> commit this patch, so ui-r-.

Locally I have already the white icons. I can add them to this patch.

Comment 18

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

(In reply to Richard Marti [:paenglab] from comment #17)
> > I'm not sure where you want to handle this, but when we're on a tab that
> > doesn't support the quick filter, we have a missing space, outlined in pink
> > in <http://dl.dropbox.com/u/2301433/Glass/MissingQFB.png>.
> This is also before this patch the case (also the menuitem in View/Toolbars
> is empty on such a tab. What do you mean, should I leave the button visible
> but with a lesser opacity? But this should be handled in Bug 667244.

Firefox seems to do something like that (with lower contrast rather than lower opacity, though).  Can you check that bug 667244 actually fixes this, and if not, leave a comment there?

> > As you can also see in the screenshot, the icons are really hard to see, so
> > I'ld like us to get a different set for the toolbar, a la Firefox, before we
> > commit this patch, so ui-r-.
> Locally I have already the white icons. I can add them to this patch.

Cool, I'ld like to see them.  :)

Thanks,
Blake.

Comment 19

[Richard Marti (:Paenglab)]

Attachment: Same appearance as Firefox v6

Patch addressing the review comments and adding white arrows for XP and Vista/Win7 on dark personas and Glass.

Comment 20

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

Comment on attachment 555516 [details] [diff] [review]
Same appearance as Firefox v6

The code still looks fine, but I'm not seeing the lighter icons on the dark background.  Indeed, the screenshot I posted at <http://dl.dropbox.com/u/2301433/Glass/MissingQFB.png> looks basically the same with this version as it did with the previous patch.  :(  Hopefully there's something simple you forgot to add, and I can review a new patch soon.  :)

So, to sum up r+, and ui-r-.

Thanks,
Blake.

Comment 21

[Richard Marti (:Paenglab)]

Attachment: Firefox inverted icons

Andreas, please can you create for the Aero theme inverted icons like the Firefox icons I attached as an example?
Then I can create the patch which uses this icons on the tab-toolbar.

Comment 22

[Richard Marti (:Paenglab)]

Attachment: Same appearance as Firefox v7

Now without the need of lighter icons because the buttons from the main toolbar keep their toolbarbutton appearance also in the tab bar. The alltabs- and overflow scrollbar buttons becomes the Firefox look with inverted icons on dark personas and Aero Glass background.

This patch needs bug 667244 applied first.

Comment 23

[Andreas Nilsson (:andreasn)]

(In reply to Richard Marti [:paenglab] from comment #21)
> Created attachment 555828 [details]
> Firefox inverted icons
> 
> Andreas, please can you create for the Aero theme inverted icons like the
> Firefox icons I attached as an example?
> Then I can create the patch which uses this icons on the tab-toolbar.

Just to verify judging from the comment #22, you don't need these any more? (I read why somewhere else, bit hard to keep track of the status across different bugs, and just got back after being away for two weeks).

Comment 24

[Richard Marti (:Paenglab)]

(In reply to Andreas Nilsson (:andreasn) from comment #23)
> Just to verify judging from the comment #22, you don't need these any more?
> (I read why somewhere else, bit hard to keep track of the status across
> different bugs, and just got back after being away for two weeks).

Yes, no need for the inverted icons with the approach of using the same buttons as on the normal toolbars.

Comment 25

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

Comment on attachment 558132 [details] [diff] [review]
Same appearance as Firefox v7

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

As mentioned in bug 667344, I'm pretty happy with it, so I'm going to say ui-r=me, and we can fix the problems in the follow-up bug 687836 before we land all the patches.

And the code is fine, too, so r=me, but let's wait until the other bug is ready before we land everything.

Comment 26

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

And by "667344", I really meant bug 667244.  :P

Comment 27

[PimpUigi]

I'm confused by the recent turn of events here, can the current situation be paraphrased?

Comment 28

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

We're waiting for bug 687836 to be ready, and then all four bugs will land together.  (By which I mean bug 687836, and the three bugs mentioned in the first comment in that bug.)

Thanks,
Blake.

Comment 29

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

I believe this has landed in comm-central as http://hg.mozilla.org/comm-central/rev/24a39254d6a8 as part of the amalgamation of bug 687836.