1342057 - DataTransfer.items always uses application/x-moz-file as mime-type for file items

Status: VERIFIED FIXED

(Core :: DOM: Events, defect)

Description

[rleafey]

Attachment: Drag Drop over input field with accept="image/*"

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/56.0.2924.87 Safari/537.36

Steps to reproduce:

This was not an issue in previous versions of Firefox, but as soon as I upgraded to v51, the issue presented itself. You can see it in action here: http://okonet.ru/react-dropzone/

1) Inspect the DOM, and note that the input field has an accept attribute set for image/*
2) Drag an image into the Drop Zone and you'll see that the field goes red because it thinks that the file being dropped does not match what is in the accept attribute.
3) Try this in any other browser (Chrome, IE 11, Safari, older versions of Firefox) and you'll see the behavior is correct.  


Actual results:

The input does not recognize the file being entered as acceptable and so the dropzone turns red. 


Expected results:

The input should recognize the file being entered as acceptable and the dropzone should turns green.

Comment 1

[Andrey Okonechnikov]

+1

Comment 2

[Loic]

Regression range:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=b2409b36cd07bba045bf38dd131166c36ea3da4a&tochange=3d0aef4586bdbd81591811ec627c85161fc11065

Comment 3

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

It appears as though this problem isn't specifically a problem with firefox's implementation of <input accepts=> but rather a problem with the react-dropzone library and its usage of the attr-accept library.

In the version of react-dropzone which is in the demo website which you linked (which does not appear to be up-to-date with the version on github), the list of files which will be considered to decide whether or not the dragged files match the accept pattern are obtained with this line:

    var dataTransferItems = e.dataTransfer && e.dataTransfer.items ? e.dataTransfer.items : [];

In versions of firefox < 51, e.dataTransfer.items was null, meaning that the empty list would be used. As `[].every(() => false)` produces `true`, this means that `allFilesAccepted` was always true.

In more recent versions of firefox, we support the dataTransfer.items interface. For annoying legacy reasons which I would like to change when I get the chance, files stored in the dataTransfer.items list are recorded as having the type `application/x-moz-file`, which the attr-accept library does not consider to be matching the pattern `image/*`.

This means that allFilesAccepted is now set to false.

The correct solution on the firefox side seems to be that we start serving the correct mime-type in the dataTransfer.items list for files. I want to do this at some point, but it's a bit of a project and I haven't had the time. I'll repurpose this bug into a bug for fixing that.

As a temporary workaround on the side of the library, the library can either use `dataTransfer.files` to access the set of files (which has the correct mime-type), or get the File object (which has the correct mime-type) from the DataTransferItem with getAsFile().

Comment 4

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

Attachment: Part 1: Use correct MIME type for files as DataTransferItem.type

This is my first shot at changing our behavior with DataTransferItem::GetType to be more consistent with Chrome's behavior.

We have previously always provided `application/x-moz-file` as the DataTransferItem.type property for file data provided by the OS. This changes that behavior to instead detect when the stored type has a KIND_FILE, and instead access the internal dom::File object, and extract the type from it. This produces better results which are more in line with the ones produced by Chrome.

MozReview-Commit-ID: 4hrZ3YoGTJj

Comment 5

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

Attachment: Part 2: Add a test that the MIME type is set correctly for image data

Unfortunately drag and drop / clipboard data is not super easy to test
with our current testing infrastructure, so we can't do too much better
than this. Additional testing will have to be done manually through our
QA process once this has landed.

MozReview-Commit-ID: 1etmIxjjiBd

Comment 6

[Gerry Chang [:gchang]]

We will have 52 RC this week. Won't fix for 51 and let's see if we can take this in 52.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6758f6e0b836
Part 1: Use correct MIME type for files as DataTransferItem.type, r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a1e069911fa
Part 2: Add a test that the MIME type is set correctly for image data, r=baku

Comment 8

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

Comment on attachment 8841081 [details] [diff] [review]
Part 1: Use correct MIME type for files as DataTransferItem.type

Not sure how badly we want this uplifted, as it doesn't have a super huge web compat impact, but there is at least one webpage which is affected (hence the report), and our current behavior is incompatible with Chrome.

Asking for approval-everywhere. Feel free to bother me if you need more information to make a judgement.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: D&D Web-Compat Issues
User impact if declined: Some websites which depend on D&D behavior and were built only on Chrome may not work correctly on Firefox. 
Fix Landed on Version: Nightly
Risk to taking this patch (and alternatives if risky): This makes changes to our web visible behavior which may affect some websites. The primary alternative is to not take it and wait until whatever version does have the change to ship it.
String or UUID changes made by this patch: None

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Approval Request Comment
[Feature/Bug causing the regression]:bug 906420
[User impact if declined]: Some websites which depend on D&D behavior and were built only on Chrome may not work correctly on Firefox. 
[Is this code covered by automated tests?]: Yes (partially - D&D also requires some manual testing due to OS specific file explorer behaviors).
[Has the fix been verified in Nightly?]: Just landed in nightly
[Needs manual test from QE? If yes, steps to reproduce]: We have Drag & Drop manual testing set up using both file sharing websites like Dropbox, and https://mystor.github.io/dragndrop. I will be updating the dragndrop tester to check against the new intended behavior after this patch. 
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: Fairly low risk.
[Why is the change risky/not risky?]: This makes changes to our web visible behavior which may affect some websites. The primary alternative is to not take it and wait until whatever version does have the change to ship it.
[String changes made/needed]: None

Comment 9

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

I had to back these out for failures like https://treeherder.mozilla.org/logviewer.html#?job_id=82222997&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/26ee17ce7b5b

Comment 10

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

Looks like the test part which was added here needs to be disabled on android because of bug 1299578.

I'll re-land with that part of the test disabled on android.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/742fc1cf3d28
Part 1: Use correct MIME type for files as DataTransferItem.type, r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/5891f51354b2
Part 2: Add a test that the MIME type is set correctly for image data, r=baku

Comment 12

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

Attachment: Part 2: Add a test that the MIME type is set correctly for image data

Unfortunately drag and drop / clipboard data is not super easy to test
with our current testing infrastructure, so we can't do too much better
than this. Additional testing will have to be done manually through our
QA process once this has landed.

MozReview-Commit-ID: 1etmIxjjiBd

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/742fc1cf3d28
https://hg.mozilla.org/mozilla-central/rev/5891f51354b2

Comment 14

[Gerry Chang [:gchang]]

Hi Brindusa, could you help find someone to verify if this issue was fixed as expected on a latest Nightly build? Thanks!

Comment 15

[roxleitan]

I tested this bug using the latest Nightly 55.0a1 (2017-03-10) on Ubuntu 16.04, Mac OS X 10.11 and Windows 10 x64.

Verified as fixed using the link from comment 0: http://okonet.ru/react-dropzone/ and with the link from comment 8 https://mystor.github.io/dragndrop.

Comment 16

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8841081 [details] [diff] [review]
Part 1: Use correct MIME type for files as DataTransferItem.type

Let's try this for beta 2. The fix was verified in nightly, and should get us closer to specs for drag and drop. If this causes any new problems with drag and drop, hopefully we'll catch them in normal beta testing.   

Too late for 52 release though.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8fd4bcf2db12
https://hg.mozilla.org/releases/mozilla-aurora/rev/ec2b6e298feb

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/ccc12c0fbf9f
https://hg.mozilla.org/releases/mozilla-beta/rev/fc832b9c1e62

Comment 19

[Liz Henry (:lizzard) (relman/hg->git project)]

Let's let this bake a bit more on release 53, not put it in 52.1.0, and re-consider it for 52.2.0. This seems risky, but potentially important/widespread for web compatibility.

Comment 20

[Julien Cristau [:jcristau]]

Comment on attachment 8841081 [details] [diff] [review]
Part 1: Use correct MIME type for files as DataTransferItem.type

no apparent regressions reported since this shipped in 53, so I'll go ahead and approve for esr52.2

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/278bef1d7a64
https://hg.mozilla.org/releases/mozilla-esr52/rev/b6d5f7f945de