drag/drop: DataTransfer.types is wrong type

RESOLVED FIXED in Firefox 52

Status

()

Core
DOM: Core & HTML
RESOLVED FIXED
11 months ago
6 months ago

People

(Reporter: Piers Haken, Assigned: bz)

Tracking

({addon-compat, dev-doc-complete, site-compat})

48 Branch
mozilla52
addon-compat, dev-doc-complete, site-compat
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed)

Details

Attachments

(8 attachments)

(Reporter)

Description

11 months ago
User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:48.0) Gecko/20100101 Firefox/48.0
Build ID: 20160817112116

Steps to reproduce:

in firefox, DataTransfer.types is a DOMStringList. it should be a DOMString[].

https://html.spec.whatwg.org/multipage/interaction.html#the-datatransfer-interface
http://w3c.github.io/html/editing.html#the-datatransfer-interface


Actual results:

TypeError: transfer.types.indexOf is not a function


Expected results:

spec compliance
We currently don't support DOMString[] webidl. From the discussion on bug 923919, seems we weren't going to implement this. Any updates on the plan, Boris?
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 2

11 months ago
There is nothing to support; DOMString[] is not valid IDL at all.  See https://github.com/whatwg/html/issues/11 for the year-old spec issue to sort out how it should actually work.

Once the HTML spec gets its act together and decides what the API should actually look like, we will align with that API.
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 3

11 months ago
In the meantime, I guess we can look into what other engines do here...  One option is to just implement something and end up with a de-facto standard in the face of the spec not being useful here.
Hi Stone, would you mind checking comment 3 since you are investigating our drag/drop implementation recently?
Flags: needinfo?(sshih)

Updated

11 months ago
Assignee: nobody → sshih
Flags: needinfo?(sshih)

Comment 5

10 months ago
According to current spec definition [1], the type of DataTransfer#types is DOMString[] (was DOMStringList [2]). In gecko [3], DataTransfer#types uses DOMStringList. Search chromium codes [4], the type of DataTransfer#types is DOMString[].

Tested on different browsers, Edge does not support indexOf but Chrome/Canary does. Looks like we follow old spec [2] and chromium follows current spec. Since the type of DataTransfer#types is going to be changed, maybe we can add a wrapper of DOMStringList with indexOf supported as a short-term solution and replace it when the spec is updated. Or, we leave it as it is and change it when spec is updated.

[1] https://html.spec.whatwg.org/multipage/interaction.html#the-datatransfer-interface
[2] https://www.w3.org/TR/2011/WD-html5-20110113/dnd.html#the-datatransfer-interface
[3] http://searchfox.org/mozilla-central/source/dom/webidl/DataTransfer.webidl#21
[4] https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/clipboard/DataTransfer.idl?q=datatransfer.idl&sq=package:chromium&dr&l=40
(Assignee)

Comment 6

10 months ago
> Search chromium codes [4], the type of DataTransfer#types is DOMString[]

Right, but what does DOMString[] actually map to in Chrome?  They certainly don't implement the old IDL DOMString[] type.  Does it just return a js Array?  Something else?  That's what needs checking.

> Or, we leave it as it is and change it when spec is updated.

The goal here is to figure out what the spec should be updated _to_ and tell Anne, then implement it.

What does Safari return?  I can check that given steps to reproduce, but this bug has no steps to reproduce...

Comment 7

10 months ago
I'm sure but looks like the return type of chromium[1] and webkit[2] is Vector<String>. Trying to make sure it is. [3] is the sample page I used with some manually added console logs for testing.

[1] https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/clipboard/DataTransfer.cpp?q=DataTransfer.cp&sq=package:chromium&l=187
[2] https://github.com/WebKit/webkit/blob/master/Source/WebCore/dom/DataTransfer.cpp#L147
[3] http://www.w3schools.com/html/tryit.asp?filename=tryhtml5_draganddrop
(Assignee)

Comment 8

10 months ago
The C++ return type is.  The question is what the JS reflection of that ends up being.

Thanks for the testcase link.  Looks like in Chrome at least the return type is a plain array, and it returns a new object each time.

Spec issue for this is https://github.com/whatwg/html/issues/11 and I'll follow up there.
(Assignee)

Comment 9

10 months ago
https://github.com/whatwg/html/issues/11#issuecomment-249155408
(Assignee)

Updated

10 months ago
Depends on: 1306220

Comment 10

10 months ago
https://github.com/whatwg/html/pull/1860
(Assignee)

Updated

10 months ago
Depends on: 946906
(Assignee)

Updated

10 months ago
Blocks: 1308287
(Assignee)

Comment 11

10 months ago
Created attachment 8798728 [details] [diff] [review]
part 1.  Add some more documentation to DataTransferItemList
Attachment #8798728 - Flags: review?(michael)
(Assignee)

Updated

10 months ago
Assignee: sshih → bzbarsky
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
(Assignee)

Comment 12

10 months ago
Created attachment 8798730 [details] [diff] [review]
part 2.  Make the type of a DataTransferItem immutable
Attachment #8798730 - Flags: review?(michael)
(Assignee)

Comment 13

10 months ago
Created attachment 8798731 [details] [diff] [review]
part 3.  Restrict mutation of a DataTransferItem's kind to codepaths that are actually changing its data
Attachment #8798731 - Flags: review?(michael)
(Assignee)

Comment 14

10 months ago
Created attachment 8798732 [details] [diff] [review]
part 4.  Drop the pointless ErrorResult from the DataTransferItemList indexed getter
Attachment #8798732 - Flags: review?(michael)
(Assignee)

Comment 15

10 months ago
Created attachment 8798733 [details] [diff] [review]
part 5.  Notify the DataTransfer whenever its types list changes
Attachment #8798733 - Flags: review?(michael)
(Assignee)

Comment 16

10 months ago
Created attachment 8798734 [details] [diff] [review]
part 6.  Remove the unused xpidl versions of DataTransfer.types and DataTransfer.getData
Attachment #8798734 - Flags: review?(michael)
(Assignee)

Comment 17

10 months ago
Created attachment 8798735 [details] [diff] [review]
part 7.  Get rid of uses of .contains() on a DataTransfer's .types in our code, since arrays don't have it

Gijs, I figure someone who's a Firefox/Toolkit peer should look at this part.  It'll be folded with part 8 when I land, since DOMStringList doesn't have .includes() on it.
Attachment #8798735 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Comment 18

10 months ago
Created attachment 8798736 [details] [diff] [review]
part 8.  Change DataTransfer.types from being a DOMStringList to being a frozen array
Attachment #8798736 - Flags: review?(michael)

Comment 19

10 months ago
Comment on attachment 8798735 [details] [diff] [review]
part 7.  Get rid of uses of .contains() on a DataTransfer's .types in our code, since arrays don't have it

Review of attachment 8798735 [details] [diff] [review]:
-----------------------------------------------------------------

rs=me
Attachment #8798735 - Flags: review?(gijskruitbosch+bugs) → review+

Updated

10 months ago
Attachment #8798728 - Flags: review?(michael) → review+

Updated

10 months ago
Attachment #8798730 - Flags: review?(michael) → review+
Comment on attachment 8798731 [details] [diff] [review]
part 3.  Restrict mutation of a DataTransferItem's kind to codepaths that are actually changing its data

Review of attachment 8798731 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/events/DataTransfer.cpp
@@ +1475,5 @@
>                                          nsIPrincipal* aPrincipal)
>  {
>    RefPtr<DataTransferItem> item = new DataTransferItem(this,
> +                                                       NS_LITERAL_STRING(kCustomTypesMime),
> +                                                       DataTransferItem::KIND_STRING);

It would be nice if we could also set the kind correctly in DataTransferItemList::Add() to avoid the extra mutation in SetData().

::: dom/events/DataTransferItem.h
@@ +100,5 @@
>      mPrincipal = aPrincipal;
>    }
>  
>    already_AddRefed<nsIVariant> DataNoSecurityCheck();
>    already_AddRefed<nsIVariant> Data(nsIPrincipal* aPrincipal, ErrorResult& aRv);

I'm not sure if you change this in future patches (as I haven't read them yet), but I'm pretty sure that Data() can also change the Kind of the data, as it calls SetData() internally if it needs to fill in external data.
Attachment #8798731 - Flags: review?(michael) → review+
Comment on attachment 8798732 [details] [diff] [review]
part 4.  Drop the pointless ErrorResult from the DataTransferItemList indexed getter

Review of attachment 8798732 [details] [diff] [review]:
-----------------------------------------------------------------

This is great. I think I used to do permissions check in IndexedGetter which is why it was marked as [Throws]. Now that I don't anymore this is much nicer.
Attachment #8798732 - Flags: review?(michael) → review+
Comment on attachment 8798733 [details] [diff] [review]
part 5.  Notify the DataTransfer whenever its types list changes

Review of attachment 8798733 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/events/DataTransferItem.cpp
@@ +230,1 @@
>    }

I really don't like this code path. A bit of me wants to change this codepath to not allow for the Kind() to change when we fill in external data, especially because it means that a file can disappear from mItems after trying to get its value.

This would mean that we would just always produce null in this situation.

I think that should be left to a follow-up however, so this solution is good for now.

::: dom/events/DataTransferItemList.cpp
@@ +444,5 @@
> +  if (item->Kind() == DataTransferItem::KIND_FILE || aIndex == 0) {
> +    mDataTransfer->TypesListMayHaveChanged();
> +    if (!aHidden) {
> +      mItems.AppendElement(item);
> +    }

I feel like we should probably be adding the item to mItems before notifying mDataTransfer so that it sees the correct state when that function is being called. If we're only invalidating our cache it should be fine, but I still would feel more comfortable with this being the other way around.
Attachment #8798733 - Flags: review?(michael) → review+
Comment on attachment 8798734 [details] [diff] [review]
part 6.  Remove the unused xpidl versions of DataTransfer.types and DataTransfer.getData

Review of attachment 8798734 [details] [diff] [review]:
-----------------------------------------------------------------

\o/
Attachment #8798734 - Flags: review?(michael) → review+
Comment on attachment 8798736 [details] [diff] [review]
part 8.  Change DataTransfer.types from being a DOMStringList to being a frozen array

Review of attachment 8798736 [details] [diff] [review]:
-----------------------------------------------------------------

I think this is generally good, but I'm a bit worried about the correctness of the caching (partially because I don't understand how webidl caching works).

::: dom/events/DataTransfer.cpp
@@ +315,1 @@
>  {

Should we Clear() aTypes? If aTypes is non-empty this function will act super strangely.

@@ +321,5 @@
>    for (uint32_t i = 0; i < items->Length(); i++) {
>      DataTransferItem* item = items->ElementAt(i);
>      MOZ_ASSERT(item);
>  
>      if (item->ChromeOnly() && !nsContentUtils::LegacyIsCallerChromeOrNativeCode()) {

Can we use [SubjectPrincipal] to get rid of this call? Does [SubjectPrincipal] work with [Pure, Cached, Frozen]?

::: dom/webidl/DataTransfer.webidl
@@ +17,5 @@
>    [Throws]
>    void setDragImage(Element image, long x, long y);
>  
> +  [Pure, Cached, Frozen]
> +  readonly attribute sequence<DOMString> types;

Will this handle that Chrome code should see a different types array correctly? I don't want some addon code having looked at dataTransfer.types causing data leakage to content.
Attachment #8798736 - Flags: review?(michael)
(Assignee)

Comment 25

10 months ago
> It would be nice if we could also set the kind correctly in DataTransferItemList::Add()

Do you mean in DataTransferItemList::AppendNewItem?  Because in Add() we're sometimes mutating an existing item-place, so do in fact needs to change kind under SetData().

For AppendNewItem, I guess I could basically hoist the KindFromData() call out of SetData() or something, but since SetData() needs to do it anyway....  Not sure what general behavior you're after here.

> but I'm pretty sure that Data() can also change the Kind of the data

Indeed.  As you note, it does this via calling SetData() and that SetData() callsite is included in the "must notify the datatransfer" list.  ;)

> I really don't like this code path.

I assume you mean the FillInExternalData() bits (yay splinter's broken quoting)?

I agree that it's ugly and all that.  I'm glad you don't want me to fix it here and now.  ;)

> I feel like we should probably be adding the item to mItems before notifying
> mDataTransfer so that it sees the correct state when that function is being called.

Good catch.  Done.

> Should we Clear() aTypes? If aTypes is non-empty this function will act super strangely.

The bindings will always call it with empty aTypes.

I guess I can add a Clear() call, especially because we have internal consumers.

> Can we use [SubjectPrincipal] to get rid of this call?

Yes, that's bug 1308287.

> Will this handle that Chrome code should see a different types array correctly?

Yes, once bug 946906 is fixed.  That's why it's blocking this bug.
Comment on attachment 8798736 [details] [diff] [review]
part 8.  Change DataTransfer.types from being a DOMStringList to being a frozen array

Review of attachment 8798736 [details] [diff] [review]:
-----------------------------------------------------------------

Makes sense, in that case r+
Attachment #8798736 - Flags: review+

Comment 27

10 months ago
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6147b5ffb4a0
part 1.  Add some more documentation to DataTransferItemList.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a4c0703e825
part 2.  Make the type of a DataTransferItem immutable.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/1873ffaafd25
part 3.  Restrict mutation of a DataTransferItem's kind to codepaths that are actually changing its data.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/5725d27a0bf1
part 4.  Drop the pointless ErrorResult from the DataTransferItemList indexed getter.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/d15ec8c5a75f
part 5.  Notify the DataTransfer whenever its types list changes.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/1493789a0ce5
part 6.  Remove the unused xpidl versions of DataTransfer.types and DataTransfer.getData.  r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/67d40adfa5e1
part 7.  Change DataTransfer.types from being a DOMStringList to being a frozen array.  r=mystor,gijs

Comment 28

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6147b5ffb4a0
https://hg.mozilla.org/mozilla-central/rev/9a4c0703e825
https://hg.mozilla.org/mozilla-central/rev/1873ffaafd25
https://hg.mozilla.org/mozilla-central/rev/5725d27a0bf1
https://hg.mozilla.org/mozilla-central/rev/d15ec8c5a75f
https://hg.mozilla.org/mozilla-central/rev/1493789a0ce5
https://hg.mozilla.org/mozilla-central/rev/67d40adfa5e1
Status: ASSIGNED → RESOLVED
Last Resolved: 10 months ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
(Assignee)

Comment 29

10 months ago
This will have addon compat impact; lots of addons are doing things like "event.dataTransfer.types.contains".  Unfortunately, there really isn't a good way to keep that working while following the spec.  The relevant addons should change to event.dataTransfer.types.includes if they only need to target current Firefox.  If they need to stay compatible across a range of versions, then something like this:

  var func = event.dataTransfer.types.contains || event.dataTransfer.types.includes;
  func.call(event.dataTransfer.types, args);

will work...  There may be other patterns that end up calling contains() on a .types return value too; searching for "contains(" in addons MXR gives way too much noise to tell.

Jorge, if you think this will cause serious problems, I can try to add some sort of backwards-compat shim in this case (e.g. define "contains" as an alias for "includes" on this one array return value, when called from chrome code).  Please let me know?
Flags: needinfo?(jorge)
Keywords: addon-compat, dev-doc-needed

Updated

10 months ago
Blocks: 1309296
Do these (and some others) not need to be changed from contains to includes too?

https://dxr.mozilla.org/mozilla-central/source/dom/base/contentAreaDropListener.js#36
https://dxr.mozilla.org/mozilla-central/source/editor/libeditor/EditorEventListener.cpp#942
(Assignee)

Comment 31

10 months ago
> https://dxr.mozilla.org/mozilla-central/source/dom/base/contentAreaDropListener.js#36

This one does not, because I didn't change the thing returned by mozTypesAt.

> https://dxr.mozilla.org/mozilla-central/source/editor/libeditor/EditorEventListener.cpp#942

This has nothing to do with the JS array includes/contains mess.  Contains() on the C++ type is the right thing to use there; it wouldn't have compiled otherwise.  ;)
Sorry you are right. It was cpp not js. Braindead. Thanks for the js explanation.
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #29)
> Jorge, if you think this will cause serious problems, I can try to add some
> sort of backwards-compat shim in this case (e.g. define "contains" as an
> alias for "includes" on this one array return value, when called from chrome
> code).  Please let me know?

It does appear to be used heavily, so a shim would be ideal in this case.
Flags: needinfo?(jorge)
(Assignee)

Comment 34

10 months ago
OK, filed bug 1309970 on adding such a shim.
Depends on: 1309970
Posted the site compatibility doc: https://www.fxsitecompat.com/en-CA/docs/2016/datatransfer-types-is-now-domstringlist-instead-of-array/
Keywords: site-compat
I've documented this in relevant places:

https://developer.mozilla.org/en-US/docs/Web/API/DataTransfer
https://developer.mozilla.org/en-US/docs/Web/API/DataTransfer/types
https://developer.mozilla.org/en-US/docs/Web/API/HTML_Drag_and_Drop_API/Recommended_drag_types#Updates_to_DataTransfer.types
https://developer.mozilla.org/en-US/docs/Web/API/HTML_Drag_and_Drop_API/Drag_operations#Updates_to_DataTransfer.types

And added a note to the Fx 52 release notes:

https://developer.mozilla.org/en-US/Firefox/Releases/52#DOM_HTML_DOM

Let me know if these updates look ok. Thanks!
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.