1730334 - Attachment icons stretched on high-DPI monitor

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, defect)

Description

[Fan]

Attachment: attachment_icons_bug.jpg

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:92.0) Gecko/20100101 Firefox/92.0

Steps to reproduce:

Open a message containing attachments.

Actual results:

Icons are stretched in attachments pane (see screenshot).

Comment 1

[Fan]

Tested on Thunderbird 91.0.3 for macOS (high-DPI monitor).

Comment 2

[Henry Wilkes (they/them) [:henry-x]]

I cannot reproduce this, but I also do not have a high dpi screen. It is probably the xul layout being bad scaling html images, which I have seen before. This one is a little bit surprising though because the moz-icon: request specifies a size of 16 https://searchfox.org/comm-central/rev/2392baf95c8a9513a39f70e123779d12a0872376/mail/base/content/widgets/mailWidgets.js#1732 , but I guess the actual image may be larger.

If it is what I suspect, then just changing the parent to html (or just wrapping it in a div) works to fix it. A cleaner solution would be to change the whole attachmentcell-content structure to html (and its semantics can be improved along the way). I could fix this, but I would just need someone to test it with a high dpi monitor to be sure its actually working.

Comment 3

[u692859]

Attachment: attachment_icons_bug_confirmation.jpg

I can confirm the bug. Tested with Thunderbird 93.0b4 (64-bit) on a UHD screen (3840x2160) under Windows 10 Pro.

Comment 4

[Richard Marti (:Paenglab)]

I don't see this with TB 91 and Daily on Windows 10 and UHD screen. What scaling do you use and does it still happen in Troubleshoot mode (see Help menu)?

Comment 5

[u692859]

Attachment: attachment_icons_bug_91_no_scaling.jpg

Comment 6

[u692859]

Attachment: attachment_icons_bug_91_troubleshoot.jpg

It actually has to do with scaling in Windows. The problem does not occur without scaling, with the scaling of 200% set for me it also occurs in Thunderbird 91.1.1 (64-bit, troubleshoot mode); please see attached screenshots.

Comment 7

[Richard Marti (:Paenglab)]

I tried 100%, 125%, 150%, 175%, 200% and 225% and never saw the stretching. I started TB on this screen and also on another one and moved to the scaled screen, no problem visible.

Comment 8

[Richard Marti (:Paenglab)]

My last proposal: try to disable or enable HW-acceleration in Prefs/General at the bottom.

Comment 9

[Fan]

Attachment: attachment_icons_bug_win10_125.jpg

@Richard: I can reproduce in Windows 10 too (125 % scaling, after Thunderbird restart). HW-acceleration enabled or not.

Comment 10

[u692859]

@Richard: Switching hardware acceleration on or off has no effect.

Comment 11

[Henry Wilkes (they/them) [:henry-x]]

I can try and fix this by converting the structure to html. That has fixed this sort of issue in other situations, and people experiencing the problem can test if it works.

Out of curiosity, for people experiencing the problem, can you test the dimensions of the img element in browser toolbox. Specifically, with an attachment icon visible, open up the console and get the img element with

img = document.querySelector(".attachmentcell-icon")

and then test if any of its dimensional properties - .height, .width, .naturalHeight, .naturalWidth - differ from 16.

Comment 12

[Henry Wilkes (they/them) [:henry-x]]

I actually saw this by accident in a try run screenshot for osx https://firefoxci.taskcluster-artifacts.net/M6cMGQDJSkG3PDvQp1SPMw/0/public/test_info/mozilla-test-fail-screenshot_hq2mrb5l.png

Comment 13

[Fan]

Attachment: img = document.querySelector(".attachmentcell-icon") [125%]

@Henry: icon width is proportionnal to scale (100% -> 16px, 125% -> 20px, 150% -> 24px...)

```

```

Comment 14

[Fan]

Attachment: console_log_125percent.log

@Henry: icon width is proportional to scale (100% -> 16px, 125% -> 20px, 150% -> 24px...).

Comment 15

[Henry Wilkes (they/them) [:henry-x]]

Attachment: Bug 1730334 - Drop the view and orient attributes for attachment-lists. r=aleca

Comment 16

[Henry Wilkes (they/them) [:henry-x]]

Attachment: Bug 1730334 - Include attachmentItem margin in attachmentBucket min-height calculation. r=aleca

This is needed for the windows platform where a vertical margin is set.

Depends on D127285

Comment 17

[Henry Wilkes (they/them) [:henry-x]]

Attachment: Bug 1730334 - Move attachment item name and size logic into MozAttachmentList. r=aleca

Depends on D127286

Comment 18

[Henry Wilkes (they/them) [:henry-x]]

Attachment: Bug 1730334 - Change attachmentItem internal structure to html. r=aleca

Depends on D127287

Comment 19

[Henry Wilkes (they/them) [:henry-x]]

Attachment: Bug 1730334 - Remove excess attachment list click handler. r=aleca

Using a CSS rule obscures the logic, when we can just also handle descendant targets.

Depends on D127288

Comment 20

[Henry Wilkes (they/them) [:henry-x]]

Some things to still fix with the tests https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=020a0e5b9afa640ea75f095fc268102816d84ac0&selectedTaskRun=E14NTF9pQT-4QlsiasJlHw.0 but I'll do that next week.

Comment 21

[Henry Wilkes (they/them) [:henry-x]]

Attachment: Bug 1730334 - Tidy the bigFileObserver event handler. r=aleca

Depends on D127289

Comment 22

[Henry Wilkes (they/them) [:henry-x]]

Attachment: Bug 1730334 - Tidy openpgp attachment listeners. r=aleca

Also fixed a typo.

Depends on D127976

Comment 23

[Henry Wilkes (they/them) [:henry-x]]

Attachment: Bug 1730334 - Tidy the attachment list hideDropMarker method. r=aleca

Depends on D127977

Comment 24

[Henry Wilkes (they/them) [:henry-x]]

Attachment: Bug 1730334 - Always show an attachment's extension in attachment lists. r=aleca

If the attachment's full name would overflow and needs to be cropped, we still want to see the extension part at the end.

Depends on D127288

Comment 25

[Henry Wilkes (they/them) [:henry-x]]

Latest try run https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=b6c8be6aa9899d29b27ebcddcb941a430932535a

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/a9015ae2a270
Drop the view and orient attributes for attachment-lists. r=aleca
https://hg.mozilla.org/comm-central/rev/37d5cbe97cc7
Include attachmentItem margin in attachmentBucket min-height calculation. r=aleca
https://hg.mozilla.org/comm-central/rev/10911cc92082
Move attachment item name and size logic into MozAttachmentList. r=aleca
https://hg.mozilla.org/comm-central/rev/2eeffb03e098
Change attachmentItem internal structure to html. r=aleca
https://hg.mozilla.org/comm-central/rev/4cd3e226c6d8
Remove excess attachment list click handler. r=aleca
https://hg.mozilla.org/comm-central/rev/6233d6a6a750
Tidy the bigFileObserver event handler. r=aleca
https://hg.mozilla.org/comm-central/rev/d30258ef0b20
Tidy openpgp attachment listeners. r=aleca
https://hg.mozilla.org/comm-central/rev/566075f60ea0
Tidy the attachment list hideDropMarker method. r=aleca
https://hg.mozilla.org/comm-central/rev/1a3eaca4918a
Always show an attachment's extension in attachment lists. r=aleca

Comment 27

[Henry Wilkes (they/them) [:henry-x]]

Attachment: 1730334-esr91.patch

I tested copying the trunk commits on top of esr91, but there are several merge conflicts, and they don't all have an easy resolutions. It's probably safer to go with this one-line patch for esr91 instead. It's a bit of a hack because a html:img shouldn't need a CSS min-width when a width is already set, but the xul:hbox doesn't respect this.

Comment 28

[Alessandro Castellani [:aleca]]

Comment on attachment 9247514 [details] [diff] [review]
1730334-esr91.patch

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

This fixes the problem on 91, thanks.

Comment 29

[Henry Wilkes (they/them) [:henry-x]]

Comment on attachment 9247514 [details] [diff] [review]
1730334-esr91.patch

[Approval Request Comment]
Regression caused by (bug #): 1683865
User impact if declined: Attachment icons are stretched if they are scaled
Testing completed (on c-c, etc.): No
Risk to taking this patch (and alternatives if risky): Low risk. This is just a single additional CSS rule. This patch diverges from the fix on trunk, but is much simpler (see comment 27).
Note: I'm not putting in a beta request for either this patch or the ones for trunk since they are destined for the next beta anyway.

Comment 30

[Wayne Mery (:wsmwk)]

Comment on attachment 9247514 [details] [diff] [review]
1730334-esr91.patch

[Triage Comment]
Approved for esr91

Comment 31

[Rob Lemley [:rjl]]

Thunderbird 91.3.0:
https://hg.mozilla.org/releases/comm-esr91/rev/477d400bd330