1290688 - Pasting an image from clipboard yields two clipboardData.files and three clipboardData.types (since Bug 906420)

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Claas Augner [:claas]]

Attachment: Paste event test page.

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:50.0) Gecko/20100101 Firefox/50.0

Steps to reproduce:
1. Copy image to clipboard (e.g. using Gimp or Gnome-integrated screenshot shortcut).
2. Paste image into paste event debug page (see attachment *paste_image.html*).

Actual result:
1. `event.clipboardData.files` has two named entries "image.png" and "image.jpeg" of types "image/png" and "image/jpeg" respectively.
2. `event.clipboardData.types` has three entries "image/png", "image/jpeg" and "Files".

Expected result:
1. `event.clipboardData.files` should only have one entry "image.png".
2. `event.clipboardData.types` should not have entry "Files", because the image was pasted and not dragged, see [0].

Result:
1. Sites that support both "image/png" and "image/jpeg" (*) will treat these like two separate files (instead of one).
2. Sites that respect the "Files" type entry [*] may identify one or two additional files (instead of none).

[*] I noticed this right away after switching to current Firefox Nightly (50.0), because I tend to paste screenshots a lot into Discourse [1] both on Windows (Greenshot) and Linux (Gnome shortcuts). While the second result could be due to a hack in Discourse that specifically enables image pasting in Firefox [2], having both files and items at the same time and three `types` entries looks like a pitfall to me.

[0] https://html.spec.whatwg.org/multipage/interaction.html#the-datatransfer-interface
[1] https://meta.discourse.org/
[2] https://github.com/discourse/discourse/blob/3200d836f70a4233c3d7b8ed7605ba1d2720d314/app/assets/javascripts/discourse/components/composer-editor.js.es6#L297-L377

Comment 1

[Claas Augner [:claas]]

Attachment: Screenshot of paste event test page in Firefox 50.

Comment 2

[Claas Augner [:claas]]

Attachment: Screenshot of paste event test page in Firefox 47.

Comment 3

[Claas Augner [:claas]]

Attachment: Screenshot of paste event test page in Chrome 52.

Comment 4

[Claas Augner [:claas]]

Hi Michael, as you worked on Bug 906420: Would you say this is the intended behavior?

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Claas, is this a regression?

Comment 6

[Claas Augner [:claas]]

Olli, hard to say. As the feature (i.e. pasteEvent.clipboardData.files|items|types) was not implemented in FF 49 and earlier, I wouldn't call this a regression. Rather it makes things better and worse at the same time.

Let me rephrase the two problems that I see will arise and potentially frustrate both users and developers (who might then blame FF, which we should avoid):

1. Sites that used a workaround to get the pasted image (cf. Discourse hack) will need to adapt the change, i.e. switch to the HTML spec's standard behavior. If they don't, this will almost certainly lead to images being pasted two to four times instead of just once (depending on implementation).
=> We need to make sure that we communicate this change well.
=> We should give developer instructions on how to feature detect this (w/o parsing the user agent, which is bad practice).

2. With the current implementation being inconsistent with Chrome's implementation (cf. attachment 8776329 [details]), sites will probably still need to differentiate Chrome and Firefox to avoid a pasted image being inserted both as PNG and JPEG in Firefox.
=> If this is a bug, we need to fix this behavior.
=> If this is a feature, we should give developer instructions on how to deal with it (e.g. prefer one type over the other, for which there would need to be a way to detect that both files/items originate from the same image).

PS: Concerning the "Files" type entry, it seems to me like the HTML Standard is giving two conflicting statements:

> In addition, if any files are being dragged, then one of the types will be the string "Files"."
(https://html.spec.whatwg.org/multipage/interaction.html#the-datatransfer-interface)

> If there are any items in the drag data store item list whose kind is File, then add an entry to the list L consisting of the string "Files".
(https://html.spec.whatwg.org/multipage/interaction.html#dom-datatransfer-types)

To me the Chrome 52 behavior (cf. attachment 8776329 [details]) makes more sense, that is the pasteEvent for a pasted image contains no files but one item (of type "image/png") and one type.

PPS: Don't get me wrong: I love that the HTML spec behavior has been implemented here. But we should avoid pitfalls and make the transition smooth and easy for developers.

Comment 7

[Nika Layzell [:nika] (ni? for response)]

I agree that the current behavior seems wrong after my patch. At the very least we should probably not provide every possible format of images when pasting, just pngs.

Chrome's behavior seems super non-spec compliant, as they have a File-type entry in the DataTransfer, and advertise that their DataTransfer.types contains files, but the .files FileList doesn't contain any files.

I think we should:
* Only provide the image.png, no image.jpg
* Only include the type Files in dataTransfer.types
* Include the image in dataTransfer.files
* Include the image in dataTransfer.items as a file item

This _is_ apparently incompatible with Chrome though. Anne, we should try to make sure that the spec agrees with whatever we decide to do :).

Comment 8

[Anne (:annevk)]

Rick, can you help out with comment 7?

Michael, I suggest you file an issue at https://github.com/whatwg/html/issues/new.

Comment 9

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 1: Don't provide more than one type from the clipboard when pasting images

try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=85df624d268d

Comment 10

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 2: Don't include the types of files in DataTransfer.types, as per the spec

Comment 11

[Nika Layzell [:nika] (ni? for response)]

These patches seem to bring us closer to what the spec says to do. We aren't identical to chrome though (assuming that the screenshots are accurate), in that we will include the PNG file in the DataTransfer.files list, rather than only including it in DataTransferItemList.

Comment 12

[Nika Layzell [:nika] (ni? for response)]

Neil, this bug is causing test failures due to browser_clipboard_pastefile.js not passing anymore. This is because the second part of this bug changes the way that DataTransfer.types is implemented such that it checks whether the type of the given argument is actually a File (rather than just looking for application/x-moz-file) before adding the "Files" type.

I would change the test to copy a real file, but unfortunately, we currently don't support copying and pasting files (see bug 1288773). Do you have a recommendation for how I should handle this failing test?

Comment 13

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 3: Add the application/x-moz-file to the DataTransfer.types list even though the spec says not to for back-compat

It seems like application/x-moz-file needs to be added to DataTransfer.types due to internal code which depends on this, even though the spec says that only string types should be added to this list. This patch ensures that application/x-moz-file is present on the DataTransfer.types

Comment 14

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 4: Disable browser_clipboard_pastefile.js due to changes in internal DataTransfer logic making it no longer usable

We talked in person, and I think we agree that there isn't an easy way to write this test correctly with our current setup, due to the difficulty of testing drag & drop in firefox right now. This patch disables the test to avoid the test failures on try.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f8d54321b4d6
Part 1: Don't provide more than one type from the clipboard when pasting images, r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/dd6c459c4f41
Part 2: Don't include the types of files in DataTransfer.types, as per the spec, r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/cbc84ba469d7
Part 3: Add the application/x-moz-file to the DataTransfer.types list even though the spec says not to for back-compat, r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/5150bc3f6d21
Part 4: Disable browser_clipboard_pastefile.js due to changes in internal DataTransfer logic making it no longer usable, r=enndeakin

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f8d54321b4d6
https://hg.mozilla.org/mozilla-central/rev/dd6c459c4f41
https://hg.mozilla.org/mozilla-central/rev/cbc84ba469d7
https://hg.mozilla.org/mozilla-central/rev/5150bc3f6d21

Comment 17

[Rick Byers]

Sorry for the delay, I've been on vacation most of Aug.  [e-mail] (storage team lead) is starting to invest in improving these APIs in blink - I'll ask him to comment.