2061788 - Dragging an inline 'data:' image within an editor inserts the base64 data URL as text instead of moving the image

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Hartmut Welpmann [:welpy-cw]]

Attachment: bug2058559-repro.html

Running mozregression on Thunderbird using the STR from bug 2058559 produced these regression windows:
https://hg-edge.mozilla.org/comm-central/pushloghtml?fromchange=93868b0908eb8808f2a8d257861d196d48b807da&tochange=73f2407abd386cd2e47dfb755b0083e2f270e89f
https://hg-edge.mozilla.org/mozilla-central/pushloghtml?fromchange=acbfb05622cd827a7418d36628f75857628a81ce&tochange=796ebba319e077c8b79384b5812a0f88546cd7d4.

This was identified as the likely regressor, here's the AI-assisted assessment and testcase:

Steps to reproduce

1. Open the attached HTML testcase in Firefox (a contenteditable containing
   an <img> whose src is a data:image/...;base64,... URL).
2. Drag the inline image to a new position inside the contenteditable.
   (Reported in Thunderbird's compose editor: drop a photo inline, then drag it
   to another spot in the message body.)

Actual results

The image is not moved. Instead the raw base64 text
(data:image/png;base64,...) is inserted at the drop point as a text node, and
the image is removed.

Expected results

Per HTML5 drag-and-drop, an intra-document drag is a move: the <img>
element should move to the drop point. No text should be inserted.

What the change does

editor/libeditor/HTMLEditorDataTransfer.cpp:

• HTMLTransferablePreparer::AddDataFlavorsInBestOrder() now adds
  kURLDataMime (text/x-moz-url) and orders URL data before text.
• InsertFromDataTransfer() now treats kURLDataMime as insertable text
  (previously only kTextMime / kMozTextInternal), plus a new
  InsertURLAsLinkInternal().
• New editor/libeditor tests: test_pasteURLFromTransferable.html.

widget/cocoa/nsClipboard.mm:

• Reads kURLDataMime from the pasteboard (public.url).

The change was written for paste ("put URL data before text in HTML editors
to preserve pasted links"), but the new URL-as-text branch also catches the
drop path. Dragging an inline image now exposes the URL flavor (the data
URL), and the editor inserts it as text instead of moving the element.

Suggested fix

In HTMLEditor::InsertFromDataTransfer, do not treat kURLDataMime as
insertable text when:

• the value is a data: URL, and/or
• the drag source element belongs to the same document (intra-document drags
  should move, not paste).

Testcase

Attached: bug2058559-repro.html

• Generates a guaranteed-valid data:image/png via canvas.toDataURL().
• Logs dataTransfer types on dragstart/dragover/drop — expect
  text/x-moz-url (kURLDataMime) to be present.
• Auto-detects the outcome (base64 text present in body = FAIL).
• Fails on builds ≥ c69dbf06208e, passes on the build just before — a
  one-changeset confirmation.

Comment 1

[BugBot [:suhaib / :marco]]

Set release status flags based on info from the regressing bug 1717071

Comment 2

[Yury]

(In reply to Hartmut Welpmann [:welpy-cw] from comment #0)

Suggested fix

In HTMLEditor::InsertFromDataTransfer, do not treat kURLDataMime as
insertable text when:

• the value is a data: URL, and/or
• the drag source element belongs to the same document (intra-document drags
  should move, not paste).

How about dragging between two different documents? Like two FF windows or two TB compose windows?

Comment 3

[Yury]

AI suggested edit:

diff --git a/editor/libeditor/HTMLEditorDataTransfer.cpp b/editor/libeditor/HTMLEditorDataTransfer.cpp
--- a/editor/libeditor/HTMLEditorDataTransfer.cpp
+++ b/editor/libeditor/HTMLEditorDataTransfer.cpp
@@ -2422,11 +2422,13 @@ nsresult HTMLEditor::InsertFromDataTrans
                                "InlineStylesAtInsertionPoint::Clear) failed");
           return rv;
         }
       } else if (type.EqualsLiteral(kURLDataMime)) {
-        // Handle URL data before text so HTML editors insert a link here.
         nsAutoString url;
         GetStringFromDataTransfer(aDataTransfer, type, aIndex, url);
+        if (StringBeginsWith(url, u"data:"_ns)) {
+          continue;
+        }
         AutoPlaceholderBatch treatAsOneTransaction(
             *this, ScrollSelectionIntoView::Yes, __FUNCTION__);
         nsresult rv =
             InsertURLAsLinkInternal(url, aDroppedAt, aDeleteSelectedContent);
@@ -2438,8 +2440,11 @@ nsresult HTMLEditor::InsertFromDataTrans
     if (type.EqualsLiteral(kTextMime) || type.EqualsLiteral(kMozTextInternal) ||
         type.EqualsLiteral(kURLDataMime)) {
       nsAutoString text;
       GetStringFromDataTransfer(aDataTransfer, type, aIndex, text);
+      if (type.EqualsLiteral(kURLDataMime) && StringBeginsWith(text, u"data:"_ns)) {
+        continue;
+      }
       AutoPlaceholderBatch treatAsOneTransaction(
           *this, ScrollSelectionIntoView::Yes, __FUNCTION__);
       nsresult rv = InsertTextAt(text, aDroppedAt, aDeleteSelectedContent);
       NS_WARNING_ASSERTION(NS_SUCCEEDED(rv),

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Thank you for the report.

I don't think ignoring data: URL is the right fix because the data transfer may have only the data URL.

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 2061788 - Make `HTMLEditor::InsertFromDataTransfer` use the almost same order as `HTMLTransferablePreparer::AddDataFlavorsInBestOrder` r=edgar!

When we create a DataTransfer from nsITransferable,
HTMLTransferablePreparer::AddDataFlavorsInBestOrder adds the data
with preferred order of HTMLEditor. However,
HTMLEditor::InsertFromDataTransfer is called with DataTransfer
created outside HTMLEditor. Therefore, the order can be any.
However, HTMLEditor should use the same order as far as possible.

However, the coming DataTransfer's data order may be meaningful
especially when it's an image because converting image format may be
lossy conversion. Therefore, this patch does not reorder the images
with clipboard.paste_image_type pref to keep current behavior.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/e64393201929
https://hg.mozilla.org/integration/autoland/rev/313a6f32f387
Make `HTMLEditor::InsertFromDataTransfer` use the almost same order as `HTMLTransferablePreparer::AddDataFlavorsInBestOrder` r=edgar

Comment 8

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/313a6f32f387

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

This is not a recent regression (since 151) and there is no bug reports about major web apps. So, I think the patch just rides the train.

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

And perhaps, we should wait for a couple of weeks to request an uplift for ESR153 because the change may affect to some web apps as a regression.

Comment 11

[Donal Meehan [:dmeehan]]

Did you want to nominate this fix for a relnote? If so, set the relnote-firefox flag to "?"
https://wiki.mozilla.org/Release_Management/Release_Notes_Nomination

Possible wording:

Fixed dragging an image to a new position in a rich-text editor replacing it with a long line of text instead of moving it.

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Release Note Request (optional, but appreciated)
[Why is this notable]: Publishing the regression fix of this bug might lead regression reports if this caused some regressions.
[Affects Firefox for Android]: Yes, but most users do not use DnD on Android, so, mostly "no"
[Suggested wording]: Fixed dragging an image to a new position in a rich-text editor replacing it with a long line of text instead of moving it.
[Links (documentation, blog post, etc)]: N/A

Comment 13

[Donal Meehan [:dmeehan]]

Thanks, added to the Fx156 nightly release notes, please allow 30 minutes for the site to update.

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I think we should uplift this to ESR153 because of a feature broken after ESR140.

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I'd like to request an uplift, but Lando returns error "406 Not Acceptable" from https://lando.moz.tools/uplift/ when I submit the request form, RyanVM, do you know the reason?

Comment 16

[Yury]

You are aware that for the patch to apply you also need to backport bug 2043632 and bug 2055644? And the latter caused a regression in bug 2065855. So either backport four bugs or make a special ESR 153 patch.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900) from comment #15)

I'd like to request an uplift, but Lando returns error "406 Not Acceptable" from https://lando.moz.tools/uplift/ when I submit the request form, RyanVM, do you know the reason?

Probably WAF interference (I've seen this before when it sees certain things in the commit message it doesn't like), but that'd be a question for #Conduit. Given comment 16, you might be better off trying to create a stack locally of whatever dependencies are required and submit with moz-phab uplift --train firefox-esr153 instead.

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Yury from comment #16)

You are aware that for the patch to apply you also need to backport bug 2043632 and bug 2055644? And the latter caused a regression in bug 2065855. So either backport four bugs or make a special ESR 153 patch.

Yeah, no problem. They just added the utility method and changed the behavior for EditContext which has not been shipped yet. So, I can backport the patch simply.

(In reply to Ryan VanderMeulen [:RyanVM] from comment #17)

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900) from comment #15)

I'd like to request an uplift, but Lando returns error "406 Not Acceptable" from https://lando.moz.tools/uplift/ when I submit the request form, RyanVM, do you know the reason?

Probably WAF interference (I've seen this before when it sees certain things in the commit message it doesn't like), but that'd be a question for #Conduit. Given comment 16, you might be better off trying to create a stack locally of whatever dependencies are required and submit with moz-phab uplift --train firefox-esr153 instead.

Thanks. Okay, I'll post a new patch manually.

Comment 19

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 2061788 - Make `HTMLEditor::InsertFromDataTransfer` use the almost same order as `HTMLTransferablePreparer::AddDataFlavorsInBestOrder`

When we create a DataTransfer from nsITransferable,
HTMLTransferablePreparer::AddDataFlavorsInBestOrder adds the data
with preferred order of HTMLEditor. However,
HTMLEditor::InsertFromDataTransfer is called with DataTransfer
created outside HTMLEditor. Therefore, the order can be any.
However, HTMLEditor should use the same order as far as possible.

However, the coming DataTransfer's data order may be meaningful
especially when it's an image because converting image format may be
lossy conversion. Therefore, this patch does not reorder the images
with clipboard.paste_image_type pref to keep current behavior.

Original Revision: https://phabricator.services.mozilla.com/D318030

Comment 20

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Hmm, I cannot post the uplift request from https://lando.moz.tools/uplift/request anyway due to 406 Not Acceptable...

User impact if declined/Reason for urgency:
This is a basic editing feature regression between ESR140 and ESR153. So, users won't be able to drag <img src="data:image/*"> in contenteditable apps as expected.
Code covered by automated testing?: Yes
Fix verified in Nightly?: Yes
Needs manual QE testing?: No
Risk associated with taking this patch: Low
Explanation of risk level:
This patch makes HTMLEditor::InsertFromDataTransfer (which is used for paste handling on macOS and Android) sort the data as almost the same order as the normal paste handler. (Previously, it does not sort the data order to consider which is the best type of data to insert.)
String changes made/needed?: No
Is Android affected?: Yes

Comment 21

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr153/rev/7e0d8fd51c6a