Closed Bug 739311 Opened 12 years ago Closed 12 years ago

Clicking "x more" button shrinks(!) message header height, so that *less* or even *no* recipients are in view [multiple, many, lots of recipients]

Categories

(Thunderbird :: Message Reader UI, defect)

Product:
Thunderbird
Component:
Message Reader UI
Version:
11 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 18.0

People

(Reporter: derek, Assigned: renon)

References

Details

(Keywords: useless-UI, ux-control, ux-efficiency, Whiteboard: UXprio?, [tb-papercut])

Attachments

(4 files, 2 obsolete files)

Screenshot 1: message header with many recipients and "18 more" button (collapsed)
29.08 KB, image/png
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
Details
Screenshot 2: message header with many recipients, after clicking "18 more" button (header buttons partly out of view!)
30.87 KB, image/png
Details
modified comments of previous patch
2.87 KB, patch
squib
: review+
Details | Diff | Splinter Review 
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.
Summary: can't drag the divider down for multiple recipients → can't drag the addressee pane divider down to resize for multiple recipients
Attachment #609387 - Attachment description: after I click '18 more' → after I rotate scrollwheel
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.
Component: General → Message Reader UI
QA Contact: general → message-reader
Whiteboard: dupeme?
Depends on: 511408
Attachment #609385 - Attachment description: Screen Shot 2012-03-26 at 19.01.10.png → Screenshot 1: message header with many recipients and more button (collapsed)
Attachment #609388 - Attachment description: after I click '18 more' → Screenshot 2: message header with many recipients, after clicking "18 more" button (header buttons partly out of view!)
Attachment #609385 - Attachment description: Screenshot 1: message header with many recipients and more button (collapsed) → Screenshot 1: message header with many recipients and "18 more" button (collapsed)
Attachment #609387 - Attachment description: after I rotate scrollwheel → 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!
OS: Mac OS X → All
Hardware: x86 → All
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: useless-UI, ux-control, ux-efficiency
Summary: can't drag the addressee pane divider down to resize for multiple recipients → Clicking "x more" button shrinks(!) message header height, so that *less* or even *no* recipients are in view [multiple, many, lots of recipients]
Whiteboard: dupeme? → UXprio?
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>
Can you attach patches see the comments we left in bug 349547
Assignee: nobody → michel.renon
Status: NEW → ASSIGNED
Whiteboard: UXprio? → UXprio?, [tb-papercut]
Comment on attachment 657663 [details] [diff] [review]
compute height of 'expandedHeaderView' from 'expandedHeadersBox'

Ludo, can you set the right reviewer for this?
Attachment #657663 - Flags: review?
I'd say mkmelin or squib.
Comment on attachment 657663 [details] [diff] [review]
compute height of 'expandedHeaderView' from 'expandedHeadersBox'

:squib should be appropriate.
Attachment #657663 - Flags: review? → review?(squibblyflabbetydoo)
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.
Attachment #657663 - Flags: review?(squibblyflabbetydoo) → review-
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.
With your suggestions, patch is reduced to just 3 lines of code ! thanks !
Attachment #657663 - Attachment is obsolete: true
Attachment #663601 - Flags: review?(squibblyflabbetydoo)
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."

        Attachment #663601 -
        Attachment is obsolete: true

        Attachment #663601 -
        Flags: review?(squibblyflabbetydoo)

        Attachment #663672 -
        Flags: review?(squibblyflabbetydoo)

    
    

    
  
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!

        Attachment #663672 -
        Flags: review?(squibblyflabbetydoo) → review+

    
    

    
  
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.)

    
  
Keywords: checkin-needed

    
    

    
  
https://hg.mozilla.org/comm-central/rev/57f319d552b5
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Flags: in-testsuite-
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 18.0

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: