1562200 - [de-xbl] Attachment list in messagepane display regressions

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[alta88]

1. For external (detached, file and link, http) attachments the list item display is broken.
2. There is a lot of false cropping for short names when there are multiple attachments. And attachments whose size is resolved async do not get updated/adjusted for new width (Bug 1345167 took pains to make sure this is not sloppy looking).

Probably regressed by Bug 1547947.

Comment 1

[Jorg K (CEST = GMT+2)]

Affects TB 68, right?

Comment 2

[Alessandro Castellani [:aleca]]

Thanks for reporting the regression.
I'll take a look at this over the weekend.

Comment 3

[Jorg K (CEST = GMT+2)]

Alex, can you get to this asap, we need this for beta 4 this week. If you want to check how it's meant to work, please try a Daily from 2019-04-05 when bug 1345167 comment #49 landed.

Comment 4

[Alessandro Castellani [:aleca]]

Yes, I'm working on this right now and I should have it done for today.

Comment 5

[Alessandro Castellani [:aleca]]

Hey alta88, could you please upload a screenshot showing the regression, and write down a quick list of actions in order to trigger the issue?
I think I'm missing something as I can't seem to be able to recreate the visual quirks you listed.
Thank you so much.

Comment 6

[Richard Marti (:Paenglab)]

Alessandro, I haven't checked what looks not correct but you can try to detach an attachment, then it should be underlined like a link. Then you could detach a second attachment and then delete it in the saved place. Then this one should be grey in TB. All from memory and not 100% sure it's correct.

Comment 7

[Alessandro Castellani [:aleca]]

Attachment: Screen Shot 2019-07-01 at 11.49.12 PM.png

Thanks Richard for the info, I tried that already.
I also checked external attachments in an RSS feed, and tried pretty much any interaction I could think of with the attachments, but I can't seem to stumble upon the broken list.

Here's what I did so far:

• Load RSS feed with external attachments
• Check message with 5 attachments with different name lengths and size
• Detach multiple attachments, and delete some of them
• Upload multiple attachments at once in different sizes
• Convert attachments to wetransfer link

Am I missing some obvious workflow?

Comment 8

[Jorg K (CEST = GMT+2)]

Attachment: 2019-07-02 08_59_50-Syntax - Tasty Web Development Treats - Feeds - Mozilla Thunderbird.png

OK, I tried http://feed.syntax.fm/rss in TB 68 beta 4. I see this. Is this expected?

Comment 9

[Alessandro Castellani [:aleca]]

Attachment: Screen Shot 2019-07-02 at 12.02.16 AM.png

Nope, I see it like this on both Linux and macos.
Is this a Windows specific issue?

Comment 10

[alta88]

(In reply to Alessandro Castellani (:aleca) from comment #9)

Created attachment 9075299 [details]
Screen Shot 2019-07-02 at 12.02.16 AM.png

Nope, I see it like this on both Linux and macos.
Is this a Windows specific issue?

The screenshots in comment 7 and comment 8 show the problem. It's as though the custom element constructor is not creating all nodes in the dom correctly for external, and what is in textContent is the url and not the name. I currently see it on a mac, but doubt it's os specific.

The false cropping shows up for all attachments, internal and external, when there's even only one. This shouldn't happen; there should only be cropping in real outlier cases. And this code did formerly work to optimally lay out multiple attachments horizontally with name both long and short. The patch cited above, which made external sizes change (fetching true size only on user click, for privacy reasons) from what may have been reported. This then needed a recalculation to adjust the layout and avoid false cropping.

A test case needs to have several attachments, which wrap/don't wrap the layout, and have a mix of rather long and also short names. It should be easy to copy/paste to create such eml files.

Comment 11

[Alessandro Castellani [:aleca]]

OK, so to quickly recap.

Sizing issue
The attachments shouldn't be cropped if there's space, and they should resize accordingly to fill up the RichListBox

External attachment name
You say the name should be visible and not the URL, but by looking at the code it seems it supposed to show the URL for external attachments.
Is this wrong? https://searchfox.org/comm-central/source/mail/base/content/msgHdrView.js#2304

Comment 12

[alta88]

No, the name is supposed to be used in the list, as it shows in the toolbar already. As per comment 3, it's best to see how it worked before the soon thereafter landing of the de-xbl code which, as I've said, seems to have changed how the nodes for the list item are now constructed. Meaning the child nodes of an internal item are different from external. So for an email with 2 attachments, detach one and inspect them.

Comment 13

[Jorg K (CEST = GMT+2)]

Hmm, looks like this will miss TB 68 beta 4 which I'll build tomorrow. I definitely need it for beta 5 for next week, hopefully our last beta in the 68 series.

Alta88, you said in the RDF bug that you'd be unavailable for a few weeks from early June. When would yo be able to do a review?

Comment 14

[Jorg K (CEST = GMT+2)]

Does backing out https://hg.mozilla.org/releases/comm=beta/rev/72c5f4b9c7201d1f96a330f8703032e9961d8265 from beta make a difference. As I see it, that uplift was optional. (EDIT: destroy link).

Alex, you can pull the beta with hg clone https://hg.mozilla.org/releases/comm-beta, then hg qbackout -r 72c5f4b9c7201d1f96a330f8703032e9961d8265. Then send this this to try for the platform(s) you want. Anyway, just a thought.

Comment 15

[Alessandro Castellani [:aleca]]

Attachment: bug1562200-attachmentlist-regression.patch

This should do it.

The issues where happening because the de-xbl didn't turn this into a custom element, therefore we lost all the inheritance of the text, icons, and size value.

This should take care of properly updating everything whenever there's a change, and also fixes some regressions I found in the message compose related to renaming the file and uploading it to a cloud service.

Comment 16

[Alessandro Castellani [:aleca]]

Attachment: bug1562200-attachmentlist-regression.patch

Removed some leftover comments.

Comment 17

[Jorg K (CEST = GMT+2)]

Comment on attachment 9076051 [details] [diff] [review]
bug1562200-attachmentlist-regression.patch

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

::: mail/base/content/mailWidgets.js
@@ +1353,5 @@
>      let item = this.ownerDocument.createXULElement("richlistitem");
>      item.classList.add("attachmentItem");
>      item.setAttribute("name", name || attachment.name);
>      item.setAttribute("role", "option");
> +    // <xul:hbox class="attachmentcell-content" flex="1">

What's that comment? Is that what the code below is constructing? Maybe not.

EDIT: Mid-air collision, comment already removed.

Comment 18

[Jorg K (CEST = GMT+2)]

Comment on attachment 9076053 [details] [diff] [review]
bug1562200-attachmentlist-regression.patch

This restores the functionality as I know it. I tried a few things and they all looked good. Sorry, I'm not aware of the fine print here. Alta88, can you review this or should we build you a version to try? Mac? Linux? Windows?

Also, I'm not a de-XBL expert to moving the review over to Geoff in Magnus' absence. Or maybe Khushil wants to check it.

Comment 19

[Jorg K (CEST = GMT+2)]

Comment on attachment 9076053 [details] [diff] [review]
bug1562200-attachmentlist-regression.patch

(Let's add the beta flag so I don't forget)

Comment 20

[Jorg K (CEST = GMT+2)]

Comment on attachment 9076053 [details] [diff] [review]
bug1562200-attachmentlist-regression.patch

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

I looked over the changes again and they look plausible. One question, see below.

::: mail/base/content/msgHdrView.js
@@ -2337,5 @@
>      }
>  
>      if (attachment.isExternalAttachment) {
>        displayUrl = attachment.displayUrl;
> -      attachmentitem.textContent = displayUrl;

Hmm, how did this get here and why are we removing it now?

Comment 21

[Alessandro Castellani [:aleca]]

I don't know why it was there as we don't want to replace the file name with the URL.
We need to remove it completely because, after de-xbl'ing that component, updating the textContent of the attachmentItem completely deletes any child node, replacing them with the URL string.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/8fbca3217e6e
Attachment list messagepane display regressions. r=jorgk

Comment 23

[Jorg K (CEST = GMT+2)]

Well, I needed a patch to push since I couldn't rerun the decision task on the busted run (bug 1563627), so I landed this one. Let's to a PLR and follow up on anything that isn't right. BTW, I added a space before the !important here:
https://hg.mozilla.org/comm-central/rev/8fbca3217e6e#l1.19

Sigh, I was so busy with the M-C bustage that I didn't fix the commit message.

Comment 24

[Geoff Lankow (:darktrojan)]

Comment on attachment 9076053 [details] [diff] [review]
bug1562200-attachmentlist-regression.patch

I'm okay with this. Setting `.image` on the item itself doesn't appear to do anything but that's not major.

Comment 25

[Jorg K (CEST = GMT+2)]

(In reply to Geoff Lankow (:darktrojan) from comment #24)

Setting .image on the item itself doesn't appear to do anything but that's not major.

Hmm, you're referring to these:
https://searchfox.org/comm-central/search?q=attachmentItem.image+%3D&case=true&regexp=false&path=

We can pull them out in a follow-up. So does this one need some sort of replacement?
https://searchfox.org/comm-central/rev/97b409bd5b08873dcf101a12457b93a2a8fb984c/mail/components/compose/content/MsgComposeCommands.js#1642

Note that in the compose window's attachment bucket these
https://searchfox.org/comm-central/search?q=item.image+%3D&case=true&regexp=false&path=MsgComposeCommands.js
are needed since they provide the icon, at least that's what I thought.

Please file another bug for the clean-up, I'll take the patch to beta now and we're done here.

Comment 26

[Jorg K (CEST = GMT+2)]

TB 68 beta 4:
https://hg.mozilla.org/releases/comm-beta/rev/521024a6a7e93b59983c2a8dcf19b2bae895fc4e

Comment 27

[alta88]

thanks, and r+ as it's no longer entirely broken, but please address Bug 1563793.