Closed Bug 1563793 Opened 1 year ago Closed 1 year ago

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

Categories

(Thunderbird :: General, defect)

defect
Not set
normal

Tracking

(thunderbird68 fixed, thunderbird69 fixed, thunderbird70 fixed)

RESOLVED FIXED
Thunderbird 70.0
Tracking Status
thunderbird68 --- fixed
thunderbird69 --- fixed
thunderbird70 --- fixed

People

(Reporter: alta88, Assigned: aleca)

References

Details

(Keywords: regression)

Attachments

(1 file, 2 obsolete files)

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

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

Assignee: nobody → alessandro
Status: NEW → ASSIGNED
Attached patch 1563793-attachmentlist.patch (obsolete) — Splinter Review

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?

Attachment #9076320 - Flags: review?(geoff)
Attachment #9076320 - Flags: review?(alta88)

Maybe Richard?

Flags: needinfo?(richard.marti)

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?

Flags: needinfo?(richard.marti)
Summary: [de-xbl] Attachment list in messagepane display regressions (cont) → [de-xbl] Attachment list in messagepane display regressions (continued)
Version: unspecified → 69

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 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;
Attachment #9076320 - Flags: review?(geoff)
Attachment #9076320 - Flags: review?(alta88)
Attachment #9076320 - Flags: review-
Attached patch 1563793-attachmentlist.patch (obsolete) — Splinter Review

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.

Attachment #9076320 - Attachment is obsolete: true
Attachment #9076621 - Flags: review?(geoff)
Attachment #9076621 - Flags: review?(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?

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.

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

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 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.
Attachment #9076621 - Flags: review?(alta88) → review+

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.

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 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.
Attachment #9076621 - Flags: review?(geoff) → review+

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

I need this for beta on Thursday.

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.

Attachment #9076621 - Attachment is obsolete: true
Attachment #9076867 - Flags: review+
Keywords: checkin-needed
Attachment #9076867 - Flags: approval-comm-release+
Attachment #9076867 - Flags: approval-comm-beta+
Target Milestone: --- → Thunderbird 70.0

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/dd49b5df594c
Fix more attachment list messagepane display regressions. r=darktrojan,alta88

Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Keywords: checkin-needed
Resolution: --- → FIXED
Attachment #9076867 - Flags: approval-comm-release+
You need to log in before you can comment on or make changes to this bug.