Last Comment Bug 739311 - Clicking "x more" button shrinks(!) message header height, so that *less* or even *no* recipients are in view [multiple, many, lots of recipients]
: Clicking "x more" button shrinks(!) message header height, so that *less* or ...
Status: RESOLVED FIXED
UXprio?, [tb-papercut]
: useless-UI, ux-control, ux-efficiency
Product: Thunderbird
Classification: Client Software
Component: Message Reader UI (show other bugs)
: 11 Branch
: All All
: -- normal (vote)
: Thunderbird 18.0
Assigned To: michelr
:
:
Mentors:
Depends on: 511408
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-26 11:04 PDT by Derek Williams
Modified: 2012-09-29 20:42 PDT (History)
8 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Screenshot 1: message header with many recipients and "18 more" button (collapsed) (29.08 KB, image/png)
2012-03-26 11:04 PDT, Derek Williams
no flags Details
Screenshot 3: message header with many recipients, after clicking "18 more" button and scrolling up (to have full view of header buttons): header pane height is less then before, causing less or even no recipients in view! (20.91 KB, image/png)
2012-03-26 11:06 PDT, Derek Williams
no flags Details
Screenshot 2: message header with many recipients, after clicking "18 more" button (header buttons partly out of view!) (30.87 KB, image/png)
2012-03-26 11:09 PDT, Derek Williams
no flags Details
compute height of 'expandedHeaderView' from 'expandedHeadersBox' (3.65 KB, patch)
2012-09-02 05:23 PDT, michelr
squibblyflabbetydoo: review-
Details | Diff | Splinter Review
simplified version of compute height of 'expandedHeaderView' (2.73 KB, patch)
2012-09-21 16:41 PDT, michelr
no flags Details | Diff | Splinter Review
modified comments of previous patch (2.87 KB, patch)
2012-09-22 04:40 PDT, michelr
squibblyflabbetydoo: review+
Details | Diff | Splinter Review

Description Derek Williams 2012-03-26 11:04:11 PDT
Created attachment 609385 [details]
Screenshot 1: message header with many recipients and "18 more" button (collapsed)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:12.0) Gecko/20100101 Firefox/12.0
Build ID: 20120321033733

Steps to reproduce:

My issue was listed, but is marked 'resolved, works for me'.

Please refer to screenshot.

I clicked on '18 more' (screenshot 1) expecting the address pane to enlarge so as to accommodate all the recipients.


Actual results:

As you can see, the recipients' addresses are all obscured (screenshot 2). They can be visified by rotating the scroll wheel on my Kensington Expert Mouse, but no amount of clicking or dragging will persuade the divider line to enlarge the pane so as to be able to view all the addresses in one open window.


Expected results:

When '18 more' is clicked, the pane should enlarge so as to accommodate the addressees. If the divided is clicked on, a handle should appear that would allow resizing of the address pane.
Comment 1 Derek Williams 2012-03-26 11:06:27 PDT
Created attachment 609387 [details]
Screenshot 3: message header with many recipients, after clicking "18 more" button and scrolling up (to have full view of header buttons):  header pane height is less then before, causing less or even no recipients in view!

This is what happens after I click '18 more'
Comment 2 Derek Williams 2012-03-26 11:09:38 PDT
Created attachment 609388 [details]
Screenshot 2: message header with many recipients, after clicking "18 more" button (header buttons partly out of view!)
Comment 3 rsx11m 2012-03-26 11:32:03 PDT
That's most likely bug 511408, at least for the "more" part. It looks however aggravated judging from the screen shots provided in that not even a single line of recipients is shown.
Comment 4 Thomas D. (currently busy elsewhere; needinfo?me) 2012-04-18 03:55:49 PDT
Confirming. I have just reproduced this on a test msg on Win7/Earlybird
Mozilla/5.0 (Windows NT 6.1; rv:13.0) Gecko/20120417 Thunderbird/13.0a2
(haven't tested on release version yet)

Depending on the actual message and screen size, this problem is much worse than shown in the first screenshots by reporter: Clicking on "x more" button significantly *shrinks* (!) the header pane height, thus truncating the buttons at the top and *hiding* some or even *all* of those recipients that were visible before. I'm sure users will love us for that joke, and for forcing them to scroll in an even smaller header pane that will only ever show a maximum of 3 recipient rows at a time. It's closely related to bug 511408, but I don't think I saw this bug when I originally reported bug 511408:
* Bug 511408 is "header pane does't grow when clicking more (and cannot be manually expanded)";
* this bug is "header pane *shrinks* when clicking more (hiding even those recipients that were visible before, instead of showing more)".

Clearly, this is ridiculously useless-UI for a very basic and exposed scenario, and should have UXprio!

This might eventually be fixed by bug 511408, but I'd recommend to keep this open separately as a reminder of the aggravated situation until it has been verified fixed. Moreover, I'd think this useless-UI bug here can be fixed more easily (just ensure keeping the same absolute header pane height) and thus should be fixed asap before fixing more complex bug 511408.
Comment 5 michelr 2012-08-07 10:32:41 PDT
Hi, I made some tests on this bug :
The resize seems to be made only by css. So I added few lines of javascript to force height and avoid auto-scrolling.
It's a hack, I don't know if it may become a definitive solution.
But it works well for my tb everyday.

Here is the patch, as text (I don't know if it's correctly formatted):
(file chrome/messenger/content/messenger/mailWidgets.xml in omni.ja)

--- mailWidgets.xml	2012-07-14 00:40:09.000000000 +0200
+++ mailWidgets2.xml	2012-07-31 13:11:43.356151431 +0200
@@ -734,6 +734,18 @@
       <method name="buildViews">
         <body>
           <![CDATA[
+            // MODIF
+            // force eHV to re-compute his height
+            let eHV = document.getElementById("expandedHeaderView");
+            eHV.setAttribute("height", "");
+            // it's not the best place to put this code because it's called
+            // for each header field. It should be called only one time.
+            // but it's ok :
+            //   - it's a 2 lines hack ; easy to move in the future
+            //   - no slowdown
+            //   - it works !
+            //   - it has no side effect
+            // 
             this.maxLinesBeforeMore = Application.prefs.getValue(
               "mailnews.headers.show_n_lines_before_more",
               this.maxLinesBeforeMore);
@@ -833,6 +845,24 @@
 
             // Re-render the node, this time with all the addresses.
             this.fillAddressesNode(this.emailAddresses, true);
+            
+            
+            // MODIF
+            // compute height of 'expandedHeaderView' from 'expandedHeadersBox'
+            let eHV = document.getElementById("expandedHeaderView");
+            let eHB = document.getElementById("expandedHeadersBox");
+            let t = getComputedStyle(eHB, null).getPropertyValue("height");
+            // remove units
+            t = t.slice(0, -2);
+            // respect max-height property
+            let maxH =  getComputedStyle(eHV, null).getPropertyValue("max-height").slice(0, -2);            
+            t = Math.min(t, maxH);            
+            eHV.setAttribute("height", t);
+
+            // this attribute will be reinit in the 'buildViews' method
+
+            /*
+            NO NEED TO SCROLL ANYMORE
 
             // Scroll slightly to make it clear to the user how to reach
             // the remaining addresses
@@ -841,6 +871,7 @@
 
             // assume that the last two chars are "px" and discard them
             eHV.scrollTop = style.getPropertyValue("font-size").slice(0, -2);
+            */
           ]]>
         </body>
       </method>
Comment 6 Ludovic Hirlimann [:Usul] 2012-08-08 00:58:24 PDT
Can you attach patches see the comments we left in bug 349547
Comment 7 michelr 2012-09-02 05:23:42 PDT
Created attachment 657663 [details] [diff] [review]
compute height of 'expandedHeaderView' from 'expandedHeadersBox'
Comment 8 Thomas D. (currently busy elsewhere; needinfo?me) 2012-09-03 05:28:35 PDT
Comment on attachment 657663 [details] [diff] [review]
compute height of 'expandedHeaderView' from 'expandedHeadersBox'

Ludo, can you set the right reviewer for this?
Comment 9 :aceman 2012-09-03 05:32:42 PDT
I'd say mkmelin or squib.
Comment 10 Ludovic Hirlimann [:Usul] 2012-09-03 05:34:31 PDT
Comment on attachment 657663 [details] [diff] [review]
compute height of 'expandedHeaderView' from 'expandedHeadersBox'

:squib should be appropriate.
Comment 11 Jim Porter (:squib) 2012-09-16 01:03:10 PDT
Comment on attachment 657663 [details] [diff] [review]
compute height of 'expandedHeaderView' from 'expandedHeadersBox'

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

Thanks for the patch! We'll need some Mozmill tests for this. You can probably just modify the tests in mail/test/mozmill/message-header/test-message-header.js to ensure that the size increases appropriately. Also, please remove any trailing whitespace from your patch.

That said, this seems to work for the limited test cases I've tried, so good work!

::: mail/base/content/mailWidgets.xml
@@ +779,5 @@
>          <body>
>            <![CDATA[
> +            // MODIF
> +            // force eHV to re-compute his height
> +            let eHV = document.getElementById("expandedHeaderView");

In general, we prefer more descriptive (read: longer) variable names, though if you only use it once, it's better to collapse it into a single statement.

@@ +780,5 @@
>            <![CDATA[
> +            // MODIF
> +            // force eHV to re-compute his height
> +            let eHV = document.getElementById("expandedHeaderView");
> +            eHV.setAttribute("height", "");

Use removeAttribute.

@@ +782,5 @@
> +            // force eHV to re-compute his height
> +            let eHV = document.getElementById("expandedHeaderView");
> +            eHV.setAttribute("height", "");
> +            // it's not the best place to put this code because it's called
> +            // for each header field. It should be called only one time.

Please do this in UpdateExpandedMessageHeaders in mail/base/content/msgHdrViewOverlay.js (preferably near the similar statement for the #msgHeaderView).

@@ +932,5 @@
> +            // MODIF
> +            // compute height of 'expandedHeaderView' from 'expandedHeadersBox'
> +            let eHV = document.getElementById("expandedHeaderView");
> +            let eHB = document.getElementById("expandedHeadersBox");
> +            let t = getComputedStyle(eHB, null).getPropertyValue("height");

I think element.clientHeight will suffice here. If not, try element.boxObject.height.

@@ +937,5 @@
> +            // remove units
> +            t = t.slice(0, -2);
> +            // respect max-height property
> +            let maxH =  getComputedStyle(eHV, null).getPropertyValue("max-height").slice(0, -2);            
> +            t = Math.min(t, maxH);            

This shouldn't be necessary.

@@ +943,5 @@
> +
> +            // this attribute will be reinit in the 'buildViews' method
> +
> +            /*
> +            NO NEED TO SCROLL ANYMORE

In general, we don't comment out unused code. Just remove it. If we need it back, we'll look at our version history.
Comment 12 rsx11m 2012-09-17 07:46:45 PDT
Comment on attachment 657663 [details] [diff] [review]
compute height of 'expandedHeaderView' from 'expandedHeadersBox'

Adding to the review in the previous comment, I'm wondering if the following lines are needed or should be just removed:

> +            // MODIF

> +            // it's not the best place to put this code because it's called
> +            // for each header field. It should be called only one time.
> +            // but it's ok :
> +            //   - it's a 2 lines hack ; easy to move in the future
> +            //   - no slowdown
> +            //   - it works !
> +            //   - it has no side effect
> +            // 

While inline comments are useful for understanding of the code, keep in mind that the "blame" function will refer to your changes (thus no need for the "MODIF" tags) and also to the bug report here. Thus, the motivation/justification is preserved in the linked-to bug report and don't have to be reiterated in the code itself.
Comment 13 michelr 2012-09-21 16:41:37 PDT
Created attachment 663601 [details] [diff] [review]
simplified version of compute height of 'expandedHeaderView'

With your suggestions, patch is reduced to just 3 lines of code ! thanks !
Comment 14 Jim Porter (:squib) 2012-09-21 17:23:24 PDT
Comment on attachment 663601 [details] [diff] [review]
simplified version of compute height of 'expandedHeaderView'

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

I'll run with this tomorrow and give a full review, but to speed things along, I'll just look at the code for now...

::: mail/base/content/mailWidgets.xml
@@ +915,5 @@
>              this.clearChildNodes(this.emailAddresses);
>  
>              // Re-render the node, this time with all the addresses.
>              this.fillAddressesNode(this.emailAddresses, true);
> +            

Please remove this trailing whitespace.

@@ +916,5 @@
>  
>              // Re-render the node, this time with all the addresses.
>              this.fillAddressesNode(this.emailAddresses, true);
> +            
> +            // compute height of 'expandedHeaderView' from 'expandedHeadersBox'

Please use sentence case in comments.

::: mail/base/content/msgHdrViewOverlay.js
@@ +1019,5 @@
>    // Remove the height attr so that it redraws correctly. Works around a problem
>    // that attachment-splitter causes if it's moved high enough to affect
>    // the header box:
>    document.getElementById("msgHeaderView").removeAttribute("height");
> +  document.getElementById("expandedHeaderView").removeAttribute("height");

Could you add a short comment to this line explaining why we need to do this? Something like: "This height attribute may be set by toggleWrap() if the user clicked the "more" button in the header. Remove it so that the height is determined automatically."
Comment 15 michelr 2012-09-22 04:40:20 PDT
Created attachment 663672 [details] [diff] [review]
modified comments of previous patch
Comment 16 Jim Porter (:squib) 2012-09-22 15:24:34 PDT
Comment on attachment 663672 [details] [diff] [review]
modified comments of previous patch

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

This looks good, and since the code is so simple, I think we can forgo automated tests (Litmus tests couldn't hurt, but you don't need to worry about that). Thanks for the patch!
Comment 17 Jim Porter (:squib) 2012-09-22 15:32:16 PDT
Oh, I should probably mention one more important bit: assuming you're happy with the patch, add "checkin-needed" to the keywords up top. That will alert the appropriate people and get your code in Thunderbird proper. (We usually have the author of the patch do this so that they have a chance to fix something they missed before the code gets committed.)
Comment 18 Ryan VanderMeulen [:RyanVM] 2012-09-29 20:42:29 PDT
https://hg.mozilla.org/comm-central/rev/57f319d552b5

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