Bug 76925 – dropzone does not normalize type strings

• Component: HTML Editing
• Status: RESOLVED FIXED

[Daniel Cheng]

Per http://www.whatwg.org/specs/web-apps/current-work/multipage/dnd.html#the-dropzone-attribute, the types are supposed to be normalized by lowercasing (with NO special conversion from text->text/plain and url->text/uri-list, but since dropzone checks against the types() collection directly, the type normalization that ports generally do in getData()/setData(), etc is ignored.

[Daniel Cheng]

Created attachment 123775 [details]
Patch

[Tony Chang]

Comment on attachment 123775 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        Covered by existing tests.

I would say that you changed/added a test case in fast/events/dropzone-002.html.

> LayoutTests/ChangeLog:10
> +        * fast/events/dropzone-002.html: Update layout test to check case normalization as well.
> +        Also fixes the test to use text/uri-list, since no URL->text/uri-list special casing is
> +        defined for new drag and drop APIs.

Does no special casing of URL->text/uri-list defined mean that string:url should fail?  It seems bad to lose test coverage for that case: we should test both string:url and string:text/uri-list.

[Daniel Cheng]

Created attachment 123785 [details]
Patch

[Daniel Cheng]

Comment on attachment 123775 [details]
Patch

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

>> Source/WebCore/ChangeLog:11
>> +        Covered by existing tests.
> 
> I would say that you changed/added a test case in fast/events/dropzone-002.html.

Done.

>> LayoutTests/ChangeLog:10
>> +        defined for new drag and drop APIs.
> 
> Does no special casing of URL->text/uri-list defined mean that string:url should fail?  It seems bad to lose test coverage for that case: we should test both string:url and string:text/uri-list.

I'm not sure how I should check this. I could add a new dropzone test and set the expectation that no drop should happen? I looked around the existing dropzone tests and it doesn't seem like there's a 'negative' dropzone test that checks for things that shouldn't match. Perhaps I could add one in a separate patch, as that's not directly related to fixing the normalization of type strings.

[Tony Chang]

(In reply to comment #4)
> > Does no special casing of URL->text/uri-list defined mean that string:url should fail?  It seems bad to lose test coverage for that case: we should test both string:url and string:text/uri-list.
> 
> I'm not sure how I should check this. I could add a new dropzone test and set the expectation that no drop should happen? I looked around the existing dropzone tests and it doesn't seem like there's a 'negative' dropzone test that checks for things that shouldn't match. Perhaps I could add one in a separate patch, as that's not directly related to fixing the normalization of type strings.

I actually think it's OK for URL->text/uri-list to happen, although we should ask for the spec to be clear about this.

My main point was that we should not remove the test case that covers this behavior regardless of what the result is.  Layout tests are primarily for regression coverage.

[Daniel Cheng]

Created attachment 123790 [details]
Patch

[Daniel Cheng]

Comment on attachment 123790 [details]
Patch

Clearing flags on attachment: 123790

Committed r105800: <http://trac.webkit.org/changeset/105800>

[Daniel Cheng]

All reviewed patches have been landed.  Closing bug.

[Yael]

Comment on attachment 123790 [details]
Patch

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

> Source/WebCore/dom/Clipboard.cpp:217
> -        return hasFileOfType(keyword.substring(5));
> -                             
> +        return hasFileOfType(keyword.substring(5).lower());
> +
>      if (keyword.startsWith("string:"))
> -        return hasStringOfType(keyword.substring(7));
> +        return hasStringOfType(keyword.substring(7).lower());

The dropZone attribute is converted to lowercase in findDropZone(), before this is called.
Why was this change needed?

[Daniel Cheng]

(In reply to comment #9)
> (From update of attachment 123790 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=123790&action=review
> 
> > Source/WebCore/dom/Clipboard.cpp:217
> > -        return hasFileOfType(keyword.substring(5));
> > -                             
> > +        return hasFileOfType(keyword.substring(5).lower());
> > +
> >      if (keyword.startsWith("string:"))
> > -        return hasStringOfType(keyword.substring(7));
> > +        return hasStringOfType(keyword.substring(7).lower());
> 
> The dropZone attribute is converted to lowercase in findDropZone(), before this is called.
> Why was this change needed?

The original intent was to address url->text/uri-list normalization as well but that's been deferred to a separate patch.

The layout test change is still valuable; I should revert the code change though. Sorry about missing that.