Wrong attachment icons with Filelink
Categories
(Thunderbird :: FileLink, defect)
Tracking
(thunderbird_esr91+ fixed, thunderbird95 affected)
People
(Reporter: je, Assigned: TbSync)
References
Details
Attachments
(2 files, 1 obsolete file)
48 bytes,
text/x-phabricator-request
|
wsmwk
:
approval-comm-esr91+
|
Details | Review |
48 bytes,
text/x-phabricator-request
|
wsmwk
:
approval-comm-esr91+
|
Details | Review |
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:68.0) Gecko/20100101 Firefox/68.0
Steps to reproduce:
This happens with TB 64 Bit 78.2 <=78.2.3 on Windows 10 64 Bit.
- Add an attachment (it now has the icon of the Windows file type).
- Convert the attachment to an Filelink attachment (it now has the Filelink provider's icon).
- Add another attachment
Actual results:
The icon of the first attachment changes back to Windows' file type icon.
Expected results:
The icon should remain the Filelink providers icon.
Updated•4 years ago
|
Updated•4 years ago
|
Reporter | ||
Comment 1•3 years ago
|
||
This still happens in Thunderbird 91 and also on Ubuntu 20.
Assignee | ||
Updated•3 years ago
|
Assignee | ||
Comment 2•3 years ago
|
||
Updated•3 years ago
|
Assignee | ||
Updated•3 years ago
|
Assignee | ||
Updated•3 years ago
|
Comment 3•3 years ago
|
||
(minor so somewhat unclear if warranted for 91)
Assignee | ||
Comment 4•3 years ago
|
||
It is a visual annoyance and only needs a minor unproblematic patch. Let's get this on ESR, but let it right the train.
Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/73a0f03a037d
Do not forget cloud icons during refresh. r=mkmelin
Assignee | ||
Comment 6•3 years ago
|
||
Can we back this out? I did not know that the loaded flag was not only used by cloudFile in compose but also while fetching emails. The fix must look different and having the regression fix ontop of this one, is bad for the history / readability. I also want to add some tests which will take a bit of time and do not want this the regression to exist for so long in the product.
Assignee | ||
Updated•3 years ago
|
Backout by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/eba5ece82c4e
Backed out changeset 73a0f03a037d for causing bug 1741195. r=backout
Assignee | ||
Updated•3 years ago
|
Updated•3 years ago
|
Assignee | ||
Comment 9•3 years ago
|
||
Assignee | ||
Updated•3 years ago
|
Assignee | ||
Updated•3 years ago
|
Comment 10•3 years ago
|
||
Did you test to see how this works with a external http attachment? The comment states why the loading indicator was put where it was put.
Assignee | ||
Comment 11•3 years ago
•
|
||
I have no idea what comment you are talking about. Feel free to add more tests like I did with this patch, as this section is/was heavily missing tests.
Without a clue what you mean, I did test to attach web pages and that looked ok.
Comment 12•3 years ago
|
||
This comment, right above code you removed:
// Set elements busy to show the user this is potentially a long network
// fetch for the link attachment.
I presume you know how to link those comment lines to this bug, so you can understand the usage and testcases there.
Your defensive tone is unwarranted; the goal here is to avoid regressions. This bug has already been backed out once, and it's disappointing to see this project operating on a death by a thousand bug reports strategy. Finally, do not ask others to do work for you.
Assignee | ||
Comment 13•3 years ago
|
||
I did not remove that code, I moved it into refreshAttachmentIcon()
which is now called by setAttachmentLoaded()
.
And it was your tone which I found unwarranted.
Comment 14•3 years ago
|
||
Pushed by thunderbird@calypsoblue.org:
https://hg.mozilla.org/comm-central/rev/6ab422bc28d9
Handle cloud icons separately and improve state awareness of refreshAttachmentIcon(). r=mkmelin
Comment 15•3 years ago
|
||
Looks like test failure @ https://treeherder.mozilla.org/logviewer?job_id=358883647&repo=comm-central&lineNumber=3340
Comment 16•3 years ago
|
||
(In reply to alta88 from comment #10)
Did you test to see how this works with a external http attachment? ,,,
See attachment "test" in bug 1284004.
Assignee | ||
Comment 17•3 years ago
|
||
Assignee | ||
Comment 18•3 years ago
•
|
||
It looks like I deliberately changed the tested file to force the test to fail, to check whether the test is actually run or not. Sorry that it slipped into the final patch.
Updated•3 years ago
|
Comment 19•3 years ago
|
||
I've tested both detached and external attachments (test case from bug 1284004). Both appear to work, however, there's something wrong with the highlight/focus (background) colors, the result can be white on white or blue on blue (on Windows).
Comment 20•3 years ago
|
||
Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/759e39ffe7ca
Fix typo in browser_repeat_upload.js and check the correct file. r=mkmelin
Assignee | ||
Comment 21•3 years ago
|
||
(In reply to newsfan from comment #19)
I've tested both detached and external attachments (test case from bug 1284004). Both appear to work, however, there's something wrong with the highlight/focus (background) colors, the result can be white on white or blue on blue (on Windows).
Thanks for your help. Does the unreadable situation occur for you only when hovering over the selected and active/focused attachment?
Comment 22•3 years ago
|
||
Yes, well, sort of.
"Link" attachment (external or detached): Background goes blue if selected attachment is not hovered --> blue on blue.
Normal attachment: Background goes white if selected attachment is hovered --> white on white.
Do you see that as well? I can (or you can) file a bug if there isn't one already (didn't check).
Assignee | ||
Comment 23•3 years ago
|
||
Yes, I see that as well. Moz-regression pointed to
https://phabricator.services.mozilla.com/D131187
But I am not 100% sure that this is true, but it does not seem to be a regression caused by this bug. Filing a new bug would be great, thanks.
Comment 24•3 years ago
|
||
Bug 1742567. I didn't imply that the regression was caused here. Are you sure about the result of moz-regression? That's a total backend/C++ change unrelated to theming/CSS.
Assignee | ||
Comment 25•3 years ago
|
||
I didn't imply that the regression was caused here
This patch of course could have been the cause for it, so I checked and made sure it isn't. Your reporting was totally fine.
Are you sure about the result of moz-regression?
No, I am not sure. It could also be some change in mozilla-central, which landed during the same time. We will have to investigate. But let us move that discussion to Bug 1742567.
Assignee | ||
Comment 26•3 years ago
•
|
||
Comment on attachment 9251518 [details]
Bug 1667652 - Handle cloud icons separately and improve state awareness of refreshAttachmentIcon(). r=mkmelin
[Approval Request Comment]
Regression caused by (bug #):
User impact if declined:
Missing out the improved icons of cloud files.
Testing completed (on c-c, etc.):
Baked on 96 for more than 3 weeks.
Risk to taking this patch (and alternatives if risky):
Low.
Assignee | ||
Comment 27•3 years ago
•
|
||
Comment on attachment 9252031 [details]
Bug 1667652 - Fix typo in browser_repeat_upload.js and check the correct file. r=mkmelin
[Approval Request Comment]
Regression caused by (bug #):
User impact if declined:
Missing out the improved icons of cloud files.
Testing completed (on c-c, etc.):
Baked on 96 for more than 3 weeks.
Risk to taking this patch (and alternatives if risky):
Low.
Comment 28•3 years ago
|
||
Comment on attachment 9252031 [details]
Bug 1667652 - Fix typo in browser_repeat_upload.js and check the correct file. r=mkmelin
[Triage Comment]
Approved for esr91
Comment 29•3 years ago
|
||
Comment on attachment 9251518 [details]
Bug 1667652 - Handle cloud icons separately and improve state awareness of refreshAttachmentIcon(). r=mkmelin
[Triage Comment]
Approved for esr91
Comment 30•3 years ago
|
||
bugherder uplift |
Thunderbird 91.4.1:
https://hg.mozilla.org/releases/comm-esr91/rev/92e1d805bd7a
Comment 31•3 years ago
|
||
bugherder uplift |
Thunderbird 91.4.1:
https://hg.mozilla.org/releases/comm-esr91/rev/11529576c3a7
Description
•