dataTransfer.types (HTML5 drag & drop) should return DOMStringList

• Component: DOM
• Status: RESOLVED WONTFIX

Currently returns flat array. Firefox nightlies implement this correctly.

[Alexey Proskuryakov]

*** Bug 55837 has been marked as a duplicate of this bug. ***

[Alexey Proskuryakov]

*** Bug 73968 has been marked as a duplicate of this bug. ***

[Eric Seidel (no email)]

This is covered by IETC Test:
http://samples.msdn.microsoft.com/ietestcenter/html5/dragdrop/types_attribute.htm

[Eric Seidel (no email)]

http://dev.w3.org/html5/spec/dnd.html#datatransfer

[Eric Seidel (no email)]

And http://www.whatwg.org/specs/web-apps/current-work/#the-datatransfer-interface

[Daniel Cheng]

Created attachment 126878 [details]
Patch

[WebKit Review Bot]

Attachment 126878 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/edit..." exit_code: 1

Source/WebCore/dom/Clipboard.h:36:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 26 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Philippe Normand]

Comment on attachment 126878 [details]
Patch

Attachment 126878 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11522007

[Early Warning System Bot]

Comment on attachment 126878 [details]
Patch

Attachment 126878 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11523020

[WebKit Review Bot]

Comment on attachment 126878 [details]
Patch

Attachment 126878 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11518075

New failing tests:
http/tests/security/clipboard/clipboard-file-access.html
http/tests/local/fileapi/send-dragged-file.html
http/tests/local/fileapi/file-last-modified.html
http/tests/local/fileapi/send-sliced-dragged-file.html

[Gyuyoung Kim]

Comment on attachment 126878 [details]
Patch

Attachment 126878 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11515140

[Daniel Cheng]

Created attachment 127014 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 127014 [details]
Patch

Attachment 127014 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11523349

[Daniel Cheng]

Created attachment 127027 [details]
Patch

Fixed compile on all platforms

[Eric Seidel (no email)]

Comment on attachment 127027 [details]
Patch

I would like to review this before you land.  When I scanned earlier, I had doubts.  I'll look shortly.

[Eric Seidel (no email)]

Comment on attachment 127027 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=127027&action=review

I strongly recommend reading http://www.webkit.org/coding/RefPtr.html if you haven't already.  I think Señor Levin might also want to take another look.

> Source/WebCore/dom/Clipboard.cpp:151
> +    return types()->contains(type); 

It's sad that we're creating a new object every time. :(  I guess we were doing that before, but it wasn't as obvious.

Also, this is no longer null-safe.  If platform Clipboard implementations ever return NULL this will crash. :(

> Source/WebCore/platform/chromium/ChromiumDataObject.cpp:115
> +        for (HashSet<String>::const_iterator it = hashedResults.begin();
> +             it != hashedResults.end();
> +             ++it) {

WebKit has no 80c rule.  Also, I'm not sure if we use { } for single line for statements.

> Source/WebCore/platform/chromium/ChromiumDataObject.cpp:118
> +        return results;

This should be results.release();

> Source/WebCore/platform/chromium/ClipboardChromium.cpp:306
>      return results;

results.release();

> Source/WebCore/platform/efl/ClipboardEfl.cpp:84
> +    return 0;

This will cause crashes based on other changes you made. :(

> Source/WebCore/platform/win/ClipboardWin.cpp:489
> +static void addMimeTypesForFormat(PassRefPtr<DOMStringList> results, const FORMATETC& format)

This should not be a PassRefPtr.  This function does not take ownership of the List.  RefPtr& or just a raw ptr is fine.

> LayoutTests/ChangeLog:8
> +        Update tests to use contains() instead of indexOf().

Lame that it doensn't have indexOf.  I wonder if this will break sites...

http://www.w3.org/TR/DOM-Level-3-Core/core.html#DOMStringList

> LayoutTests/ChangeLog:9
> +

I would like to see a test of clipboard.types == clipboard.types.  I suspect they're !=.  I suspect they were != before (since it looks like our code created a new array for each call, but it would be nice to confirm, and have an expectation for that behavior.  (It would also be nice to know what FF does.)

[Daniel Cheng]

Created attachment 127084 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 127084 [details]
Patch

Seems OK.  I would like to see you comment on the possible compatibility concern in the ChangeLog.  There is also now a risk that we could be adding types twice.  DOMStringList is ordered and will not remove duplicates like the HashSet would have.  Do you know if we care about the order at all?  Do we match our previous ordering behavior?  (I know these seem like small things, but it's best to think about them now, instead of when we have strange compatibility troubles.)

[Daniel Cheng]

(In reply to comment #16)
> (From update of attachment 127027 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=127027&action=review
> 
> I strongly recommend reading http://www.webkit.org/coding/RefPtr.html if you haven't already.  I think Señor Levin might also want to take another look.
> 

I've fixed all the RefPtr and null issues. Whether or not this is the way we should fix it remains TBD.

> > Source/WebCore/dom/Clipboard.cpp:151
> > +    return types()->contains(type); 
> 
> It's sad that we're creating a new object every time. :(  I guess we were doing that before, but it wasn't as obvious.
> 
> Also, this is no longer null-safe.  If platform Clipboard implementations ever return NULL this will crash. :(
> 

I've addressed this for now by changing all implementations to return a DOMStringList.

> > Source/WebCore/platform/chromium/ChromiumDataObject.cpp:115
> > +        for (HashSet<String>::const_iterator it = hashedResults.begin();
> > +             it != hashedResults.end();
> > +             ++it) {
> 
> WebKit has no 80c rule.  Also, I'm not sure if we use { } for single line for statements.
> 

I'm surprised the style checker didn't catch this. Fixed.

> > Source/WebCore/platform/chromium/ChromiumDataObject.cpp:118
> > +        return results;
> 
> This should be results.release();
> 
> > Source/WebCore/platform/chromium/ClipboardChromium.cpp:306
> >      return results;
> 
> results.release();
> 
> > Source/WebCore/platform/efl/ClipboardEfl.cpp:84
> > +    return 0;
> 
> This will cause crashes based on other changes you made. :(
> 
> > Source/WebCore/platform/win/ClipboardWin.cpp:489
> > +static void addMimeTypesForFormat(PassRefPtr<DOMStringList> results, const FORMATETC& format)
> 
> This should not be a PassRefPtr.  This function does not take ownership of the List.  RefPtr& or just a raw ptr is fine.
> 
> > LayoutTests/ChangeLog:8
> > +        Update tests to use contains() instead of indexOf().
> 
> Lame that it doensn't have indexOf.  I wonder if this will break sites...
> 

FF doesn't support indexOf, so websites that want to work cross-browser /should/ be using if (types.indexOf) { } else if (types.contains) { }.

> http://www.w3.org/TR/DOM-Level-3-Core/core.html#DOMStringList
> 
> > LayoutTests/ChangeLog:9
> > +
> 
> I would like to see a test of clipboard.types == clipboard.types.  I suspect they're !=.  I suspect they were != before (since it looks like our code created a new array for each call, but it would be nice to confirm, and have an expectation for that behavior.  (It would also be nice to know what FF does.)

Hmm. From the spec:
The types attribute must return a live DOMStringList giving the strings that the following steps would produce. The same object must be returned each time.

Firefox currently returns a different object each time.

Fixing this is probably a separate patch, but I don't see a way to do it without coming into conflict with layering issues (to be fair, Clipboard and its various implementations are already a pretty big mess in this regard). If we return a HashSet<String>, it'd be impossible to make it live since the Clipboard object won't know how to tell the glue object to update its view. Note that the "items" and "files" attributes currently suffer from this problem as well, since they aren't live and always return a copy.

In order to implement a live view like some other things like document.getElementsByname, I think we'd have to create TypesDOMStringList, keep a cached pointer to it in Clipboard once it's created, and make sure all Clipboard implementations update it as appropriate.

[Daniel Cheng]

Comment on attachment 127084 [details]
Patch

(In reply to comment #18)
> (From update of attachment 127084 [details])
> Seems OK.  I would like to see you comment on the possible compatibility concern in the ChangeLog.  There is also now a risk that we could be adding types twice.  DOMStringList is ordered and will not remove duplicates like the HashSet would have.  Do you know if we care about the order at all?  Do we match our previous ordering behavior?  (I know these seem like small things, but it's best to think about them now, instead of when we have strange compatibility troubles.)

I'll update the ChangeLogs with my findings about Firefox (IE, to my knowledge, did not previously implement the types attribute at all).

Duplicates: That reminds me--I should add a new layout test for this. Previously, we didn't conform completely to the spec--it actually allows for "Files" to show up twice in the types attribute if there are files in the drag as well as data associated with the type "Files".

Order: Unimportant, since we used to use a HashSet, which gave no ordering guarantees as far as I know.

[WebKit Review Bot]

Attachment 127014 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/edit..." exit_code: 1

Source/WebCore/dom/Clipboard.h:36:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 127027 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/edit..." exit_code: 1

Source/WebCore/dom/Clipboard.h:36:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 30 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 127084 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/edit..." exit_code: 1

Source/WebCore/dom/Clipboard.h:36:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 30 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Daniel Cheng]

Created attachment 127287 [details]
Patch

[Daniel Cheng]

Upon further inspection, I realized that "Files" will never appear twice anyway, since we always lowercase the format name in setData().

I still added a test to document the fact that we don't conform to the spec for returning the same, live object each time for some datatransfer attributes though.

[Eric Seidel (no email)]

Comment on attachment 127287 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=127287&action=review

LGTM.  Thanks.

> Source/WebCore/platform/chromium/ChromiumDataObject.cpp:113
> +        for (HashSet<String>::const_iterator it = hashedResults.begin(); it != hashedResults.end(); ++it)
> +          results->append(*it);

I think it would be totally OK to add a DOMStringList::fromHashSet function which did this.  But this is also OK.

[Daniel Cheng]

Committed r107894: <http://trac.webkit.org/changeset/107894>

[Erik Arvidsson]

This is a serious step backwards. Before this change things like indexOf, map, forEach all worked.

DOMStringList is deprecated and should not be used in new APIs

[Daniel Cheng]

If we intend to roll this change back, we should do it sooner rather than later. As far as I'm aware, WebKit was the only browser that returned an Array rather than a DOMStringList. Once this change goes out to stable versions of Safari and Chrome, pages will stop using the compatibility code they used to need.

[Eric Seidel (no email)]

I'm all for avoiding DOMStringList, it's ugly.  But as Daniel points out, we were the odd-man out.  I'm happy to revert this if you plan to lead the evangelizing charge arv?

[Erik Arvidsson]

I think we should just add contains to Array.prototype and then we will be compatible with Firefox.

I did start the evangelizing. DOM4 now have a warning that DOMStringList should not be used for new APIs and the discussion about adding Array.prototype.contains to ES6 has started.

Cameron also suggested adding [ArrayClass] to DOMTokenList to make existing cases work.

[Daniel Cheng]

Since event.dataTransfer.types is not a legacy API and DOMStringList is deprecated, I think the best course of action would be to evangelize changing the property type in the spec to Array rather than add contains to Array.prototype.

[Daniel Cheng]

So it looks like there's a good chance we'll be rolling this change back...
http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2012-February/034954.html

[Ian 'Hixie' Hickson]

As far as the spec goes, do whichever you think makes sense (sounds like returning an Array is it) and then mention on the HTML spec bug that you've done that, and I'll make the spec match the implementations.

[Daniel Cheng]

Reverted r107894 for reason:

dataTransfer.types should be an Array since DOMStringList is deprecated.

Committed r109182: <http://trac.webkit.org/changeset/109182>

[Alexey Proskuryakov]

*** Bug 108767 has been marked as a duplicate of this bug. ***

[Domenic Denicola]

FWIW years later we're trying to align all browsers on this using the new FrozenArray construct. I filed https://bugs.webkit.org/show_bug.cgi?id=162932 to track that.