Bug 30416 - dataTransfer.types (HTML5 drag & drop) should return DOMStringList
Summary: dataTransfer.types (HTML5 drag & drop) should return DOMStringList
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh Intel OS X 10.5
: P2 Normal
Assignee: Daniel Cheng
URL:
Keywords:
: 55837 73968 108767 (view as bug list)
Depends on: 39429
Blocks: 76198
  Show dependency treegraph
 
Reported: 2009-10-15 16:37 PDT by webkit
Modified: 2016-10-04 13:25 PDT (History)
17 users (show)

See Also:


Attachments
Patch (31.11 KB, patch)
2012-02-13 17:55 PST, Daniel Cheng
no flags Details | Formatted Diff | Diff
Patch (33.81 KB, patch)
2012-02-14 12:18 PST, Daniel Cheng
no flags Details | Formatted Diff | Diff
Patch (36.17 KB, patch)
2012-02-14 13:19 PST, Daniel Cheng
no flags Details | Formatted Diff | Diff
Patch (37.76 KB, patch)
2012-02-14 17:08 PST, Daniel Cheng
no flags Details | Formatted Diff | Diff
Patch (41.79 KB, patch)
2012-02-15 18:08 PST, Daniel Cheng
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description webkit 2009-10-15 16:37:53 PDT
Currently returns flat array. Firefox nightlies implement this correctly.
Comment 1 Alexey Proskuryakov 2011-03-05 23:25:56 PST
*** Bug 55837 has been marked as a duplicate of this bug. ***
Comment 2 Alexey Proskuryakov 2011-12-07 14:26:09 PST
*** Bug 73968 has been marked as a duplicate of this bug. ***
Comment 3 Eric Seidel (no email) 2012-01-24 03:02:02 PST
This is covered by IETC Test:
http://samples.msdn.microsoft.com/ietestcenter/html5/dragdrop/types_attribute.htm
Comment 4 Eric Seidel (no email) 2012-01-24 03:02:16 PST
http://dev.w3.org/html5/spec/dnd.html#datatransfer
Comment 6 Daniel Cheng 2012-02-13 17:55:06 PST
Created attachment 126878 [details]
Patch
Comment 7 WebKit Review Bot 2012-02-13 17:58:46 PST
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.
Comment 8 Philippe Normand 2012-02-13 18:10:57 PST
Comment on attachment 126878 [details]
Patch

Attachment 126878 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11522007
Comment 9 Early Warning System Bot 2012-02-13 19:10:56 PST
Comment on attachment 126878 [details]
Patch

Attachment 126878 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11523020
Comment 10 WebKit Review Bot 2012-02-13 19:39:27 PST
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
Comment 11 Gyuyoung Kim 2012-02-13 22:46:31 PST
Comment on attachment 126878 [details]
Patch

Attachment 126878 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11515140
Comment 12 Daniel Cheng 2012-02-14 12:18:36 PST
Created attachment 127014 [details]
Patch
Comment 13 Gyuyoung Kim 2012-02-14 13:13:04 PST
Comment on attachment 127014 [details]
Patch

Attachment 127014 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11523349
Comment 14 Daniel Cheng 2012-02-14 13:19:39 PST
Created attachment 127027 [details]
Patch

Fixed compile on all platforms
Comment 15 Eric Seidel (no email) 2012-02-14 16:00:10 PST
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.
Comment 16 Eric Seidel (no email) 2012-02-14 16:34:39 PST
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.)
Comment 17 Daniel Cheng 2012-02-14 17:08:34 PST
Created attachment 127084 [details]
Patch
Comment 18 Eric Seidel (no email) 2012-02-14 17:35:19 PST
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.)
Comment 19 Daniel Cheng 2012-02-14 17:37:18 PST
(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.
Comment 20 Daniel Cheng 2012-02-14 17:41:00 PST
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.
Comment 21 WebKit Review Bot 2012-02-15 01:53:01 PST
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.
Comment 22 WebKit Review Bot 2012-02-15 02:09:59 PST
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.
Comment 23 WebKit Review Bot 2012-02-15 03:08:56 PST
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.
Comment 24 Daniel Cheng 2012-02-15 18:08:18 PST
Created attachment 127287 [details]
Patch
Comment 25 Daniel Cheng 2012-02-15 18:10:38 PST
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.
Comment 26 Eric Seidel (no email) 2012-02-15 20:31:08 PST
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.
Comment 27 Daniel Cheng 2012-02-15 23:26:31 PST
Committed r107894: <http://trac.webkit.org/changeset/107894>
Comment 28 Erik Arvidsson 2012-02-23 12:25:22 PST
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
Comment 29 Daniel Cheng 2012-02-23 12:58:14 PST
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.
Comment 30 Eric Seidel (no email) 2012-02-23 13:59:24 PST
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?
Comment 31 Erik Arvidsson 2012-02-23 14:49:05 PST
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.
Comment 32 Daniel Cheng 2012-02-23 15:03:22 PST
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.
Comment 33 Daniel Cheng 2012-02-24 11:56:32 PST
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
Comment 34 Ian 'Hixie' Hickson 2012-02-24 15:35:53 PST
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.
Comment 35 Daniel Cheng 2012-02-28 18:20:32 PST
Reverted r107894 for reason:

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

Committed r109182: <http://trac.webkit.org/changeset/109182>
Comment 36 Alexey Proskuryakov 2013-02-04 12:54:15 PST
*** Bug 108767 has been marked as a duplicate of this bug. ***
Comment 37 Domenic Denicola 2016-10-04 13:25:05 PDT
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.