Closed Bug 1342057 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: DOM: Events, defect)

50 Branch
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla55
Tracking Status
firefox51 --- wontfix
firefox52 --- wontfix
firefox-esr52 54+ fixed
firefox53 --- fixed
firefox54 --- fixed
firefox55 --- verified

People

(Reporter: rleafey, Assigned: Nika)

References

Details

(Keywords: regression)

Attachments

(3 files, 1 obsolete file)

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.
+1
Regression range:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=b2409b36cd07bba045bf38dd131166c36ea3da4a&tochange=3d0aef4586bdbd81591811ec627c85161fc11065
Blocks: 906420
Status: UNCONFIRMED → NEW
Component: Untriaged → DOM: Events
Ever confirmed: true
Flags: needinfo?(michael)
Keywords: regression
OS: Unspecified → All
Hardware: Unspecified → All
Version: 51 Branch → 50 Branch
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().
Flags: needinfo?(michael)
Summary: Firefox 51.0.1 does not properly respect accept="image/*" attribute on input fields. → DataTransfer.items always uses application/x-moz-file as mime-type for file items
Assignee: nobody → michael
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
Attachment #8841081 - Flags: review?(amarchesini)
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
Attachment #8841082 - Flags: review?(amarchesini)
We will have 52 RC this week. Won't fix for 51 and let's see if we can take this in 52.
Attachment #8841082 - Flags: review?(amarchesini) → review+
Attachment #8841081 - Flags: review?(amarchesini) → review+
Pushed by michael@thelayzells.com:
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 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
Attachment #8841081 - Flags: approval-mozilla-release?
Attachment #8841081 - Flags: approval-mozilla-esr52?
Attachment #8841081 - Flags: approval-mozilla-beta?
Attachment #8841081 - Flags: approval-mozilla-aurora?
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.
Flags: needinfo?(michael)
Pushed by michael@thelayzells.com:
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
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
Attachment #8841082 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/742fc1cf3d28
https://hg.mozilla.org/mozilla-central/rev/5891f51354b2
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Flags: in-testsuite+
Hi Brindusa, could you help find someone to verify if this issue was fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(brindusa.tot)
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.
Status: RESOLVED → VERIFIED
Flags: needinfo?(brindusa.tot)
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.
Attachment #8841081 - Flags: approval-mozilla-release?
Attachment #8841081 - Flags: approval-mozilla-release-
Attachment #8841081 - Flags: approval-mozilla-beta?
Attachment #8841081 - Flags: approval-mozilla-beta+
Attachment #8841081 - Flags: approval-mozilla-aurora?
Attachment #8841081 - Flags: approval-mozilla-aurora+
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 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
Attachment #8841081 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
You need to log in before you can comment on or make changes to this bug.