Bug 33607 – REGRESSION (r49268): DHTML drag not allowed unless event.dataTransfer.effectAllowed is set (differs from HTML5)

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Brian Weinstein]

Created attachment 46473 [details]
Doesn't Work

REGRESSION (r49268): DHTML drag not allowed unless event.dataTransfer.effectAllowed is set (differs from HTML5).

[Brian Weinstein]

Created attachment 46474 [details]
Works

[Brian Weinstein]

<rdar://7507114>

[Brian Weinstein]

Created attachment 46484 [details]
WIP PATCH

The additional test output with this change to the test is: 

 16 When effectAllowed == "undefined"
 17 
 18 PASS event.dataTransfer.dropEffect is "none"
 19 PASS event.dataTransfer.dropEffect is "none"
 20 PASS event.dataTransfer.dropEffect is "none"
 21 PASS event.dataTransfer.dropEffect is "none"
 22 PASS event.dataTransfer.dropEffect is "none"
 23 

This patch fixes the test case that is attached here, but doesn't seem to be working correctly in DRT.

[Brian Weinstein]

Created attachment 46485 [details]
WIP PATCH 2

This gives these additional results:

 16 When effectAllowed == "undefined"
 17 
 18 PASS event.dataTransfer.dropEffect is "none"
 19 FAIL event.dataTransfer.dropEffect should be none. Was copy.
 20 FAIL event.dataTransfer.dropEffect should be none. Was move.
 21 FAIL event.dataTransfer.dropEffect should be none. Was link.
 22 PASS event.dataTransfer.dropEffect is "none"
 23 

This is much closer.

[Brian Weinstein]

Created attachment 46494 [details]
WIP PATCH 3 - No ChangeLogs - But Code Finished

[Brian Weinstein]

Created attachment 46497 [details]
[PATCH] Fix

[Adam Roben (:aroben)]

Comment on attachment 46497 [details]
[PATCH] Fix

>  bool Clipboard::sourceOperation(DragOperation& op) const
>  {
> -    if (m_effectAllowed.isNull())
> -        return false;
> -    op = dragOpFromIEOp(m_effectAllowed);
> +    op = dragOpFromIEOp(!m_effectAllowed.isNull() ? m_effectAllowed : String("uninitialized"));
>      return true;
>  }

Why not initialize m_effectAllowed to "uninitialized" in the Clipboard constructor? Is there other code that relies on m_effectAllowed being null?

> @@ -1,3 +1,17 @@
> [phone]  Brian Weinstein  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        REGRESSION (r49268): DHTML drag not allowed unless event.dataTransfer.effectAllowed 
> +        is set (differs from HTML5).
> +        Fixes <https://bugs.webkit.org/show_bug.cgi?id=33607> and <rdar://7507114>.
> +        
> +        Updated the drag and drop test to test if effectAllowed isn't set, in addition
> +        to its other tets.

Typo: tets

> @@ -99,11 +101,11 @@
>          // Extracted from the HTML 5 drag-and-drop section, http://dev.w3.org/html5/spec/Overview.html#dnd
>          if (chosenDropEffect == "none")
>              return true;
> -        if (chosenDropEffect == "copy" && ["copy", "copyLink", "copyMove", "uninitialized", "all"].indexOf(allowedDropEffect) != -1)
> +        if (chosenDropEffect == "copy" && ["copy", "copyLink", "copyMove", "uninitialized", "all", "undefined"].indexOf(allowedDropEffect) != -1)
>              return true;
> -        if (chosenDropEffect == "move" && ["move", "copyMove", "linkMove", "uninitialized", "all"].indexOf(allowedDropEffect) != -1)
> +        if (chosenDropEffect == "move" && ["move", "copyMove", "linkMove", "uninitialized", "all", "undefined"].indexOf(allowedDropEffect) != -1)
>              return true;
> -        if (chosenDropEffect == "link" && ["link", "copyLink", "linkMove", "uninitialized", "all"].indexOf(allowedDropEffect) != -1)
> +        if (chosenDropEffect == "link" && ["link", "copyLink", "linkMove", "uninitialized", "all", "undefined"].indexOf(allowedDropEffect) != -1)
>              return true;
>          return false;
>      }

I think it's a little confusing to make this change. Before your patch, isDropEffectAllowed matched a part of the HTML5 spec pretty closely. But now this function contains this new "undefined" string, which isn't mentioned in HTML5 at all.

Maybe it would be better to have an explicit test somewhere that when the "undefined" effect was chosen, event.dataTransfer.effectAllowed continues to be "uninitialized"?

[Brian Weinstein]

Created attachment 46500 [details]
[PATCH] Simpler Fix with Adam's Comments

[Adam Roben (:aroben)]

Comment on attachment 46500 [details]
[PATCH] Simpler Fix with Adam's Comments

>  Clipboard::Clipboard(ClipboardAccessPolicy policy, bool isForDragging) 
>      : m_policy(policy) 
> +    , m_effectAllowed("uninitialized")

At some point we should switch m_effectAllowed to be an AtomicString. Maybe when we fix setEffectAllowed to only allow setting it to certain values?

r=me

[Brian Weinstein]

Committed in r53203.