Last Comment Bug 714337 - quickfilter bar doesn't remember its icon button state
: quickfilter bar doesn't remember its icon button state
Product: Thunderbird
Classification: Client Software
Component: Mail Window Front End (show other bugs)
: 12 Branch
: x86_64 Windows 7
: -- normal with 1 vote (vote)
: Thunderbird 13.0
Assigned To: Mike Conley (:mconley)
: 636445 (view as bug list)
Depends on:
  Show dependency treegraph
Reported: 2011-12-30 12:09 PST by Asa Dotzler [:asa]
Modified: 2012-03-08 11:19 PST (History)
10 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---

quickfilter bar reverting to icon and text when it should not (47.57 KB, image/jpeg)
2011-12-30 12:09 PST, Asa Dotzler [:asa]
no flags Details
Patch v1 (1.16 KB, patch)
2012-01-09 14:01 PST, Mike Conley (:mconley)
bwinton: ui‑review+
Details | Diff | Splinter Review
Patch v2 (1.56 KB, patch)
2012-01-12 12:48 PST, Mike Conley (:mconley)
bugmail: review+
standard8: approval‑comm‑aurora+
standard8: approval‑comm‑beta+
Details | Diff | Splinter Review
Tests v1 (3.90 KB, patch)
2012-02-23 10:32 PST, Mike Conley (:mconley)
no flags Details | Diff | Splinter Review
Tests v2 (4.12 KB, patch)
2012-02-23 10:58 PST, Mike Conley (:mconley)
bugmail: review+
Details | Diff | Splinter Review
Tests v3 (r+'d by asuth) (3.72 KB, patch)
2012-02-23 12:00 PST, Mike Conley (:mconley)
standard8: approval‑comm‑aurora+
standard8: approval‑comm‑beta+
Details | Diff | Splinter Review

Description Asa Dotzler [:asa] 2011-12-30 12:09:09 PST
Created attachment 585016 [details]
quickfilter bar reverting to icon and text when it should not

I run in the vertical view layout and the quickfilter bar often fails to remember that its buttons should be icons and not icons and labels. This pushes the filter box out of view some of the time. It's really annoying and I cannot figure out what triggers it. Every time I restart Thunderbird, I have to fiddle with the width of the message pane to re-trigger the icon only mode to get the filter text box back in view. See screenshot.
Comment 1 Blake Winton (:bwinton) (:☕️) 2011-12-30 14:45:29 PST
Mike, could this be fixed by an addition to the persistence cleanup work you did?
Comment 2 Mike Conley (:mconley) 2012-01-04 11:29:18 PST
Unsure, but I can fiddle with it.
Comment 3 Maarten Brouwers 2012-01-07 11:35:56 PST
Confirmed, also on Mac OS-X
Comment 4 Ludovic Hirlimann [:Usul] 2012-01-09 05:53:07 PST
I just saw it on Aurora.
Comment 5 Mike Conley (:mconley) 2012-01-09 14:01:22 PST
Created attachment 587121 [details] [diff] [review]
Patch v1

Wow, that was simpler than expected.

So, the QFB buttons default to full icons and text when TB starts up fresh for the first time.  If the window is shrunk enough, a "shrunk" attribute on the element holding the buttons is set to true.

We're now persisting that flipped attribute.  I tried it locally on Win 7, and it seemed to work alright.
Comment 6 Siddharth Agarwal [:sid0] (inactive) 2012-01-09 14:58:57 PST
Why does this bug not show up with the default classic message view even after the window's been shrunk and Thunderbird's restarted, and why can't we do the same thing?
Comment 7 Mike Conley (:mconley) 2012-01-10 09:58:38 PST

Hm, using the classic message view, I'm still seeing the problem in TB 11.

Comment 8 Blake Winton (:bwinton) (:☕️) 2012-01-11 13:07:29 PST
Comment on attachment 587121 [details] [diff] [review]
Patch v1

(I don't _think_ we need ui-review for making Thunderbird start up in the same state that it closed in, but I'll say ui-r=me, if it makes you feel better. ;)
Comment 9 Mike Conley (:mconley) 2012-01-12 06:49:06 PST
Comment on attachment 587121 [details] [diff] [review]
Patch v1

Hm, so after testing this for a few days, I keep seeing the bug crop up.

So, not as easy as I originally thought.  I'll keep digging.
Comment 10 Mike Conley (:mconley) 2012-01-12 12:48:45 PST
Created attachment 588159 [details] [diff] [review]
Patch v2

So it looks like an attempt was already made to fix this problem, but we were calling "node.onoverflow();", and "node.onresize()", which I think is not actually possible.

Thus, the situation this bug describes also results in an error in the Error Console stating, "node.onoverflow is not a function".

The was the best solution I could find to fire the onoverflow and onresize events.  It sure beats eval.

asuth:  Am I overlooking anything here?

Comment 11 Andrew Sutherland [:asuth] 2012-01-12 15:53:01 PST
Comment on attachment 588159 [details] [diff] [review]
Patch v2

If we can't call onresize(), will the test for 'onresize' work?

I think this should be promoted to mozmill tested since this is fairly voodoo and I thought I had done it correctly in the first place (but apparently I did not, or something regressed it, etc.)  The tests for the on-the-fly stuff happens in:

I think you should be able to add a test that creates a new 3-pane window of a given size and make sure the initial state is correct (after waiting for our hideous sleep to complete :(
Comment 12 heyix 2012-02-23 03:51:37 PST
I've got the same issue - and find it really annoying like asa. I see it's still in progress. Will it be fixed in TB 11?
Comment 13 Mike Conley (:mconley) 2012-02-23 06:27:25 PST

Unfortunately, no, I don't think this patch will make it for TB 11.

Feel like hacking on the patch?

Comment 14 Mike Conley (:mconley) 2012-02-23 09:21:10 PST

I spoke too soon!  I've just been re-prioritized onto this bug. :)

Comment 15 heyix 2012-02-23 10:15:26 PST
Hi Mike,

sounds good - many thanks.

Comment 16 Mike Conley (:mconley) 2012-02-23 10:32:34 PST
Created attachment 600082 [details] [diff] [review]
Tests v1
Comment 17 Mike Conley (:mconley) 2012-02-23 10:58:01 PST
Created attachment 600093 [details] [diff] [review]
Tests v2

These tests pass for me when Patch v2 is applied, and fail when it's reverted.

Look ok?
Comment 18 Asa Dotzler [:asa] 2012-02-23 11:14:18 PST
You have made my week, maybe my month. I curse this several times a day.
Comment 19 Mike Conley (:mconley) 2012-02-23 11:16:55 PST
Comment on attachment 588159 [details] [diff] [review]
Patch v2

As discussed during Product Council, we want this for TB 11.
Comment 20 Mike Conley (:mconley) 2012-02-23 11:17:15 PST
Comment on attachment 600093 [details] [diff] [review]
Tests v2

Same as above.
Comment 21 Mike Conley (:mconley) 2012-02-23 11:17:42 PST

Glad to hear it!  :D

Comment 22 Mike Conley (:mconley) 2012-02-23 11:37:54 PST
Comment on attachment 600093 [details] [diff] [review]
Tests v2

These tests fail on Linux because when seeing if the icons are expanded if the message pane is small.  This is because it seems that our threshold for "small" is different on Linux than on OSX or Windows, and I believe we're restricted on the resolution for our Linux test builders.

The interesting test case is for when the window spawns, and the message pane is *large*, and ensuring that the icons are collapsed.  So I think, due to time constraints, I am going to remove the superfluous test where we ensure that the icons expand if the message pane is small.

So, for now, reverting my a? requests until I have the new patch up.
Comment 23 Mike Conley (:mconley) 2012-02-23 12:00:34 PST
Created attachment 600124 [details] [diff] [review]
Tests v3 (r+'d by asuth)

Removing uninteresting and problematic-for-Linux test case.  Carrying forward r+ from asuth.

I'll run these tests locally again on my Windows / OSX boxes before re-requesting approval for aurora/beta.
Comment 24 Mike Conley (:mconley) 2012-02-23 12:11:56 PST
Comment on attachment 600124 [details] [diff] [review]
Tests v3 (r+'d by asuth)

These tests act as suspected, and now pass on all three platforms (locally).
Comment 25 Mike Conley (:mconley) 2012-02-23 12:16:52 PST
Committed to comm-central as
Comment 26 Mark Banner (:standard8, afk until Dec) 2012-02-24 02:24:20 PST
I think this broke some MozMill tests on Mac 10.6:

TEST-UNEXPECTED-FAIL | /builds/slave/comm-central-macosx64-opt-unittest-mozmill/build/mozmill/attachment/test-attachment-menus.js | test-attachment-menus.js::test_multiple_attachments_one_detached
TEST-UNEXPECTED-FAIL | /builds/slave/comm-central-macosx64-opt-unittest-mozmill/build/mozmill/attachment/test-attachment-menus.js | test-attachment-menus.js::test_multiple_attachments_all_detached
TEST-UNEXPECTED-FAIL | ( | Exited with code 1 during directory run
TEST-UNEXPECTED-FAIL | /builds/slave/comm-central-macosx64-opt-unittest-mozmill/build/mozmill/message-header/test-message-header.js | test-message-header.js::test_toolbar_collapse_and_expand
TEST-UNEXPECTED-FAIL | ( | Exited with code 1 during directory run
TEST-UNEXPECTED-FAIL | /builds/slave/comm-central-macosx64-opt-unittest-mozmill/build/mozmill/quick-filter-bar/test-display-issues.js | test-display-issues.js::test_buttons_collapse_and_expand
TEST-UNEXPECTED-FAIL | /builds/slave/comm-central-macosx64-opt-unittest-mozmill/build/mozmill/quick-filter-bar/test-display-issues.js | test-display-issues.js::test_buttons_collapse_and_expand_on_spawn_in_vertical_mode
TEST-UNEXPECTED-FAIL | /builds/slave/comm-central-macosx64-opt-unittest-mozmill/build/mozmill/quick-filter-bar/test-display-issues.js | test-display-issues.js::teardownModule
TEST-UNEXPECTED-FAIL | ( | Exited with code 1 during directory run
TEST-UNEXPECTED-FAIL | /builds/slave/comm-central-macosx64-opt-unittest-mozmill/build/mozmill/session-store/test-session-store.js | test-session-store.js::test_message_pane_width_persistence
TEST-UNEXPECTED-FAIL | ( | Exited with code 1 during directory run
Comment 27 Mike Conley (:mconley) 2012-02-24 06:47:42 PST
It seems to be intermittent.  I'm working with jhopkins now to determine if one of the Mac 10.6 test machines has an incorrect screen resolution.
Comment 28 Mike Conley (:mconley) 2012-02-24 06:59:31 PST
Alright, looks like we had some screen resolution discrepancies.  jhopkins is on the case to fix that.

I'll wait for a few more builds to land on trunk today to make sure that the problem is resolved.
Comment 29 Mike Conley (:mconley) 2012-02-24 09:05:26 PST
The problem seems to have been resolved on our testing machines.
Comment 30 Mark Banner (:standard8, afk until Dec) 2012-02-28 10:23:14 PST
Landed on branches:
Comment 31 Wayne Mery (:wsmwk, NI for questions) 2012-03-01 13:15:14 PST
*** Bug 636445 has been marked as a duplicate of this bug. ***
Comment 32 ac.ekin 2012-03-08 11:14:23 PST
I wonder when the end users can expect to see this resolved? Not knowing any better, I started this on the following location:

Someone suggested that I should report it here. I see it as "resolved" but obviously not for me. Is the ETA Thunderbird 13? Really?
Comment 33 Mike Conley (:mconley) 2012-03-08 11:17:12 PST

Hey - this was patched for Thunderbird 13, but then backported to Thunderbird 12 (EarlyBird) and Thunderbird 11 (Beta).

So this fix should be in your hands when Thunderbird 11 is released next week.

Or, if you can't wait that long, you can try Beta or EarlyBird:

All the best,

Comment 34 ac.ekin 2012-03-08 11:19:29 PST
Thank you! 8-) Happy again.

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