Last Comment Bug 633679 - Quick filter button/menuitem shouldn't be invisible when disabled
: Quick filter button/menuitem shouldn't be invisible when disabled
Status: RESOLVED FIXED
:
Product: Thunderbird
Classification: Client Software
Component: Toolbars and Tabs (show other bugs)
: Trunk
: All All
: -- trivial (vote)
: Thunderbird 12.0
Assigned To: Richard Marti (:Paenglab)
:
:
Mentors:
: 717033 735807 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-02-11 18:25 PST by Jim Porter (:squib)
Modified: 2012-03-14 12:27 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Inverse button order (2.76 KB, image/png)
2011-02-12 00:08 PST, Richard Marti (:Paenglab)
no flags Details
opacity instead of invisible (2.20 KB, image/png)
2011-02-12 00:13 PST, Richard Marti (:Paenglab)
no flags Details
blank space instead of menu entry "quick filter bar" (35.89 KB, image/png)
2011-06-28 13:02 PDT, Kuno Meyer
no flags Details
Partial fix (2.06 KB, patch)
2011-12-30 17:11 PST, Jim Porter (:squib)
richard.marti: feedback+
Details | Diff | Splinter Review
Make QFB button a toolbarbutton-1 (5.53 KB, patch)
2011-12-31 05:44 PST, Richard Marti (:Paenglab)
no flags Details | Diff | Splinter Review
Fix menu item observation (731 bytes, patch)
2011-12-31 13:13 PST, Jim Porter (:squib)
no flags Details | Diff | Splinter Review
Make QFB button a toolbarbutton-1 (13.21 KB, patch)
2012-01-06 14:19 PST, Richard Marti (:Paenglab)
mconley: review+
mconley: ui‑review+
Details | Diff | Splinter Review
Depressed QFB button (9.61 KB, image/png)
2012-01-09 13:02 PST, Mike Conley (:mconley)
no flags Details
Test fixes (2.75 KB, patch)
2012-01-11 08:37 PST, Mike Conley (:mconley)
sid.bugzilla: review+
Details | Diff | Splinter Review
Aggregated patch (r,ui-r=mconley, r=sid0) (20.27 KB, patch)
2012-01-24 08:09 PST, Mike Conley (:mconley)
no flags Details | Diff | Splinter Review

Description Jim Porter (:squib) 2011-02-11 18:25:38 PST
When you can't activate the quick filter bar, the tab bar button for it is hidden via visibility:hidden. I assume this is intentional (so the tabs don't expand/shrink when you switch to/from a tab where the qfb is useless), but this has two problems:

1) When looking at Lightning tabs, there's a big empty space where the qfb
   button would be, which looks weird.
2) The visilibity:hidden style appears to be applied to the menuitem in View ->
   Toolbar as well, so there's an invisible row. Based on the comments, it looks
   like this should actually appear as disabled, not invisible.

I'm not sure what the best way to fix (1) would be, but you could probably fix (2) by setting an attribute on the button and then use a CSS selector to make it invisible, which would hopefully not mess with the menuitem. I'm not totally sure this would work though, since I think the problem is some strangeness with the observes attribute on the menuitem.
Comment 1 Richard Marti (:Paenglab) 2011-02-12 00:08:47 PST
Created attachment 511926 [details]
Inverse button order

In my theme I changed the button order. First comes the QFB button then the lightning buttons. With this I have no gap between the lightning buttons and the tabs-alltabs-button, when the QFB button is invisible.

Only when the tab bar is full of tabs, the gap is visible.
Comment 2 Richard Marti (:Paenglab) 2011-02-12 00:13:41 PST
Created attachment 511927 [details]
opacity instead of invisible

Instead of visibility:hidden using opacity:0.3 would fill the gap, but show it's not active.
Comment 3 Kuno Meyer 2011-06-28 13:02:33 PDT
Created attachment 542564 [details]
blank space instead of menu entry "quick filter bar"

I guess this also applies to the menu item "View->Tool Bars->Quick Filter Bar". (See screenshot)
Comment 4 Kuno Meyer 2011-06-28 13:04:42 PDT
By the way: this is from the just-released Thunderbird 5.0
Comment 5 Jim Porter (:squib) 2011-12-30 16:39:55 PST
So now (on comm-central), the quick filter icon is disabled properly, but the menu-item is still invisible. We should fix that.
Comment 6 Jim Porter (:squib) 2011-12-30 17:11:50 PST
Created attachment 585086 [details] [diff] [review]
Partial fix

Here's a fix for the Javascript code and the GNOME theme. I'm not totally sure what's going on in the other themes, though. Do you mind taking a look, Richard?
Comment 7 Richard Marti (:Paenglab) 2011-12-31 05:23:35 PST
Comment on attachment 585086 [details] [diff] [review]
Partial fix

Great! This is what I tried to do but failed miserably with my JS knowledge.

This makes the menuitem visible in disabled state.
Comment 8 Richard Marti (:Paenglab) 2011-12-31 05:44:15 PST
Created attachment 585130 [details] [diff] [review]
Make QFB button a toolbarbutton-1

This additional patch makes the QFB button a toolbarbutton-1 like all other buttons. With this we could get rid of a lot of CSS code (especially under Aero) which is imitating the toolbarbutton-1. I added also the changes for Aero to show what could be gained.

Unfortunately this makes the menuitem also a toolbarbutton-1. I found this comes from the observes="qfb-show-filter-bar".

Squib, do you know how this problem could be resolved?
Comment 9 Jim Porter (:squib) 2011-12-31 13:13:39 PST
Created attachment 585160 [details] [diff] [review]
Fix menu item observation

This patch, applied on top of my original, should fix the issue with class="toolbarbutton-1" being copied onto the menuitem (it also fixes a related issue where the menuitem's label is replaced, from "Quick Filter Bar" to just "Quick Filter").
Comment 10 Richard Marti (:Paenglab) 2012-01-06 14:19:28 PST
Created attachment 586567 [details] [diff] [review]
Make QFB button a toolbarbutton-1

This is now the full patch. The QFB button is now a real toolbarbutton-1. Instead of the visibility=hidden state the button and the menuitem have now the disabled state.
Comment 11 Mike Conley (:mconley) 2012-01-09 12:12:34 PST
This looks and works nicely on Windows 7.  Testing on XP, Linux and OSX next...
Comment 12 Mike Conley (:mconley) 2012-01-09 12:48:48 PST
Seems to fix the problem on OSX as well - though I have to ask, is the QFB button really supposed to look the way it does when clicked on?  See forthcoming screenshot...
Comment 13 Mike Conley (:mconley) 2012-01-09 13:02:42 PST
Created attachment 587094 [details]
Depressed QFB button
Comment 14 Mike Conley (:mconley) 2012-01-09 13:23:48 PST
Ok, another thing I've noticed:

On Windows XP and OSX, if the QFB Button is depressed, and I switch to another tab (like the Addon Manager), the QFB button is (correctly) disabled, but the state of the QFB Button is not displayed.

This is not consistent with what we do on Linux and Windows 7.  There, if the QFB Button is depressed, it is clear that it is depressed even when switching to other tabs.

I think I prefer consistency whenever possible...so, assuming we want all of these to agree, which way should we go?  Show or hide the depressed state?
Comment 15 Richard Marti (:Paenglab) 2012-01-09 14:10:34 PST
This patch makes the QFB button a standard toolbar-button. This means the styling is defined in toolkit and only for Aero in primaryToolbar.css.

It looks like not all systems have a disabled depressed state.

I could add special styles for this but I think bwinton should decide what we need and how this should look.
Comment 16 Mike Conley (:mconley) 2012-01-10 12:02:36 PST
Paenglab:

So bwinton and I just talked about this on IRC, and I think what we'd like is that if the currently selected tab is not the mail tab, the QFB button should be disabled and not depressed.

Can this be done?

-Mike
Comment 17 Mike Conley (:mconley) 2012-01-10 12:31:11 PST
Comment on attachment 586567 [details] [diff] [review]
Make QFB button a toolbarbutton-1

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

Code looks good, and assuming that the depressed state thing gets fixed by the combination of this bug and bug 715495, r/ui-r=me.  Thanks for your work!

-Mike
Comment 18 Jim Porter (:squib) 2012-01-10 13:35:53 PST
If this is getting checked in, we should probably file a followup to fix the checked-and-disabled state in Linux (bug 715495 should cover Win7).
Comment 19 Jim Porter (:squib) 2012-01-10 14:32:36 PST
*** Bug 717033 has been marked as a duplicate of this bug. ***
Comment 20 Mike Conley (:mconley) 2012-01-10 14:40:59 PST
Checked in to comm-central as http://hg.mozilla.org/comm-central/rev/9f829c7a28d5
Comment 21 Mark Banner (:standard8, limited time in Dec) 2012-01-11 00:14:05 PST
Unfortunately this broke unit tests so I backed it out:

http://hg.mozilla.org/comm-central/rev/1b02f7bd7a43
Comment 22 Mike Conley (:mconley) 2012-01-11 08:37:19 PST
Created attachment 587711 [details] [diff] [review]
Test fixes

Whoops - I forgot to take into account the Mozmill tests.  :/  Sorry about that.

This patch fixes those failures that caused the backout.
Comment 23 Mike Conley (:mconley) 2012-01-11 09:12:14 PST
Comment on attachment 587711 [details] [diff] [review]
Test fixes

These tests now pass locally on Windows 7.
Comment 24 Siddharth Agarwal [:sid0] (inactive) 2012-01-16 09:33:30 PST
Comment on attachment 587711 [details] [diff] [review]
Test fixes

r+ assuming tests pass.
Comment 25 Richard Marti (:Paenglab) 2012-01-17 04:11:46 PST
I hope it's okay to set checkin-needed with the r+ of the test fixes.
Comment 26 Mike Conley (:mconley) 2012-01-19 10:45:36 PST
Paenglab:

Hey - the patch appears to have bitrotted against pinstripe/mail/quickFilterBar.css.  I manually corrected it, and I'm doing a try-build here:  http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=39c484518986

Can you take a look at my manual fix for quickFilterBar.css and ensure I did the right thing?  http://hg.mozilla.org/try-comm-central/rev/39c484518986#l6.1

Assuming yes, and assuming the try build passes without an issue, I'll commit this.

-Mike
Comment 27 Richard Marti (:Paenglab) 2012-01-19 15:11:56 PST
Your fix is looking good. For me it's okay to commit if the build passes.
Comment 28 Mike Conley (:mconley) 2012-01-24 08:09:33 PST
Created attachment 591100 [details] [diff] [review]
Aggregated patch (r,ui-r=mconley, r=sid0)

Sorry, this one slipped off the radar for a few days.

This is an aggregated, un-bitrotted patch.
Comment 29 Mike Conley (:mconley) 2012-01-24 08:10:45 PST
Committed to comm-central as http://hg.mozilla.org/comm-central/rev/318d0671f881
Comment 30 Jim Porter (:squib) 2012-03-14 12:27:28 PDT
*** Bug 735807 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.