1563793 - [de-xbl] Attachment list in messagepane display regressions (continued)

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[alta88]

+++ This bug was initially created as a clone of Bug #1562200 +++

Bug 1562200 is much better, still some things:

1. There is still some false cropping. For example, create two identical emails with 1 attachment, if the second has its attachment detached (the names are the same), the name of the first will be uncropped, while the second will be cropped. This may have to do with the async fetch of the detached ones file size, though it's not obvious why.

2. The listbox attempts to size itself based on number of items. For 2 or more, it leaves some bottom room. If there are more than fit in this default height a scrollbar appears. Dragging the splitter, however, does not adjust the scrollheight and show the whole list. It formerly did.

Comment 1

[Alessandro Castellani [:aleca]]

Sounds good, thanks for the detailed report.
I will take care of this today.

Comment 2

[Alessandro Castellani [:aleca]]

Attachment: 1563793-attachmentlist.patch

All right, also this should be fixed.
The issue with the scrollbar and the richlistbox not expanding properly was due to a missing flex="1" attribute.

Regarding the attachment item width, the issue is not related by the changes in the size label, but apparently after de-xbl'ing it, the scrolledWidth attribute doesn't account anymore for CSS inline margin.

I'm not sure this is a good solution, but adding 11 pixels to the sum fixes the issue.
Can someone suggest a better approach?

Comment 3

[Jorg K (CEST = GMT+2)]

Maybe Richard?

Comment 4

[Richard Marti (:Paenglab)]

Could this 11px come from the scrollbar? What's your setting for scrollbars on Mac (and Linux), show always or one of the other two?

Comment 5

[Jorg K (CEST = GMT+2)]

I don't think we want to hard-code 11px anywhere in JS code. If this is about a width calculation, can we subtract something from an overall width like was done here:
https://hg.mozilla.org/comm-central/rev/9f2d18f74b15#l1.14

Comment 6

[alta88]

Comment on attachment 9076320 [details] [diff] [review]
1563793-attachmentlist.patch

I think we know hardcoding that is not the way to go :/

Need to get rid of the |border| var in setOptimumWidth(). It can actually be negative.

      child.width = width;

Comment 7

[Alessandro Castellani [:aleca]]

Attachment: 1563793-attachmentlist.patch

I found the issue!

The problem was due to the width not being an integer.
Most of the times the calculated width of the container was something like 277.33 or 184.2, resulting in an automatic rounding to the lowest integer.
That change of half a pixel was causing the unnecessary crop.

Using getBoundingClientRect().width to get the real, non rounded width, and applying Math.ceil to always round up to the higher integer fixes the problem.

Comment 8

[alta88]

It's not an integer because |border| can be a fractional negative value, since getBoundingClientRect().width returns subpixel precision while scrollWidth returns a rounded integer. So why not just use scrollWidth to get the width?

Comment 9

[Alessandro Castellani [:aleca]]

Because scrollWidth returns a rounded up value to the closest integer, not the higher.
So if the actual width is 277.33, scrollWidth returns 277.

We need an integer because applying child.width = 277.33 (returned by the sum between scrollWidth and border), causes the cropping.
And using only scrollWidth causes the same problem because we'd end up with child.width = 277.

Using getBoundingClientRect().width to get the subpixel precision, and applying Mach.ceil to round up to the higher integer guarantees a correct sizing without accidental and unnecessary cropping.

Comment 10

[alta88]

Yes that's all how it would seem theoretically, but when I tried

   -child.width = width + border;
   +child.width = width;

there wasn't a single cropped item in my numerous test cases, which is either 1) not hitting a fraction <.50 or 2) it doesn't matter if 277.33 is reduced to 277, but only if 277 is reduced below 277 by a negative |border|.

Comment 11

[Alessandro Castellani [:aleca]]

That's what I'm doing in the patch.
I removed the border so we avoid stumbling upon a negative value, therefore preventing the subtraction of the border to the width and causing the 1px decrease.
But also I'm preventing further problems by grabbing the subpixel precision and rounding up to the higher integer.

Comment 12

[alta88]

Comment on attachment 9076621 [details] [diff] [review]
1563793-attachmentlist.patch

Yes, I know but I rather doubt it's necessary complexity without a demonstrable case. I'll leave it to you.

Comment 13

[Alessandro Castellani [:aleca]]

For sure, I agree with the fact of being picky about these things and avoid the usage of unnecessary methods.
I tried doing a simple child.width = width; and while I wasn't having any cropping issues with a single attachment, I was still having it with multiple attachments.

In my test case I have 7 attachments with fairly simple and short names, and only 1 with a long name. The attachment with the long name was getting cropped.
This is the output to compare the values between getBoundingClientRect().width and scrollWidth of the various attachments.

console.log: 195.3333282470703, 195
console.log: 184, 184
console.log: 195.3333282470703, 195
console.log: 180, 180
console.log: 158.6666717529297, 159
console.log: 277.33331298828125, 277
console.log: 142.6666717529297, 143

As you can see, the scrollWidth rounds the value to the closest integer.
In my case, the bigger attachment gets resized to 277, while it should actually be 278. That 0.3 px difference causes the cropping.

Comment 14

[alta88]

Amazingly, I also have a case with 7 attachments with only 1 long name, and chance made its fraction > .49 so thanks for the proof.

Comment 15

[Geoff Lankow (:darktrojan)]

Comment on attachment 9076621 [details] [diff] [review]
1563793-attachmentlist.patch

Better, but I'm still getting extra space below the list. 

It looks like that's because we've set `display: block` on #attachmentList to cause the contents to wrap. When it was a listbox, this happened automatically, but listbox was replaced with richlistbox and richlistbox doesn't wrap. Nor is it expecting to be `display: block`, so the size is calculated incorrectly in some cases. There's probably not much we can do about it, but I did find `overflow-x: hidden` helped sometimes.

Comment 16

[Alessandro Castellani [:aleca]]

I'll see if I can find a solution with display flex or similar.
Otherwise tomorrow I'll mark this for check-in.

Comment 17

[Jorg K (CEST = GMT+2)]

I need this for beta on Thursday.

Comment 18

[Alessandro Castellani [:aleca]]

Attachment: 1563793-attachmentlist.patch

I added the overflow-x: hidden; as suggested by darktrojan, but the display block still causes a small extra bottom border in some random cases.

Let's land this as it fixes the most glaring issues and makes the whole section almost perfect.
I'll create a follow up bug to lately handle that UI issue.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/dd49b5df594c
Fix more attachment list messagepane display regressions. r=darktrojan,alta88

Comment 20

[Jorg K (CEST = GMT+2)]

TB 69 beta (TB 68 beta coming):
https://hg.mozilla.org/releases/comm-beta/rev/1e6a4c2024e2054cc2e7ab6659189c5fc63a503f

Comment 21

[Jorg K (CEST = GMT+2)]

TB 68 beta 5:
https://hg.mozilla.org/releases/comm-beta/rev/26081cf14d0261b61044a8d4f5ea75569d5d2470