714337 - quickfilter bar doesn't remember its icon button state

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, defect)

Description

[Asa Dotzler [:asa]]

Attachment: 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) (:☕️)]

Mike, could this be fixed by an addition to the persistence cleanup work you did?

Comment 2

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

Unsure, but I can fiddle with it.

Comment 3

[Maarten Brouwers]

Confirmed, also on Mac OS-X

Comment 4

[Ludovic Hirlimann [:Usul]]

I just saw it on Aurora.

Comment 5

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

Attachment: 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

[Rain [:rain1] (inactive)]

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) (:⚙️)]

Sid:

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

-Mike

Comment 8

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

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) (:⚙️)]

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) (:⚙️)]

Attachment: 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?

-Mike

Comment 11

[Andrew Sutherland [:asuth] (he/him)]

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:
mail/test/mozmill/quick-filter-bar/test-display-issues.js

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]

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) (:⚙️)]

heyix:

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

Feel like hacking on the patch?

-Mike

Comment 14

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

heyix:

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

-Mike

Comment 15

[heyix]

Hi Mike,

sounds good - many thanks.

heyix

Comment 16

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

Attachment: Tests v1

Comment 17

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

Attachment: 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]]

You have made my week, maybe my month. I curse this several times a day.

Comment 19

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

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) (:⚙️)]

Comment on attachment 600093 [details] [diff] [review]
Tests v2

Same as above.

Comment 21

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

Asa:

Glad to hear it!  :D

-Mike

Comment 22

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

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) (:⚙️)]

Attachment: 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) (:⚙️)]

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) (:⚙️)]

Committed to comm-central as http://hg.mozilla.org/comm-central/rev/b8bdec7166ad

Comment 26

[Mark Banner (:standard8)]

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 | (runtestlist.py) | 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 | (runtestlist.py) | 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 | (runtestlist.py) | 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 | (runtestlist.py) | Exited with code 1 during directory run

Comment 27

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

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) (:⚙️)]

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) (:⚙️)]

The problem seems to have been resolved on our testing machines.

Comment 30

[Mark Banner (:standard8)]

Landed on branches:

http://hg.mozilla.org/releases/comm-aurora/rev/2fd019398510
http://hg.mozilla.org/releases/comm-beta/rev/0247b9da3bd1

Comment 32

[ac.ekin]

I wonder when the end users can expect to see this resolved? Not knowing any better, I started this on the following location:
http://bit.ly/xCEAL1

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) (:⚙️)]

ac.ekin:

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:

http://www.mozilla.org/en-US/thunderbird/channel/

All the best,

-Mike

Comment 34

[ac.ekin]

Thank you! 8-) Happy again.