Fix dragging links and images from various sources on top of the composition window
Categories
(Thunderbird :: Message Compose Window, defect, P1)
Tracking
(Not tracked)
People
(Reporter: aleca, Assigned: aleca)
References
(Regression)
Details
(Keywords: regression)
Attachments
(1 file)
(deleted),
text/x-phabricator-request
|
Details |
We need to clean up a bit the code and make sure we're properly handling the various scenarios when users drag something on top the compose window.
[macOS|Windows]
Links dragged onto the compose window can be inserted as links or as attachments. Shows:[INLINE]
and[ATTACH]
[macOS|Windows|Linux]
Attachments of other messages get ONLY added as attachments. Shows:[ATTACH]
[macOS|Windows|Linux]
Images dragged from the body of another message are only appended inline. Shows:NOTHING
, let the<editor>
handle that[macOS|Windows|Linux]
Images can be copied from a webpage and pasted into the editor. Shows:NOTHING
, let the<editor>
handle that
Assignee | ||
Comment 2•3 years ago
|
||
Comment 3•3 years ago
|
||
Drive-by question: What is this code meant to do?
GetCurrentEditor().insertText(
attachment.name ||
gMsgCompose.AttachmentPrettyName(attachment.url, null)
);
Hard to tell since gIsValidInline
seems to be false
all the time, unless we missed something.
Link insertion for a link dragged into a composition is handled by the Mozilla platform if C-C code doesn't interfere. That's done by breaking above on everything that isn't a Ci.nsIMsgMessageFetchPartService (message part) scheme. (We could be wrong, it's been a while since we looked at it.)
Assignee | ||
Comment 4•3 years ago
|
||
(In reply to Rachel Martin from comment #3)
Drive-by question: What is this code meant to do?
GetCurrentEditor().insertText( attachment.name || gMsgCompose.AttachmentPrettyName(attachment.url, null) );
Please, ignore the patch for now as I uploaded it on phabricator to more easily test various things on Linux, macOS, and Windows.
That's why I made the patch a WIP, as it should be ignored for now.
I'll ping you on the patch once it's ready for testing, thanks for looking at it.
Funny note, the data transfer types are different per platform. So what works on Linux doesn't for on macOS or Windows, and vice versa.
Joy!
Comment 5•3 years ago
|
||
We can assist with testing on Windows primarily and the other platforms if need be. Thanks for addressing the issues.
Updated•3 years ago
|
Assignee | ||
Comment 6•3 years ago
|
||
Patch updated and ready for a review.
I updated the original comment of this bug with the docs
of what the outcome of various scenarios should be.
Comment 7•3 years ago
|
||
What's the info you require? You want us to do some testing and give feedback? One nit: inline overlay when draggin
(missing "g").
Comment 8•3 years ago
|
||
Tested the following scenarios on Windows:
- dragged "non-file" URL onto composition: working, inserts link as text link
- dragged file URL (ending with .jpg or .pdf) onto composition: Inserts as text link. Wasn't the aim to restore the original functionality and offer to insert as text (inline) or attachment?
- dragged embedded image onto composition: working, gets embedded again
- dragged attachments of other message onto composition: working, gets attached again.
It's not clear why handleInlineDrop()
is necessary since we achieved adding links without such a function with our patch which behaves exactly as yours and is much simpler and which you gave negative feedback (bug 1732903 comment #39) due to the lack of point 2). What are we missing?
Assignee | ||
Comment 9•3 years ago
|
||
Thank you so much for taking the time in testing the patch.
(In reply to Rachel Martin from comment #8)
- dragged file URL (ending with .jpg or .pdf) onto composition: Inserts as text link. Wasn't the aim to restore the original functionality and offer to insert as text (inline) or attachment?
This on macOS triggers correctly the option to add the link inline or as attachment.
So, I need to handle this for Windows.
Comment 10•3 years ago
|
||
Apologies, it already works for Windows. We went to debug it and it worked. Maybe we've started the wrong build via the wrong shortcut, so embarrassing :-(
Assignee | ||
Comment 11•3 years ago
|
||
Eheh, no worries, that happens to me more often than I'd like to admit.
So, I guess the patch is good for Windows and macOS?
I'll test it on Windows as well later today just to confirm.
After 102 I'm planning to rework and enhance this whole workflow and improve the UI, but for now this maintains the old pre-91 functionalities.
Comment 12•3 years ago
|
||
That said, please compare the attachment name to what TB 78 created. Right now you have the full URL as attachment name (with the slashes) whereas TB 78 only used the file name as attachment name. Also, if you attach an entire web-page, behaviour that was deemed not useful in bug 1732903, the name of the attachment is also the full URL and when sending or saving the message, the attachment, supposedly the HTML page, cannot be opened. That worked in TB 78. Looks like that is due to the filename which has the entire URL including the slashes. So creating a "nice" filename should fix that issue.
Assignee | ||
Comment 13•3 years ago
|
||
Patch updated to use the "prettyName" of the dragged URL for both attachment and inline link creation.
Comment 14•3 years ago
|
||
Nice :-) - Please also fix the typo we mentioned in comment #7: ... user is not draggin an ...
. Time to run codespell over the codebase again (bug 1647104, it's been two years), or has that been automated now?
Assignee | ||
Comment 15•3 years ago
|
||
Mh, I have spellchecker installed but that word was not highlighted
Assignee | ||
Updated•3 years ago
|
Comment 16•3 years ago
|
||
Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/dffed3eeb941
Enable proper handling of attachable URLs dragged on the composer. r=mkmelin
Description
•