Fix HTML5 DnD regression for FF (#9245)

- Always set some drag data
- Set the dropEffect on dragEnter and dragOver events on drop target
- Send the dropEffect to server on drop event with disclaimer of current support
- Remove _dragOverCriteria_ and use _dropCriteria_ for `dragenter`, `dragover` and `drop` criteria

Tested manually basic DnD and Grid DnD on Mac with Chrome, Firefox, Safari.
Safari is still missing drag image (regression).

Tested manually basic DnD and Grid Dnd on Windows IE11 and Edge.
Drop event for both is still not working properly #9174.

3 files changed, 115 insertions, 76 deletions

diff --git a/client/src/main/java/com/vaadin/client/connectors/grid/GridDropTargetConnector.java b/client/src/main/java/com/vaadin/client/connectors/grid/GridDropTargetConnector.java
index 09142e222b..8126f85c1e 100644
--- a/client/src/main/java/com/vaadin/client/connectors/grid/GridDropTargetConnector.java
+++ b/client/src/main/java/com/vaadin/client/connectors/grid/GridDropTargetConnector.java
@@ -82,7 +82,7 @@ public class GridDropTargetConnector extends DropTargetExtensionConnector {
 
     @Override
     protected void sendDropEventToServer(String dataTransferText,
-            Event dropEvent) {
+            String dropEffect, Event dropEvent) {
 
         String rowKey = null;
         DropLocation dropLocation = null;
@@ -96,8 +96,8 @@ public class GridDropTargetConnector extends DropTargetExtensionConnector {
                     (NativeEvent) dropEvent);
         }
 
-        getRpcProxy(GridDropTargetRpc.class).drop(dataTransferText, rowKey,
-                dropLocation);
+        getRpcProxy(GridDropTargetRpc.class).drop(dataTransferText, dropEffect,
+                rowKey, dropLocation);
     }
 
     private JsonObject getRowData(TableRowElement row) {
@@ -147,7 +147,7 @@ public class GridDropTargetConnector extends DropTargetExtensionConnector {
     }
 
     @Override
-    protected void setTargetIndicator(Event event) {
+    protected void setTargetClassIndicator(Event event) {
         getTargetRow(((Element) event.getTarget())).ifPresent(target -> {
 
             // Get required class name
@@ -184,7 +184,7 @@ public class GridDropTargetConnector extends DropTargetExtensionConnector {
     }
 
     @Override
-    protected void removeTargetIndicator(Event event) {
+    protected void removeTargetClassIndicator(Event event) {
 
         // Remove all possible style names
         getTargetRow((Element) event.getTarget()).ifPresent(e -> {
diff --git a/client/src/main/java/com/vaadin/client/extensions/DragSourceExtensionConnector.java b/client/src/main/java/com/vaadin/client/extensions/DragSourceExtensionConnector.java
index 165e0ca9df..932128a8b3 100644
--- a/client/src/main/java/com/vaadin/client/extensions/DragSourceExtensionConnector.java
+++ b/client/src/main/java/com/vaadin/client/extensions/DragSourceExtensionConnector.java
@@ -77,7 +77,7 @@ public class DragSourceExtensionConnector extends AbstractExtensionConnector {
      * Sets the given element draggable and adds class name.
      *
      * @param element
-     *         Element to be set draggable.
+     *            Element to be set draggable.
      */
     protected void setDraggable(Element element) {
         element.setDraggable(Element.DRAGGABLE_TRUE);
@@ -91,7 +91,7 @@ public class DragSourceExtensionConnector extends AbstractExtensionConnector {
      * Removes draggable and class name from the given element.
      *
      * @param element
-     *         Element to remove draggable from.
+     *            Element to remove draggable from.
      */
     protected void removeDraggable(Element element) {
         element.setDraggable(Element.DRAGGABLE_FALSE);
@@ -104,7 +104,7 @@ public class DragSourceExtensionConnector extends AbstractExtensionConnector {
      * Adds dragstart and dragend event listeners to the given DOM element.
      *
      * @param element
-     *         DOM element to attach event listeners to.
+     *            DOM element to attach event listeners to.
      */
     protected void addDragListeners(Element element) {
         EventTarget target = element.cast();
@@ -117,7 +117,7 @@ public class DragSourceExtensionConnector extends AbstractExtensionConnector {
      * Removes dragstart and dragend event listeners from the given DOM element.
      *
      * @param element
-     *         DOM element to remove event listeners from.
+     *            DOM element to remove event listeners from.
      */
     protected void removeDragListeners(Element element) {
         EventTarget target = element.cast();
@@ -150,7 +150,7 @@ public class DragSourceExtensionConnector extends AbstractExtensionConnector {
      * dragstart} event occurs.
      *
      * @param event
-     *         browser event to be handled
+     *            browser event to be handled
      */
     protected void onDragStart(Event event) {
         // Convert elemental event to have access to dataTransfer
@@ -167,10 +167,12 @@ public class DragSourceExtensionConnector extends AbstractExtensionConnector {
 
         // Set text data parameter
         String dataTransferText = createDataTransferText(event);
-        if (dataTransferText != null && !dataTransferText.isEmpty()) {
-            nativeEvent.getDataTransfer()
-                    .setData(DragSourceState.DATA_TYPE_TEXT, dataTransferText);
+        // Always set something as the text data, or DnD won't work in FF !
+        if (dataTransferText == null) {
+            dataTransferText = "";
         }
+        nativeEvent.getDataTransfer().setData(DragSourceState.DATA_TYPE_TEXT,
+                dataTransferText);
 
         // Initiate firing server side dragstart event when there is a
         // DragStartListener attached on the server side
@@ -187,7 +189,7 @@ public class DragSourceExtensionConnector extends AbstractExtensionConnector {
      * of the given event.
      *
      * @param dragStartEvent
-     *         Event to set the data for.
+     *            Event to set the data for.
      * @return Textual data to be set for the event or {@literal null}.
      */
     protected String createDataTransferText(Event dragStartEvent) {
@@ -201,7 +203,7 @@ public class DragSourceExtensionConnector extends AbstractExtensionConnector {
      * handler attached.
      *
      * @param dragStartEvent
-     *         Client side dragstart event.
+     *            Client side dragstart event.
      */
     protected void sendDragStartEventToServer(Event dragStartEvent) {
         getRpcProxy(DragSourceRpc.class).dragStart();
@@ -211,7 +213,7 @@ public class DragSourceExtensionConnector extends AbstractExtensionConnector {
      * Sets the drag image to be displayed.
      *
      * @param dragStartEvent
-     *         The drag start event.
+     *            The drag start event.
      */
     protected void setDragImage(Event dragStartEvent) {
         String imageUrl = getResourceUrl(DragSourceState.RESOURCE_DRAG_IMAGE);
@@ -228,7 +230,7 @@ public class DragSourceExtensionConnector extends AbstractExtensionConnector {
      * event occurs.
      *
      * @param event
-     *         browser event to be handled
+     *            browser event to be handled
      */
     protected void onDragEnd(Event event) {
         // Initiate server start dragend event when there is a DragEndListener
@@ -248,9 +250,9 @@ public class DragSourceExtensionConnector extends AbstractExtensionConnector {
      * Initiates a server RPC for the drag end event.
      *
      * @param dragEndEvent
-     *         Client side dragend event.
+     *            Client side dragend event.
      * @param dropEffect
-     *         Drop effect of the dragend event, extracted from {@code
+     *            Drop effect of the dragend event, extracted from {@code
      *         DataTransfer.dropEffect} parameter.
      */
     protected void sendDragEndEventToServer(Event dragEndEvent,
@@ -269,11 +271,13 @@ public class DragSourceExtensionConnector extends AbstractExtensionConnector {
     }
 
     private native void setEffectAllowed(DataTransfer dataTransfer,
-            String effectAllowed)/*-{
+            String effectAllowed)
+    /*-{
         dataTransfer.effectAllowed = effectAllowed;
     }-*/;
 
-    private native String getDropEffect(DataTransfer dataTransfer)/*-{
+    static native String getDropEffect(DataTransfer dataTransfer)
+    /*-{
         return dataTransfer.dropEffect;
     }-*/;
 
@@ -282,7 +286,8 @@ public class DragSourceExtensionConnector extends AbstractExtensionConnector {
         return (DragSourceState) super.getState();
     }
 
-    private native boolean getStylePrimaryName(Element element)/*-{
+    private native boolean getStylePrimaryName(Element element)
+    /*-{
         return @com.google.gwt.user.client.ui.UIObject::getStylePrimaryName(Lcom/google/gwt/dom/client/Element;)(element);
     }-*/;
 }
diff --git a/client/src/main/java/com/vaadin/client/extensions/DropTargetExtensionConnector.java b/client/src/main/java/com/vaadin/client/extensions/DropTargetExtensionConnector.java
index e3fbeabab4..7c9e9755b8 100644
--- a/client/src/main/java/com/vaadin/client/extensions/DropTargetExtensionConnector.java
+++ b/client/src/main/java/com/vaadin/client/extensions/DropTargetExtensionConnector.java
@@ -25,6 +25,7 @@ import com.vaadin.client.ServerConnector;
 import com.vaadin.event.dnd.DropTargetExtension;
 import com.vaadin.shared.ui.Connect;
 import com.vaadin.shared.ui.dnd.DragSourceState;
+import com.vaadin.shared.ui.dnd.DropEffect;
 import com.vaadin.shared.ui.dnd.DropTargetRpc;
 import com.vaadin.shared.ui.dnd.DropTargetState;
 
@@ -92,7 +93,7 @@ public class DropTargetExtensionConnector extends AbstractExtensionConnector {
      * DOM element.
      *
      * @param element
-     *         DOM element to attach event listeners to.
+     *            DOM element to attach event listeners to.
      */
     protected void addDropListeners(Element element) {
         EventTarget target = element.cast();
@@ -108,7 +109,7 @@ public class DropTargetExtensionConnector extends AbstractExtensionConnector {
      * given DOM element.
      *
      * @param element
-     *         DOM element to remove event listeners from.
+     *            DOM element to remove event listeners from.
      */
     protected void removeDropListeners(Element element) {
         EventTarget target = element.cast();
@@ -140,34 +141,68 @@ public class DropTargetExtensionConnector extends AbstractExtensionConnector {
      * Event handler for the {@code dragenter} event.
      *
      * @param event
-     *         browser event to be handled
+     *            browser event to be handled
      */
     protected void onDragEnter(Event event) {
-        // Generate style name for drop target
-        styleDragCenter = dropTargetWidget.getStylePrimaryName()
-                + STYLE_SUFFIX_DRAG_CENTER;
+        NativeEvent nativeEvent = (NativeEvent) event;
+        if (isDropAllowed(nativeEvent)) {
+            // Generate style name for drop target
+            styleDragCenter = dropTargetWidget.getStylePrimaryName()
+                    + STYLE_SUFFIX_DRAG_CENTER;
+
+            setTargetClassIndicator(event);
 
-        setTargetIndicator(event);
+            setDropEffect(nativeEvent);
+
+            // According to spec, need to call this for allowing dropping, the
+            // default action would be to reject as target
+            event.preventDefault();
+        } else {
+            // Remove drop effect
+            nativeEvent.getDataTransfer()
+                    .setDropEffect(DataTransfer.DropEffect.NONE);
+        }
+    }
+
+    /**
+     * Set the drop effect for the dragenter / dragover event, if one has been
+     * set from server side.
+     * <p>
+     * From Moz Foundation: "You can modify the dropEffect property during the
+     * dragenter or dragover events, if for example, a particular drop target
+     * only supports certain operations. You can modify the dropEffect property
+     * to override the user effect, and enforce a specific drop operation to
+     * occur. Note that this effect must be one listed within the effectAllowed
+     * property. Otherwise, it will be set to an alternate value that is
+     * allowed."
+     *
+     * @param event
+     *            the dragenter or dragover event.
+     */
+    protected void setDropEffect(NativeEvent event) {
+        if (getState().dropEffect != null) {
+
+            DataTransfer.DropEffect dropEffect = DataTransfer.DropEffect
+                    // the valueOf() needs to have equal string and name()
+                    // doesn't return in all upper case
+                    .valueOf(getState().dropEffect.name().toUpperCase());
+            event.getDataTransfer().setDropEffect(dropEffect);
+        }
     }
 
     /**
      * Event handler for the {@code dragover} event.
      *
      * @param event
-     *         browser event to be handled
+     *            browser event to be handled
      */
     protected void onDragOver(Event event) {
         NativeEvent nativeEvent = (NativeEvent) event;
-        if (isDragOverAllowed(nativeEvent)) {
-            // Set dropEffect parameter
-            if (getState().dropEffect != null) {
-                nativeEvent.getDataTransfer().setDropEffect(
-                        DataTransfer.DropEffect
-                                .valueOf(getState().dropEffect.name()));
-            }
+        if (isDropAllowed(nativeEvent)) {
+            setDropEffect(nativeEvent);
 
             // Add drop target indicator in case the element doesn't have one
-            setTargetIndicator(event);
+            setTargetClassIndicator(event);
 
             // Prevent default to allow drop
             nativeEvent.preventDefault();
@@ -178,60 +213,55 @@ public class DropTargetExtensionConnector extends AbstractExtensionConnector {
                     .setDropEffect(DataTransfer.DropEffect.NONE);
 
             // Remove drop target indicator
-            removeTargetIndicator(event);
-        }
-    }
-
-    /**
-     * Determines if dragover event is allowed on this drop target according to
-     * the dragover criteria.
-     *
-     * @param event
-     *         Native dragover event.
-     * @return {@code true} if dragover is allowed, {@code false} otherwise.
-     * @see DropTargetExtension#setDragOverCriteria(String)
-     */
-    protected boolean isDragOverAllowed(NativeEvent event) {
-        if (getState().dragOverCriteria != null) {
-            return executeScript(event, getState().dragOverCriteria);
+            removeTargetClassIndicator(event);
         }
-
-        // Allow when criteria not set
-        return true;
     }
 
     /**
      * Event handler for the {@code dragleave} event.
      *
      * @param event
-     *         browser event to be handled
+     *            browser event to be handled
      */
     protected void onDragLeave(Event event) {
-        removeTargetIndicator(event);
+        removeTargetClassIndicator(event);
     }
 
     /**
      * Event handler for the {@code drop} event.
      *
      * @param event
-     *         browser event to be handled
+     *            browser event to be handled
      */
     protected void onDrop(Event event) {
         NativeEvent nativeEvent = (NativeEvent) event;
-        if (dropAllowed(nativeEvent)) {
+        if (isDropAllowed(nativeEvent)) {
             nativeEvent.preventDefault();
             nativeEvent.stopPropagation();
 
-            String dataTransferText = nativeEvent.getDataTransfer().getData(
-                    DragSourceState.DATA_TYPE_TEXT);
+            String dataTransferText = nativeEvent.getDataTransfer()
+                    .getData(DragSourceState.DATA_TYPE_TEXT);
 
-            sendDropEventToServer(dataTransferText, event);
+            String dropEffect = DragSourceExtensionConnector
+                    .getDropEffect(nativeEvent.getDataTransfer());
+
+            sendDropEventToServer(dataTransferText, dropEffect, event);
         }
 
-        removeTargetIndicator(event);
+        removeTargetClassIndicator(event);
     }
 
-    private boolean dropAllowed(NativeEvent event) {
+    private boolean isDropAllowed(NativeEvent event) {
+        // there never should be a drop when effect has been set to none
+        if (getState().dropEffect != null
+                && getState().dropEffect == DropEffect.NONE) {
+            return false;
+        }
+        // TODO #9246: Should add verification for checking effectAllowed and
+        // dropEffect from event and comparing that to target's dropEffect.
+        // Currently Safari, Edge and IE don't follow the spec by allowing drop
+        // if those don't match
+
         if (getState().dropCriteria != null) {
             return executeScript(event, getState().dropCriteria);
         }
@@ -244,23 +274,26 @@ public class DropTargetExtensionConnector extends AbstractExtensionConnector {
      * Initiates a server RPC for the drop event.
      *
      * @param dataTransferText
-     *         Client side textual data that can be set for the drag source and
-     *         is transferred to the drop target.
+     *            Client side textual data that can be set for the drag source
+     *            and is transferred to the drop target.
+     * @param dropEffect
+     *            the desired drop effect
      * @param dropEvent
-     *         Client side drop event.
+     *            Client side drop event.
      */
     protected void sendDropEventToServer(String dataTransferText,
-            Event dropEvent) {
-        getRpcProxy(DropTargetRpc.class).drop(dataTransferText);
+            String dropEffect, Event dropEvent) {
+        getRpcProxy(DropTargetRpc.class).drop(dataTransferText, dropEffect);
     }
 
     /**
      * Add class that indicates that the component is a target.
      *
      * @param event
-     *         The drag enter or dragover event that triggered the indication.
+     *            The drag enter or dragover event that triggered the
+     *            indication.
      */
-    protected void setTargetIndicator(Event event) {
+    protected void setTargetClassIndicator(Event event) {
         getDropTargetElement().addClassName(styleDragCenter);
     }
 
@@ -270,13 +303,14 @@ public class DropTargetExtensionConnector extends AbstractExtensionConnector {
      * This is triggered on dragleave, drop and dragover events.
      *
      * @param event
-     *         the event that triggered the removal of the indicator
+     *            the event that triggered the removal of the indicator
      */
-    protected void removeTargetIndicator(Event event) {
+    protected void removeTargetClassIndicator(Event event) {
         getDropTargetElement().removeClassName(styleDragCenter);
     }
 
-    private native boolean executeScript(NativeEvent event, String script)/*-{
+    private native boolean executeScript(NativeEvent event, String script)
+    /*-{
         return new Function('event', script)(event);
     }-*/;