563428 - Quick Filter Bar does not auto-collapse at startup when vertical layout is in use

Status: RESOLVED FIXED

(Thunderbird :: Search, defect)

Description

[Asa Dotzler [:asa]]

Attachment: quickfilter toolbar busticated in vertical layout

If you have a vertical view layout in trunk thunderbird and you're not on a giant screen, the QuickFilter UI is pretty busticated. 

To test, open the thread pane pretty wide until you can see all of the QuickFilter toolbar's UI.  Then start dragging the separator between threadpane and message pane left to shrink the QuickFilter toolbar's width.

1. the collapse labels to icons happens way too late (and doesn't persist after a restart.)
2. the space between the buttons and the search field is fixed and should collapse long before the search field starts collapsing (as you shrink the available space) If the space isn't there to show the "no results" or "xx messages" strings, then that text should just be dropped or should temporarily replace all the quickfilter buttons .
3. the "x" clear search in the search box slips out of the right side of the search box at some wrong arbitrary spot. The x should be pinned right and the search text should be obscured as the space shrinks, not the clear button -- ever.

Comment 1

[Ludovic Hirlimann [:Usul]]

on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2.5pre) Gecko/20100427 Thunderbird/3.1b2 , when I play and resize to a smaller size (which makes attachment disapear) and then resize to the orginal size , we are missing a repaint( ie the attachement filter doesn't reappear).

Comment 2

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

It looks like on startup we need to cause whatever is causing the DOM node transplantation to restore the vertical layout to ensure that it generates either the events it should be generating or a synthetic event we can hook.  (Or just have it explicitly invoke the muxer code, why not.)

I suspect #3 is a mac-ism relating to the display-type stuff that makes the search box look the way that it does; the 'x' button is likely not part of our computations and so we don't resize until it completely is off screen.  We're in the midst of an office move and all my macs are in boxes, so that part will take a day or two.  This would likely explain the non-restart complaint in #1.

In terms of #2, prototype patches accompanied by screenshots would be accepted for consideration.  The devil is in the XUL details on that one.

Ludo, I need a screenshot or screenshots from you for what you are describing; I'm not grokking your description.

Comment 3

[Ludovic Hirlimann [:Usul]]

(In reply to comment #2)
 
> Ludo, I need a screenshot or screenshots from you for what you are describing;
> I'm not grokking your description.

Can't reproduce right now so I'll make sure to take a screenshot When I do reproduce that particular thing I saw.

Comment 4

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

(In reply to comment #2)
> It looks like on startup we need to cause whatever is causing the DOM node
> transplantation to restore the vertical layout to ensure that it generates
> either the events it should be generating or a synthetic event we can hook. 
> (Or just have it explicitly invoke the muxer code, why not.)

We are getting screwed by XUL/XBL here, as per usual.  I have a workaround in my patch that generates an ugly synthetic notification to counter the lies we are told.

Blake, since you are the message reader owner and this is interaction with that, more or less, you get the review! :)


> I suspect #3 is a mac-ism relating to the display-type stuff that makes the
> search box look the way that it does; the 'x' button is likely not part of our
> computations and so we don't resize until it completely is off screen.  We're
> in the midst of an office move and all my macs are in boxes, so that part will
> take a day or two.  This would likely explain the non-restart complaint in #1.

I clearly misunderstood the complaint here, and somehow thought you were on OS X.  The problem is just that if we maintain the minwidth attribute after collapsing then the text box will never flex.  My fix is to remove the minwidth attribute when we shrink the buttons so it will flex.

clarkbw, this is a heads-up to you that I am doing this; speak up if you find this contentious but I'm not gonna request ui-review since it's just a failure mode right now.

Ideally, we would have a second tier of collapsing where the "Quick Filter:" label could collapse before we start flexing the text box.  I'm going to have to punt on that due to complexity and time concerns.

Comment 5

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

Attachment: v1 collapse mo better

The two things happening here and why no test changes.

1) The horrible kludge for the vertical layout at startup.  This would require restart testing or its own test dir and generally would be much more effort than we have historically expended on the vertical layout.

2) Turning off minwidth on collapse.  This is a straightforward layout choice change and its functionality is already covered by the general collapsing logic coverage.

Comment 6

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

Comment on attachment 443851 [details] [diff] [review]
v1 collapse mo better

Given that this amounts to annoying breakage of the quick filter bar in a disturbingly popular layout mode and I have a patch, I suggest we plan to land it for 3.1 (noting that a patch revision may happen based on the review).

Comment 8

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

Comment on attachment 443851 [details] [diff] [review]
v1 collapse mo better

>+++ b/mail/base/content/msgMail3PaneWindow.js
>@@ -244,16 +244,47 @@ function UpdateMailPaneConfig(aMsgWindow
>+    //  (clientWidth = 1036).  It would appear that when the messagepane's XBL
>+    //  binding (or maybe the splitter's?) finally activates, the quick filter
>+    //  pane gets resized down without any notification.

As a longer-term fix, could we get the splitter to send the correct notification?  (Assuming that's what the problem is, of course.  It doesn't seem to happen in Classic View, so the splitter is my first suspect.)

>+    // The choice of the delay is basically a kludge because something like 10ms
>+    //  may be insufficient to ensure we get enqueued after whatever triggers
>+    //  the layout discontinuity.  (We need to wait for a paint to happen to
>+    //  trigger the XBL binding, and then there may be more complexities...)
>+    setTimeout(function UpdateMailPaneConfig_deferredFixup() {
[snip…]
>+    }, 1000);

So, I've seen this fix fail about half the time, which isn't great odds.  I think I'ld accept either upping the delay, or trying to wait for some other event to trigger the deferred fixup, whichever you'ld prefer.

>+++ b/mail/base/content/quickFilterBar.js
>@@ -322,42 +322,58 @@ let QuickFilterBarMuxer = {
>   onOverflow: function QFBM_onOverflow() {
[snip…]
>+    let textWidget = document.getElementById(
>+                       QuickFilterManager.textBoxDomId);

That all fits on one line.

>@@ -369,16 +385,24 @@ let QuickFilterBarMuxer = {
>+    // restore the text widget's min width...
>+    let textWidget = document.getElementById(
>+                       QuickFilterManager.textBoxDomId);

That all fits on one line, too.

Other than that, I like it.

Later,
Blake.

Comment 9

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

(In reply to comment #8)
> (From update of attachment 443851 [details] [diff] [review])
> So, I've seen this fix fail about half the time, which isn't great odds.  I
> think I'ld accept either upping the delay, or trying to wait for some other
> event to trigger the deferred fixup, whichever you'ld prefer.

Hm, yeah, that's probably worse than leaving the bug as-is...

I'll try and see if there is a better built-in event I can hook up to, even if it's just the XBL constructor of the message reader or the like.

Comment 10

[Asa Dotzler [:asa]]

Possible simple changes for immediate relief: 

1. set the qfb-qs-textbox min-width to 100 or something reasonably small. 
2. hide the label for qfb-filter-label when in icon only mode. (collapse it when you collapse down the labels for the quick-filter-bar-collapsible-buttons.)
3. kill the minwidth and or -moz-margin-start and -moz-margin-end style on the qfb-results-label so that space isn't wasted when there's no active search.

Comment 11

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

An attempt to use the message reader's loaded event was insufficient.  However, the existing hacky fix worked on all 3 platforms with the existing timeout, so I figure my even more hacky fix of using a timeout of 1500 instead of 1000 will work even better!

pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/5a4e3e8be386

Comment 12

[Dan Mosedale (:dmosedale, :dmose)]

What sort of risk (if any) is there associated with accepting this patch on branch?

Comment 13

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

We are at risk of people complaining less?

The better collapsing logic is a safe and reliable improvement; the vertical view startup fix is safe and with high probability of fixing the problem (but in some cases the timing may be off and leave us as if the patch had never been landed).  Safe in this case means low complexity with low probability of unforeseen interaction with other logic.

Comment 14

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 443851 [details] [diff] [review]
v1 collapse mo better

Heh, sounds good. a=dmose.

Comment 15

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

pushed to comm-1.9.2:
http://hg.mozilla.org/releases/comm-1.9.2/rev/d702cf679f34