toggling the quick filter toolbar with a tab is odd and confusing. This should be a toggle button or a checkbox in a menu somewhere. The control being used today makes little sense and is not discoverable and does not communicate its toggled state.

Under Vista/Win7 the QFB toggle button is using the same background as bug 670304. Under XP the scroll buttons, the QFB toggle button and the allTabs button are using the toolbarbutton appearance. I exchanged the QFB toggle button image with one who has a white outline to be still viewable on dark Aero- and personas backgrounds.

Andreas if you have a better icon, I can add this.

Richard, it is my understanding that Asa's suggested to move the QFB toggle off the tab bar completely and not just to change its style, thus allowing it for example to be moved to the main toolbar instead. This becomes relevant if the tab bar is hidden by default in the future if just a single tab is open (and assuming that Lightning provides an option to move its buttons onto the toolbar as well).

No, he wrote: This should be a toggle button... The moving/removing is handled in bug 582801

I see, the combination of both yields comment #4! (too many bugs around spread all over the place...)

(In reply to comment #2) > Created attachment 548811 [details] > QFB icon on a dark personas > > Andreas if you have a better icon, I can add this. I think a funnel icon would be nice here (to get across the idea of "filtering" rather than "searching"), but that could be done in another bug, too.

(In reply to comment #7) > I think a funnel icon would be nice here (to get across the idea of > "filtering" rather than "searching"), but that could be done in another bug, > too. But then this should be changed on all platforms to be consistent.

This patch should now be ready for bug 582801. I added normal Toolbar-button appearance when in main toolbar under Win7. The button has now a label (text only mode works now). I haven't checked the appearance under XP, Linux and Mac when QFB-button is in main toolbar. Fine-tuning should be made in a followup bug after landing of this bug and bug 582801.

I moved the all tabs- and scrollbutton changes for XP to bug 670304. This patch has now only the QFB part, but now also with additions for bug 582801 for all platforms. The QFB button behaves now on the normal tool bars like a normal toolbarbutton. Maybe the QFB button should become other icons for the normal tool bars because the icon dimensions are different and especially in 'icon and text' mode the button isn't right aligned.

New version because of bitrot in quickFilterBar-aero.css

Comment on attachment 553743 [details] [diff] [review] Remove the Tab appearance v4 The patch by itself looks a bit crazy with text below it and stuff, but makes more sense in combination with bug 670304 and bug 582801. I'm fine with this UI change as long as the two other bugs land within this cycle as well.

Comment on attachment 553743 [details] [diff] [review] Remove the Tab appearance v4 Review of attachment 553743 [details] [diff] [review]: ----------------------------------------------------------------- So, there aren't any real problems I could see with the code, but I do have a couple of big questions, so I'm going to say r- until I'm happy with the answers I get. Thanks, Blake. ::: mail/base/content/quickFilterBar.xul @@ +75,4 @@ > <hbox id="thunderbird-private-tabmail-buttons"> > <!-- gets hidden but keeps space allocation when not legal--> > <toolbarbutton id="qfb-show-filter-bar" type="checkbox" > + label="&quickFilterBar.qfbShowFilterBar.label;" This looks really odd on my copy of Thunderbird. I'm guessing that one of the other patches hides the text somehow? ::: mail/locales/en-US/chrome/messenger/quickFilterBar.dtd @@ +2,5 @@ > + The label for the button on the tab bar that toggles the visibility of > + the quick filter bar. > + --> > +<!ENTITY quickFilterBar.qfbShowFilterBar.label > + "Quick Filter Bar"> I think this should be "Quick Filter", to match the other text in the customization dialog. ::: mail/themes/gnomestripe/mail/quickFilterBar.css @@ +3,4 @@ > /* :::: Filter Tab Bar Button :::: */ > > #qfb-show-filter-bar { > + -moz-box-orient: vertical; Why is this necessary? ::: mail/themes/pinstripe/mail/quickFilterBar.css @@ +3,4 @@ > /* :::: Filter Tab Bar Button :::: */ > > #qfb-show-filter-bar { > + -moz-box-orient: vertical; I'm also a little confused as to why you're changing the Pinstripe and Gnomestripe in a Windows 7 bug…

(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #13) > Comment on attachment 553743 [details] [diff] [review] > Remove the Tab appearance v4 > > Review of attachment 553743 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: mail/base/content/quickFilterBar.xul > @@ +75,4 @@ > > <hbox id="thunderbird-private-tabmail-buttons"> > > <!-- gets hidden but keeps space allocation when not legal--> > > <toolbarbutton id="qfb-show-filter-bar" type="checkbox" > > + label="&quickFilterBar.qfbShowFilterBar.label;" > > This looks really odd on my copy of Thunderbird. I'm guessing that one of > the other patches hides the text somehow? This is because you wrote in bug 670304 it should work with bug 582801. So I made also this bug working with bug 582801. The button without text on the normal toolbar would look odd, especially in text only mode ;) > ::: mail/locales/en-US/chrome/messenger/quickFilterBar.dtd > @@ +2,5 @@ > > + The label for the button on the tab bar that toggles the visibility of > > + the quick filter bar. > > + --> > > +<!ENTITY quickFilterBar.qfbShowFilterBar.label > > + "Quick Filter Bar"> > > I think this should be "Quick Filter", to match the other text in the > customization dialog. I'll change this > ::: mail/themes/gnomestripe/mail/quickFilterBar.css > @@ +3,4 @@ > > /* :::: Filter Tab Bar Button :::: */ > > > > #qfb-show-filter-bar { > > + -moz-box-orient: vertical; > > Why is this necessary? Again to work normally on normal toolbars. > ::: mail/themes/pinstripe/mail/quickFilterBar.css > @@ +3,4 @@ > > /* :::: Filter Tab Bar Button :::: */ > > > > #qfb-show-filter-bar { > > + -moz-box-orient: vertical; > > I'm also a little confused as to why you're changing the Pinstripe and > Gnomestripe in a Windows 7 bug… I'd had to change the platform to All because of bug 582801 compatibility. What do you mean, should I leave the patch as it is, with the change in quickFilterBar.dtd, or make it only working in the tab toolbar (and then for Win only). When we leave this patch, we have to wait for the landing of bug 582801

Comment on attachment 553743 [details] [diff] [review] Remove the Tab appearance v4 (In reply to Richard Marti [:paenglab] from comment #14) > (In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #13) > > This looks really odd on my copy of Thunderbird. I'm guessing that one of > > the other patches hides the text somehow? > This is because you wrote in bug 670304 it should work with bug 582801. So I > made also this bug working with bug 582801. The button without text on the > normal toolbar would look odd, especially in text only mode ;) Sounds good. > > > #qfb-show-filter-bar { > > > + -moz-box-orient: vertical; > > Why is this necessary? > Again to work normally on normal toolbars. Okay. > > ::: mail/themes/pinstripe/mail/quickFilterBar.css > > I'm also a little confused as to why you're changing the Pinstripe and > > Gnomestripe in a Windows 7 bug… > I'd had to change the platform to All because of bug 582801 compatibility. Fair enough. > What do you mean, should I leave the patch as it is, with the change in > quickFilterBar.dtd, or make it only working in the tab toolbar (and then for > Win only). Leaving it as-is works for me. > When we leave this patch, we have to wait for the landing of bug 582801 Yeah, that sounds fair. I'm happy with your answers, so I'm changing it to an r+. :) In a related note, I'm seeing a few bugs that we're working on that are all kind of in the same area. It would be good to land these early in the cycle, and give ourselves some time to fix any bugs that pop up because of their interactions. Thanks, Blake.

Changed the label from "Quick Filter Bar" to "Quick Filter"

I checked what I could do with the invisible QFB button on not filterable tabs. This is made in quickFilterBar.js with giving visibilty: hidden. I tried with my limited JS knowledge but couldn't change it to opacity: 0.4 like the other tabbar buttons. I could oversteer this in CSS but this would be a hack. Cleaner would be if in quickFilterBar.js the state would be set to inactive and then in CSS the needed styling is set.

Comment on attachment 555443 [details] [diff] [review] Patch for check-in I'm going to change this to r-, because of a conversation in bug 670304, comment 18, where we say: > > 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. So I'ld like to see the quick-filter bar icon be greyed out when it's on a tab which doesn't support the quick-filter bar in this patch. (Check the behaviour of the Copy button in the tab bar in Aurora to see what I'm looking for.) Thanks, Blake.

QFB button not hidden on tabs without filter functionality but with disabled appearance. Under Windows I added also a inverted icon for Glass background and dark personas. This patch needs still bug 582801 for correct functionality.

Comment on attachment 556395 [details] [diff] [review] Remove the Tab appearance v5 >+++ b/mail/themes/qute/jar.mn >@@ -248,6 +248,7 @@ > skin/classic/messenger/icons/filter.png (mail/icons/filter.png) >+ skin/classic/messenger/icons/filter-inverted.png (mail/icons/filter-inverted.png) >@@ -462,6 +463,7 @@ > skin/classic/aero/messenger/icons/filter.png (mail/icons/filter.png) >+ skin/classic/aero/messenger/icons/filter-inverted.png (mail/icons/filter-inverted.png) Don't we need lines like these for pinstripe and gnomestripe, too? There's no icon for the button on the Mac or Windows when I drag the QFB into the customization pane. I can't really give the ui-r this patch, cause the QFB toggle button is so different from any other button that we can put into the tab, as shown in <http://dl.dropbox.com/u/2301433/Toolbar/QfbHighlightAero.png> amd <http://dl.dropbox.com/u/2301433/Toolbar/QfbHighlightClassic.png> It doesn't get a blue glow when it's highlit. It doesn't look like a button. The icon is bright white instead of dark grey. On the plus side, it looks just like all the other buttons when it's in a toolbar, which makes me hopeful that we can get it looking like all the other buttons in the tab bar, too. The code is pretty clean, aside from the bit I mention above, but I'm guessing that the changes are going to be large enough that I'm going to want to re-review it before it lands, so I'm going to say r- as well. Sorry about that, Blake.

(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #20) > Comment on attachment 556395 [details] [diff] [review] > Remove the Tab appearance v5 > > >+++ b/mail/themes/qute/jar.mn > >@@ -248,6 +248,7 @@ > > skin/classic/messenger/icons/filter.png (mail/icons/filter.png) > >+ skin/classic/messenger/icons/filter-inverted.png (mail/icons/filter-inverted.png) > >@@ -462,6 +463,7 @@ > > skin/classic/aero/messenger/icons/filter.png (mail/icons/filter.png) > >+ skin/classic/aero/messenger/icons/filter-inverted.png (mail/icons/filter-inverted.png) > > Don't we need lines like these for pinstripe and gnomestripe, too? I can use the same icons for Mac and Linux, if it's okay. > > There's no icon for the button on the Mac or Windows when I drag the QFB > into the customization pane. I'm looking in this. > I can't really give the ui-r this patch, cause the QFB toggle button is so > different from any other button that we can put into the tab, as shown in > <http://dl.dropbox.com/u/2301433/Toolbar/QfbHighlightAero.png> amd > <http://dl.dropbox.com/u/2301433/Toolbar/QfbHighlightClassic.png> > > It doesn't get a blue glow when it's highlit. It doesn't look like a > button. The icon is bright white instead of dark grey. Now I'm a little bit confused. To look the same as Firefox, no button appearance should be shown in the tab bar as the QFB button is with this bug (and for the other buttons with bug 670304). Do you want the buttons like <http://dl.dropbox.com/u/2301433/Toolbar/QfbHighlightAero.png> or like Firefox?

(In reply to Richard Marti [:paenglab] from comment #21) > (In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #20) > > >+++ b/mail/themes/qute/jar.mn > > >@@ -248,6 +248,7 @@ > > > skin/classic/messenger/icons/filter.png (mail/icons/filter.png) > > >+ skin/classic/messenger/icons/filter-inverted.png (mail/icons/filter-inverted.png) > > >@@ -462,6 +463,7 @@ > > > skin/classic/aero/messenger/icons/filter.png (mail/icons/filter.png) > > >+ skin/classic/aero/messenger/icons/filter-inverted.png (mail/icons/filter-inverted.png) > > > > Don't we need lines like these for pinstripe and gnomestripe, too? > I can use the same icons for Mac and Linux, if it's okay. I _think_ that should be okay… > > I can't really give the ui-r this patch, cause the QFB toggle button is so > > different from any other button that we can put into the tab, as shown in > > <http://dl.dropbox.com/u/2301433/Toolbar/QfbHighlightAero.png> amd > > <http://dl.dropbox.com/u/2301433/Toolbar/QfbHighlightClassic.png> > > > > It doesn't get a blue glow when it's highlit. It doesn't look like a > > button. The icon is bright white instead of dark grey. > > Now I'm a little bit confused. To look the same as Firefox, no button > appearance should be shown in the tab bar as the QFB button is with this bug > (and for the other buttons with bug 670304). > Do you want the buttons like > <http://dl.dropbox.com/u/2301433/Toolbar/QfbHighlightAero.png> or like > Firefox? So, my problem is that we have a few different buttons with a few different styles. I'm happy to follow Firefox's styling, but we should have all the buttons look the same, not some looking one way and others looking a different way. Thanks, Blake.

(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #22) > > So, my problem is that we have a few different buttons with a few different > styles. I'm happy to follow Firefox's styling, but we should have all the > buttons look the same, not some looking one way and others looking a > different way. Then I'm using everywhere the toolbarbutton styling. I change also bug 670304 to use this styling for toolbarbuttons in the Tab bar (and I don't have to wait for inverted icons because they are no more needed).

That sounds fine. If we want to change the styling of everything later, we can. Thanks!

This patch adds: - QFB button Toolbarbutton appearance on the Tab bar under Windows. - Inverted QFB button icons for personas under XP, Linux and Mac. - ready for bug 582801. - QFB Icon shown in Customize window on all themes. - With active tabs without QFB functionality, QFB button looks like inactive Toolbarbutton instead invisible on all themes.

Removed code which belongs to bug 670304. The leftover is the same: This patch adds: - QFB button Toolbarbutton appearance on the Tab bar under Windows. - Inverted QFB button icons for personas under XP, Linux and Mac. - ready for bug 582801. - QFB Icon shown in Customize window on all themes. - With active tabs without QFB functionality, QFB button looks like inactive Toolbarbutton instead invisible on all themes.

Comment on attachment 558131 [details] [diff] [review] Remove the Tab appearance v7 Review of attachment 558131 [details] [diff] [review]: ----------------------------------------------------------------- I'm mostly happy, so here's what I'm going to do. ui-r = me, but don't land this bug until we've fixed all the issues I mention in bug 687836. And the code looks pretty good, so I'm going to say r=me, too. ::: mail/themes/qute/mail/quickFilterBar-aero.css @@ -60,3 +54,4 @@ > > -#qfb-show-filter-bar:hover { > > +#qfb-show-filter-bar:not([style="visibility: hidden;"]):hover, > > - background-image: -moz-linear-gradient(left, transparent, transparent 1px, > > +#qfb-show-filter-bar:not([style="visibility: hidden;"]):hover:active, > > - rgba(255, 255, 255, .6) 1px, rgba(255, 255, 255, .6)); > > +#qfb-show-filter-bar:not([style="visibility: hidden;"])[checked], > > +#qfb-show-filter-bar:not([style="visibility: hidden;"])[checked]:hover { Shouldn't this last selector be covered by one of the ":hover" or "[checked]" rules above?

Sorry to add a new patch but this one has now really a toolbarbutton appearance for the QFB button also under Aero on the tab bar like all buttons in the new tab toolbar. I only changed quickFilterBar-aero.css and also removed the selector you noted in the review (the code is now less complex).

Comment on attachment 561219 [details] [diff] [review] Remove the Tab appearance v8 (You could have carried over the r+ and ui-r+ from the previous patch, because we'll be fixing the remaining issues in bug 687836. ;)

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.