1636558 - Attachment pane layout breaks upon toggling pane and/or resizing in message reader with many attachments, covered/hidden by grey area, unusable

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, defect, P1)

Description

[Thomas D. (:thomas8)]

Attachment: Screenshot 1: attachment pane unusable, items hidden behind grey area

Is anyone seeing this?

Seen on Windows 10, TB 78.0a1 (2020-05-08) (64-bit)
Completely unusable for me (see screenshot).

• some more attachments in a draft, save draft (not sure if draft matters, guess not)
• look at draft in message reader
• use attachment pane twisty and/or resize the attachment pane vertically

actual result

• as you drag splitter upwards, grey area follows from bottom and covers attachments instead of revealing them in the bigger space
• grey area will not go away even when you close twisty

expected

• see more attachments

Comment 1

[Alessandro Castellani [:aleca]]

I'm seeing this on Linux as well.
Definitely a regression.

Comment 2

[Thomas D. (:thomas8)]

Alice, pretty please!!!

Comment 3

[Thomas D. (:thomas8)]

Perhaps something wrong with scrollbar/scrollbox, which doesn't appear to be flex.

Comment 4

[Richard Marti (:Paenglab)]

The attachmentList has a max-height set. I remember there was a bug about hiding attachments when where are too much.

Comment 5

[Richard Marti (:Paenglab)]

Probably a regression from bug 1610180.

Comment 6

[Thomas D. (:thomas8)]

Guess this is major because it's so prominent and hazardous effect. Might be easy to fix. Feel free to change the rating.

Comment 7

[Alice0775 White]

Reression window(no further bisection due to startup crash):
https://hg.mozilla.org/comm-central/pushloghtml?fromchange=7b54036ce7d67afa8361606c720287e5710b4775&tochange=1d6dea4edf1a948dd913439e6a98aa1e7afceefc
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=9af589864188fc4061a020c45914f54fa9f67b0d&tochange=6639deb894172375b05d6791f5f8c7d53ca79723

Comment 8

[Alessandro Castellani [:aleca]]

Thank you so much Alice, as usual you're absolutely the best!

Comment 9

[Alessandro Castellani [:aleca]]

I identified 2 bugs which caused this regression.

• Bug 1610180 implemented the max-height in the attachmentList to fix the overflow issue, but also removed those attributes from the attachmentView which are necessary to properly handle the resize of that area when the splitter is moved.
• Bug 1631198 introduced an extra vbox for the OpenPGP key, which the splitter prioritizes when resizing, consequentially ignoring the Attachment pane.

I also identified another substantial issue, which is the face that in the messenger view we started listing a lot of boxes underneath the message header, including the imip-bar.

That shouldn't happen as the body of the message should only contain the content and the attachment box, in order for the splitter to be able to resize only these 2 elements accordingly.

Most of these extra boxes have a fixed height, so we never noticed the interference they were causing.
As a follow up patch, I will convert all those extra boxes into NotificationBox elements, as the behaviour and UI is pretty identical.

Comment 10

[Alessandro Castellani [:aleca]]

Attachment: 1636558-attachment-regression.diff

If you can, test this by looking at message with a different amount of attachments, from 1 to multiple.

Try to resize the attachment area, which it shouldn't be draggable beyond the maximum amount of attachments listed, and the scrollbar should appear.

Comment 11

[Richard Marti (:Paenglab)]

Comment on attachment 9146932 [details] [diff] [review]
1636558-attachment-regression.diff

ui-r- because it has still size issues.

With one attachment it works perfectly but I have a message with four attachments, which have enough place on one row, and it shows a second blank row below which I can't reduce the pane size with dragging down the splitter. It only collapses.

On a message with 13 attachments it shows a grey box below the attachment list. It seems the size of this grey box is missing on the notifications bar height.

Playing some more doesn't show the grey bar at the bottom anymore and the notification bar has the correct height. But when I hide the attachment pane in a drafts folder and then switch to a other folder I get a grey bar there the notification bar was.

Very tricky. I'll add some screenshots.

Comment 12

[Richard Marti (:Paenglab)]

Attachment: 4-attachments.png

Screenshot of the too tall attachment list.

Comment 13

[Richard Marti (:Paenglab)]

Attachment: 13-attachments.png

Screenshot of the grey box below and the too small notification bar. This isn't because I shrunk the message to make the image smaller. It was the same when I had the message taller.

Comment 14

[Richard Marti (:Paenglab)]

Attachment: grey-bar-on-top.png

The grey box where the notifications are but with a message without notification.

Comment 15

[Alessandro Castellani [:aleca]]

Very tricky indeed.
Thanks for the detailed review, I'll see if I can reproduce these scenarios.

Comment 16

[Alessandro Castellani [:aleca]]

Attachment: 1636558-attachment-regression.diff

This was strangely annoying to figure out.
I was able to fix it by mostly using CSS and converting those elements into flex containers.

Also, we were doing a lot of calculations for the height that we can avoid by using the preferredHeight method for the richlist element.
Overall, adding an inline style of max-height to the #attachmentList is wrong as that element should always expand for the maximum available height of the its container.

The only thing I'm not happy about, is the Add 3px to account for item's margin in the preferredHeight method, but without it, we would get a visible scrollbar just because of those 3 missing pixels.

Comment 17

[Richard Marti (:Paenglab)]

Comment on attachment 9146969 [details] [diff] [review]
1636558-attachment-regression.diff

Works now much better. The attachment pane works now as it should when dragging.

I still have the grey notification bar remnant when I hide the attachment pane in a draft folder and then switch to a other folder but I have this too on Daily without the patch - I'll file a new bug for this.

Comment 18

[Richard Marti (:Paenglab)]

Filed bug 1636667 for the grey notification bar remnant.

Comment 19

[Magnus Melin [:mkmelin]]

Comment on attachment 9146969 [details] [diff] [review]
1636558-attachment-regression.diff

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

LGTM, r=mkmelin

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/73be5343da88
Fix attachment pane height regression in message pane. r=mkmelin

Comment 21

[Magnus Melin [:mkmelin]]

I think this is causing test failures:
https://treeherder.mozilla.org/#/jobs?repo=comm-central&selectedTaskRun=RwN4gEyjQfi3sfIbX42LFg-0

TEST-UNEXPECTED-FAIL | comm/mail/test/browser/folder-display/browser_openingMessages.js | messages box height (1080) not equal to the sum of displayDeck height (0) and message pane box wrapper height (1079) and message notification box height (0) - 1080 == 1079 - JS frame :: chrome://mochitests/content/browser/comm/mail/test/browser/folder-display/browser_openingMessages.js :: check_message_pane_in_tab_full_height :: line 201

Comment 22

[Alessandro Castellani [:aleca]]

Ah, shoot, sorry.
I'll fix this.

Comment 23

[Alessandro Castellani [:aleca]]

Attachment: 1636558-attachment-regression-test.diff

This should do the trick.
Apparently, getBoundingClientRect() can't properly handle CSS borders.
Here's a try run: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=1e1a937c89a641901c4f221922e0e0b920d2d319

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/5e8adf8205d5
Fix failing test for attachment pane height regression in message pane. r=darktrojan

Comment 25

[Richard Marti (:Paenglab)]

Attachment: 1636558-remove-messagepaneboxwrapper-border.patch

We have now a unneeded border on the bottom of the message pane. This patch removes it.
r+ from aleca through Matrix.

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/79a064e00626
Remove the unneeded messagepaneboxwrapper bottom border. r=aleca