[de-xbl] Attachment list in messagepane display regressions
Categories
(Thunderbird :: General, defect)
Tracking
(thunderbird68+ fixed, thunderbird69 fixed)
People
(Reporter: alta88, Assigned: aleca)
References
(Regression)
Details
(Keywords: regression)
Attachments
(4 files, 1 obsolete file)
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
patch
|
darktrojan
:
review+
jorgk-bmo
:
feedback+
jorgk-bmo
:
approval-comm-beta+
|
Details | Diff | Splinter Review |
- For external (detached, file and link, http) attachments the list item display is broken.
- 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.
Updated•5 years ago
|
Assignee | ||
Comment 2•5 years ago
|
||
Thanks for reporting the regression.
I'll take a look at this over the weekend.
Assignee | ||
Updated•5 years ago
|
Comment 3•5 years ago
|
||
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.
Assignee | ||
Comment 4•5 years ago
|
||
Yes, I'm working on this right now and I should have it done for today.
Assignee | ||
Comment 5•5 years ago
|
||
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•5 years ago
|
||
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.
Assignee | ||
Comment 7•5 years ago
|
||
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•5 years ago
|
||
OK, I tried http://feed.syntax.fm/rss in TB 68 beta 4. I see this. Is this expected?
Assignee | ||
Comment 9•5 years ago
|
||
Nope, I see it like this on both Linux and macos.
Is this a Windows specific issue?
Reporter | ||
Comment 10•5 years ago
|
||
(In reply to Alessandro Castellani (:aleca) from comment #9)
Created attachment 9075299 [details]
Screen Shot 2019-07-02 at 12.02.16 AM.pngNope, 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.
Assignee | ||
Comment 11•5 years ago
|
||
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
Reporter | ||
Comment 12•5 years ago
|
||
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•5 years ago
|
||
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•5 years ago
|
||
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.
Assignee | ||
Comment 15•5 years ago
|
||
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.
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 16•5 years ago
|
||
Removed some leftover comments.
Comment 17•5 years ago
|
||
Updated•5 years ago
|
Comment 18•5 years ago
|
||
Updated•5 years ago
|
Comment 19•5 years ago
|
||
Comment 20•5 years ago
|
||
Assignee | ||
Comment 21•5 years ago
|
||
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•5 years ago
|
||
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/8fbca3217e6e
Attachment list messagepane display regressions. r=jorgk
Comment 23•5 years ago
|
||
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•5 years ago
|
||
Comment 25•5 years ago
|
||
(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®exp=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®exp=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•5 years ago
|
||
TB 68 beta 4:
https://hg.mozilla.org/releases/comm-beta/rev/521024a6a7e93b59983c2a8dcf19b2bae895fc4e
Reporter | ||
Comment 27•5 years ago
|
||
thanks, and r+ as it's no longer entirely broken, but please address Bug 1563793.
Description
•