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?

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.

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?

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

> 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...

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

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.

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.

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

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.

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.

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.

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/

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.

> 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+

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

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

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?

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

> 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.

OK, filed bug 1309970 on adding such a shim.

Posted the site compatibility doc: https://www.fxsitecompat.com/en-CA/docs/2016/datatransfer-types-is-now-domstringlist-instead-of-array/

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!