Closed Bug 1766073 Opened 3 years ago Closed 3 years ago

Fix dragging links and images from various sources on top of the composition window

Categories

(Thunderbird :: Message Compose Window, defect, P1)

Tracking

(Not tracked)

RESOLVED FIXED
102 Branch

People

(Reporter: aleca, Assigned: aleca)

References

(Regression)

Details

(Keywords: regression)

Attachments

(1 file)

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

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.)

(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!

We can assist with testing on Windows primarily and the other platforms if need be. Thanks for addressing the issues.

Attachment #9273519 - Attachment description: WIP: Bug 1766073 - Enable proper handling of attachable URLs dragged on the composer. → Bug 1766073 - Enable proper handling of attachable URLs dragged on the composer. r=mkmelin

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.

Flags: needinfo?(rachel)

What's the info you require? You want us to do some testing and give feedback? One nit: inline overlay when draggin (missing "g").

Flags: needinfo?(rachel)

Tested the following scenarios on Windows:

  1. dragged "non-file" URL onto composition: working, inserts link as text link
  2. 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?
  3. dragged embedded image onto composition: working, gets embedded again
  4. 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?

Thank you so much for taking the time in testing the patch.

(In reply to Rachel Martin from comment #8)

  1. 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.

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 :-(

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.

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.

Patch updated to use the "prettyName" of the dragged URL for both attachment and inline link creation.

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?

Mh, I have spellchecker installed but that word was not highlighted

Target Milestone: --- → 102 Branch

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

Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Regressions: 1769929
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: