1610180 - [Attachments] The attachments bar is not scrollable even if there are too many attachments to fit

Status: RESOLVED FIXED

(Thunderbird :: Message Reader UI, defect, P2)

Description

[Michal Stanke (Mozilla.cz) [:mstanke][:MikkCZ]]

Attachment: screenshot

When there are many attachments in a received message, so they do not fit into the pane, the list overflows and is not completely visible, but the scrollbar does not appear, so the attachments cannot be accessed.

STR:

1. Send or receive a message with many attachments.
2. View the message in the main Thunderbird window.
3. Potentially change the width of the window so the attachments are in a single column and do not fit into the pane.
4. The list of attachments overflows, but there is no scrollbar.

---

For the record, this bug was reported to me by another user.

Comment 1

[Wayne Mery (:wsmwk)]

Version 68?

Comment 2

[Michal Stanke (Mozilla.cz) [:mstanke][:MikkCZ]]

68.4.1, 32-bit to be specific. Quoting the user, they were able to reproduce it on multiple of the latest versions (not sure what was the oldest one). I would say it's there for a while already.

Comment 3

[Magnus Melin [:mkmelin]]

Maybe we need to set a max-height on #attachmentList? But there's also a bunch of calculations https://searchfox.org/comm-central/rev/5f25e3440c68887112c36da83ca8c339c372b967/mail/base/content/msgHdrView.js#2932

Comment 4

[Ping Chen [:rnons] (away)]

Attachment: 1610180-attachment-list.patch

This patch sets max-height to #attachmentList

Comment 5

[Ping Chen [:rnons] (away)]

Attachment: attachment-list-scrollbar.png

Looks like this.

Comment 6

[Magnus Melin [:mkmelin]]

Comment on attachment 9136680 [details] [diff] [review]
1610180-attachment-list.patch

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

Looks pretty good! Thanks for the patch. Some minor things to update below. 
Please also include in the commit message why you're doing it (to ensure the attachments area gets scrollbars when needed)

::: mail/base/content/msgHdrView.js
@@ +2915,4 @@
>  
>      attachmentList.setOptimumWidth();
>  
> +    const attachmentBarHeight =

we usally use "let" for these

@@ +2926,2 @@
>        document.getElementById("messagepanebox").getBoundingClientRect().height /
> +      2;

I think this should continue to be 4, and please clarify the comment above to make it clear we don't want the message pane taking up more then 1/4 of the space.

@@ +2929,5 @@
>      attachmentView.setAttribute(
>        "height",
>        Math.min(attachmentHeight, maxAttachmentHeight)
>      );
>      attachmentView.setAttribute("maxheight", attachmentHeight);

was going to ask if this should't be updated, but seems it's not used for anything, so please remove it here and below

Comment 7

[Ping Chen [:rnons] (away)]

Attachment: 1610180-attachment-list-reviewed.patch

Updated commit message, fixed review comments. Please take another look.

Comment 8

[Magnus Melin [:mkmelin]]

Comment on attachment 9137031 [details] [diff] [review]
1610180-attachment-list-reviewed.patch

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

Looks good thanks! r=mkmelin
I'll fix the nit before landing.

::: mail/base/content/msgHdrView.js
@@ +2919,5 @@
>        attachmentView.getBoundingClientRect().height -
> +      attachmentList.getBoundingClientRect().height;
> +
> +    // By design, attachmentView should not take up more than 1/4 of the message
> +    // pane space

tiny nit: add period at end of the sentence.

Comment 9

[Ping Chen [:rnons] (away)]

Attachment: 1610180-attachment-list-reviewed.patch

Add reviewer to commit message. Add ending period to inline comment.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/77796c5f7c3b
set max-height on #attachmentList to make it scrollable. r=mkmelin