Fix NPE when detaching DragSource & DropTarget on client side (#9341)

When the component the extensions have been attached to are removed, there was an NPE due to getParent() not being available anymore. Fixed by not doing clean up on those cases as it is not necessary.

Fixes #9101

* Add back missing detach call

client/src/main/java/com/vaadin/client/connectors/grid/GridDragSourceConnector.java

@@ -352,8 +352,6 @@ public class GridDragSourceConnector extends DragSourceExtensionConnector {
 
     @Override
     public void onUnregister() {
-        super.onUnregister();
-
         // Remove draggable from all row elements in the escalator
         Range visibleRange = getEscalator().getVisibleRowRange();
         for (int i = visibleRange.getStart(); i < visibleRange.getEnd(); i++) {
@@ -371,6 +369,8 @@ public class GridDragSourceConnector extends DragSourceExtensionConnector {
                     .setDelayToCancelTouchScroll(-1);
             touchScrollDelayUsed = false;
         }
+
+        super.onUnregister();
     }
 
     private Grid<JsonObject> getGrid() {

client/src/main/java/com/vaadin/client/extensions/DragSourceExtensionConnector.java

@@ -27,7 +27,6 @@ import com.google.gwt.dom.client.Style.Position;
 import com.google.gwt.user.client.ui.Image;
 import com.google.gwt.user.client.ui.Widget;
 import com.vaadin.client.BrowserInfo;
-import com.vaadin.client.ComponentConnector;
 import com.vaadin.client.ServerConnector;
 import com.vaadin.client.annotations.OnStateChange;
 import com.vaadin.client.ui.AbstractComponentConnector;
@@ -62,15 +61,11 @@ public class DragSourceExtensionConnector extends AbstractExtensionConnector {
     private final EventListener dragStartListener = this::onDragStart;
     private final EventListener dragEndListener = this::onDragEnd;
 
-    /**
-     * Widget of the drag source component.
-     */
     private Widget dragSourceWidget;
 
     @Override
     protected void extend(ServerConnector target) {
-        dragSourceWidget = ((ComponentConnector) target).getWidget();
-
+        dragSourceWidget = ((AbstractComponentConnector) target).getWidget();
         // HTML5 DnD is by default not enabled for mobile devices
         if (BrowserInfo.get().isTouchDevice() && !getConnection()
                 .getUIConnector().isMobileHTML5DndEnabled()) {
@@ -138,14 +133,21 @@ public class DragSourceExtensionConnector extends AbstractExtensionConnector {
 
     @Override
     public void onUnregister() {
-        super.onUnregister();
+        AbstractComponentConnector parent = (AbstractComponentConnector) getParent();
+        // if parent is null, the whole component has been removed,
+        // no need to do clean up then
+        if (parent != null) {
+            parent.onDragSourceDetached();
 
-        Element dragSource = getDraggableElement();
+            Element dragSource = getDraggableElement();
 
-        removeDraggable(dragSource);
-        removeDragListeners(dragSource);
+            removeDraggable(dragSource);
+            removeDragListeners(dragSource);
 
-        ((AbstractComponentConnector) getParent()).onDragSourceDetached();
+            dragSourceWidget = null;
+        }
+
+        super.onUnregister();
     }
 
     @OnStateChange("resources")

client/src/main/java/com/vaadin/client/extensions/DropTargetExtensionConnector.java

@@ -26,7 +26,6 @@ import com.google.gwt.dom.client.Element;
 import com.google.gwt.dom.client.NativeEvent;
 import com.google.gwt.user.client.ui.Widget;
 import com.vaadin.client.BrowserInfo;
-import com.vaadin.client.ComponentConnector;
 import com.vaadin.client.ServerConnector;
 import com.vaadin.client.ui.AbstractComponentConnector;
 import com.vaadin.shared.ui.Connect;
@@ -76,9 +75,6 @@ public class DropTargetExtensionConnector extends AbstractExtensionConnector {
     private final EventListener dragLeaveListener = this::onDragLeave;
     private final EventListener dropListener = this::onDrop;
 
-    /**
-     * Widget of the drop target component.
-     */
     private Widget dropTargetWidget;
 
     /**
@@ -89,7 +85,7 @@ public class DropTargetExtensionConnector extends AbstractExtensionConnector {
 
     @Override
     protected void extend(ServerConnector target) {
-        dropTargetWidget = ((ComponentConnector) target).getWidget();
+        dropTargetWidget = ((AbstractComponentConnector) target).getWidget();
 
         // HTML5 DnD is by default not enabled for mobile devices
         if (BrowserInfo.get().isTouchDevice() && !getConnection()
@@ -139,13 +135,21 @@ public class DropTargetExtensionConnector extends AbstractExtensionConnector {
 
     @Override
     public void onUnregister() {
-        super.onUnregister();
+        AbstractComponentConnector parent = (AbstractComponentConnector) getParent();
+        // parent is null when the component has been removed,
+        // clean up only if only the extension was removed
+        if (parent != null) {
+            parent.onDropTargetDetached();
 
-        removeDropListeners(getDropTargetElement());
-        ((AbstractComponentConnector) getParent()).onDropTargetDetached();
+            removeDropListeners(getDropTargetElement());
 
-        // Remove drop target indicator
-        removeDropTargetStyle();
+            // Remove drop target indicator
+            removeDropTargetStyle();
+
+            dropTargetWidget = null;
+        }
+
+        super.onUnregister();
     }
 
     /**
@@ -313,12 +317,13 @@ public class DropTargetExtensionConnector extends AbstractExtensionConnector {
      * Initiates a server RPC for the drop event.
      *
      * @param types
-     *         List of data types from {@code DataTransfer.types} object.
+     *            List of data types from {@code DataTransfer.types} object.
      * @param data
-     *         Map containing all types and corresponding data from the {@code
+     *            Map containing all types and corresponding data from the
+     *            {@code
      *         DataTransfer} object.
      * @param dropEffect
-     *         The desired drop effect.
+     *            The desired drop effect.
      */
     protected void sendDropEventToServer(List<String> types,
             Map<String, String> data, String dropEffect,
@@ -329,15 +334,17 @@ public class DropTargetExtensionConnector extends AbstractExtensionConnector {
     /**
      * Add class name for the drop target element indicating that data can be
      * dropped onto it. The class name has the following format:
+     *
      * <pre>
      *     [primaryStyleName]-droptarget
      * </pre>
-     * The added class name is update
-     * automatically by the framework when the primary style name changes.
+     *
+     * The added class name is update automatically by the framework when the
+     * primary style name changes.
      */
     protected void addDropTargetStyle() {
-        getDropTargetElement().addClassName(
-                getStylePrimaryName(getDropTargetElement())
+        getDropTargetElement()
+                .addClassName(getStylePrimaryName(getDropTargetElement())
                         + STYLE_SUFFIX_DROPTARGET);
     }
 
@@ -346,8 +353,8 @@ public class DropTargetExtensionConnector extends AbstractExtensionConnector {
      * be dropped onto it.
      */
     protected void removeDropTargetStyle() {
-        getDropTargetElement().removeClassName(
-                getStylePrimaryName(getDropTargetElement())
+        getDropTargetElement()
+                .removeClassName(getStylePrimaryName(getDropTargetElement())
                         + STYLE_SUFFIX_DROPTARGET);
     }
 

uitest/src/main/java/com/vaadin/tests/dnd/DragSourcesInTabSheet.java

@@ -0,0 +1,83 @@
+package com.vaadin.tests.dnd;
+
+import com.vaadin.annotations.Widgetset;
+import com.vaadin.server.VaadinRequest;
+import com.vaadin.shared.ui.grid.DropMode;
+import com.vaadin.tests.components.AbstractTestUIWithLog;
+import com.vaadin.tests.data.bean.Person;
+import com.vaadin.ui.Button;
+import com.vaadin.ui.Grid;
+import com.vaadin.ui.Label;
+import com.vaadin.ui.TabSheet;
+import com.vaadin.ui.VerticalLayout;
+import com.vaadin.ui.Window;
+import com.vaadin.ui.components.grid.GridDragSource;
+import com.vaadin.ui.components.grid.GridDropTarget;
+import com.vaadin.ui.dnd.DragSourceExtension;
+import com.vaadin.ui.dnd.DropTargetExtension;
+
+@Widgetset("com.vaadin.DefaultWidgetSet")
+public class DragSourcesInTabSheet extends AbstractTestUIWithLog {
+
+    @Override
+    protected void setup(VaadinRequest request) {
+        TabSheet tabSheet = new TabSheet();
+        addComponent(tabSheet);
+
+        tabSheet.addTab(new VerticalLayout(createLabels()))
+                .setCaption("Labels");
+
+        Button dsButton = new Button("DragSource");
+        new DragSourceExtension<>(dsButton)
+                .addDragEndListener(event -> log("drag end button"));
+        Button dtButton = new Button("DropTarget");
+        new DropTargetExtension<>(dtButton).addDropListener(event -> log("drop "
+                + event.getDragSourceComponent().orElse(null) + " on button"));
+        tabSheet.addTab(new VerticalLayout(dsButton, dtButton))
+                .setCaption("Buttons");
+
+        tabSheet.addTab(new VerticalLayout(createGrids())).setCaption("Grids");
+
+        addComponent(new Button("Open window", event -> openWindow()));
+    }
+
+    private Label[] createLabels() {
+        Label dragSource = new Label("DragSource");
+        new DragSourceExtension<>(dragSource)
+                .addDragEndListener(event -> log("drag end label"));
+        Label dropTarget = new Label("DropTarget");
+        new DropTargetExtension<>(dropTarget).addDropListener(event -> log(
+                "drop " + event.getDragSourceComponent().orElse(null)
+                        + " on label"));
+        return new Label[] { dragSource, dropTarget };
+    }
+
+    private Grid[] createGrids() {
+        Grid<Person> dsGrid = new Grid<>(Person.class);
+        dsGrid.setItems(Person.createTestPerson1(), Person.createTestPerson2());
+        new GridDragSource<>(dsGrid).addGridDragEndListener(event -> log(
+                "drag end " + event.getDraggedItems().iterator().next()));
+        Grid<Person> dtGrid = new Grid<>(Person.class);
+        dtGrid.setItems(Person.createTestPerson1(), Person.createTestPerson2());
+        new GridDropTarget<>(dtGrid, DropMode.BETWEEN)
+                .addGridDropListener(event -> log("drop on grid row "
+                        + event.getDropTargetRow().orElse(null) + " "
+                        + event.getDragData().orElse(null)));
+        return new Grid[] { dsGrid, dtGrid };
+    }
+
+    private void openWindow() {
+        Window window = new Window("Window with drag sources");
+        VerticalLayout layout = new VerticalLayout();
+        layout.addComponents(createLabels());
+        layout.addComponents(createGrids());
+        window.setContent(layout);
+        addWindow(window);
+    }
+
+    @Override
+    protected String getTestDescription() {
+        return "Verify that removing drag source and drop target components in a tabsheet/window works";
+    }
+
+}