Fix event handling when Grid is inside a Composite (#13334)

Change-Id: I2092e6a4afc3d67f74f2971af0f410012cb62008

client/src/com/vaadin/client/Util.java

@@ -46,6 +46,7 @@ import com.google.gwt.user.client.DOM;
 import com.google.gwt.user.client.Event;
 import com.google.gwt.user.client.EventListener;
 import com.google.gwt.user.client.Window;
+import com.google.gwt.user.client.ui.Composite;
 import com.google.gwt.user.client.ui.HasWidgets;
 import com.google.gwt.user.client.ui.RootPanel;
 import com.google.gwt.user.client.ui.Widget;
@@ -893,6 +894,13 @@ public class Util {
     /**
      * Helper method to find first instance of given Widget type found by
      * traversing DOM upwards from given element.
+     * <p>
+     * <strong>Note:</strong> If {@code element} is inside some widget {@code W}
+     * , <em>and</em> {@code W} in turn is wrapped in a {@link Composite}
+     * {@code C}, this method will not find {@code W}. It returns either
+     * {@code C} or null, depending on whether the class parameter matches. This
+     * may also be the case with other Composite-like classes that hijack the
+     * event handling of their child widget(s).
      *
      * @param element
      *            the element where to start seeking of Widget

client/src/com/vaadin/client/ui/grid/Grid.java

@@ -167,7 +167,7 @@ public class Grid<T> extends ResizeComposite implements
         protected void dispatch(HANDLER handler) {
             EventTarget target = getNativeEvent().getEventTarget();
             if (Element.is(target)
-                    && Util.findWidget(Element.as(target), null) == grid) {
+                    && !grid.isElementInChildWidget(Element.as(target))) {
 
                 focusedCell = grid.cellFocusHandler.getFocusedCell();
                 GridSection section = GridSection.FOOTER;
@@ -2318,8 +2318,7 @@ public class Grid<T> extends ResizeComposite implements
 
         Element e = Element.as(target);
         RowContainer container = escalator.findRowContainer(e);
-        Cell cell = null;
-        boolean isGrid = Util.findWidget(e, null) == this;
+        Cell cell;
 
         String eventType = event.getType();
         if (container == null) {
@@ -2357,7 +2356,7 @@ public class Grid<T> extends ResizeComposite implements
         // Fire GridKeyEvents and pass the event to escalator.
         super.onBrowserEvent(event);
 
-        if (isGrid) {
+        if (!isElementInChildWidget(e)) {
 
             // Sorting through header Click / KeyUp
             if (handleHeaderDefaultRowEvent(event, container, cell)) {
@@ -2378,6 +2377,26 @@ public class Grid<T> extends ResizeComposite implements
         }
     }
 
+    private boolean isElementInChildWidget(Element e) {
+        Widget w = Util.findWidget(e, null);
+
+        if (w == this) {
+            return false;
+        }
+
+        /*
+         * If e is directly inside this grid, but the grid is wrapped in a
+         * Composite, findWidget is not going to find this, only the wrapper.
+         * Thus we need to check its parents to see if we encounter this; if we
+         * don't, the found widget is actually a parent of this, so we should
+         * return false.
+         */
+        while (w != null && w != this) {
+            w = w.getParent();
+        }
+        return w != null;
+    }
+
     private boolean handleEditorRowEvent(Event event, RowContainer container,
             Cell cell) {
 

client/src/com/vaadin/client/ui/grid/GridUtil.java

@@ -16,7 +16,9 @@
 package com.vaadin.client.ui.grid;
 
 import com.google.gwt.dom.client.Element;
+import com.google.gwt.user.client.ui.Composite;
 import com.google.gwt.user.client.ui.Widget;
+import com.vaadin.client.Util;
 
 /**
  * Utilities for working with Grid.
@@ -41,6 +43,28 @@ public class GridUtil {
         return container != null ? container.getCell(e) : null;
     }
 
+    /**
+     * Returns the Grid instance containing the given element, if any.
+     * <p>
+     * <strong>Note:</strong> This method may not work reliably if the grid in
+     * question is wrapped in a {@link Composite} <em>unless</em> the element is
+     * inside another widget that is a child of the wrapped grid; please refer
+     * to the note in {@link Util#findWidget(Element, Class) Util.findWidget}
+     * for details.
+     * 
+     * @param e
+     *            the element whose parent grid to find
+     * @return the parent grid or null if none found.
+     */
+    public static Grid<?> findClosestParentGrid(Element e) {
+        Widget w = Util.findWidget(e, null);
+
+        while (w != null && !(w instanceof Grid)) {
+            w = w.getParent();
+        }
+        return (Grid<?>) w;
+    }
+
     /**
      * Accesses the package private method Widget#setParent()
      * 

client/src/com/vaadin/client/ui/grid/renderers/ClickableRenderer.java

@@ -16,6 +16,8 @@
 package com.vaadin.client.ui.grid.renderers;
 
 import com.google.gwt.dom.client.BrowserEvents;
+import com.google.gwt.dom.client.Element;
+import com.google.gwt.dom.client.EventTarget;
 import com.google.gwt.event.dom.client.ClickEvent;
 import com.google.gwt.event.dom.client.ClickHandler;
 import com.google.gwt.event.dom.client.DomEvent;
@@ -24,9 +26,9 @@ import com.google.gwt.event.shared.EventHandler;
 import com.google.gwt.event.shared.HandlerManager;
 import com.google.gwt.user.client.ui.Widget;
 import com.google.web.bindery.event.shared.HandlerRegistration;
-import com.vaadin.client.Util;
 import com.vaadin.client.ui.grid.Cell;
 import com.vaadin.client.ui.grid.Grid;
+import com.vaadin.client.ui.grid.GridUtil;
 
 /**
  * An abstract superclass for renderers that render clickable widgets. Click
@@ -112,10 +114,19 @@ public abstract class ClickableRenderer<T, W extends Widget> extends
         @Override
         @SuppressWarnings("unchecked")
         protected void dispatch(RendererClickHandler handler) {
-            cell = WidgetRenderer.getCell(getNativeEvent());
-            assert cell != null;
-            Grid<R> grid = Util.findWidget(cell.getElement(), Grid.class);
+
+            EventTarget target = getNativeEvent().getEventTarget();
+
+            if (!Element.is(target)) {
+                return;
+            }
+
+            Element e = Element.as(target);
+            Grid<R> grid = (Grid<R>) GridUtil.findClosestParentGrid(e);
+
+            cell = GridUtil.findCell(grid, e);
             row = grid.getDataSource().getRow(cell.getRow());
+
             handler.onClick(this);
         }
     }

client/src/com/vaadin/client/ui/grid/renderers/WidgetRenderer.java

@@ -15,15 +15,10 @@
  */
 package com.vaadin.client.ui.grid.renderers;
 
-import com.google.gwt.dom.client.Element;
-import com.google.gwt.dom.client.EventTarget;
-import com.google.gwt.dom.client.NativeEvent;
+import com.google.gwt.dom.client.TableCellElement;
 import com.google.gwt.user.client.ui.Widget;
 import com.vaadin.client.Util;
-import com.vaadin.client.ui.grid.Cell;
 import com.vaadin.client.ui.grid.FlyweightCell;
-import com.vaadin.client.ui.grid.Grid;
-import com.vaadin.client.ui.grid.GridUtil;
 
 /**
  * A renderer for rendering widgets into cells.
@@ -75,27 +70,35 @@ public abstract class WidgetRenderer<T, W extends Widget> extends
      */
     public abstract void render(FlyweightCell cell, T data, W widget);
 
-    protected W getWidget(Element e) {
-        return Util.findWidget(e.getFirstChildElement(), null);
+    /**
+     * Returns the widget contained inside the given cell element. Cannot be
+     * called for cells that do not contain a widget.
+     * 
+     * @param e
+     *            the element inside which to find a widget
+     * @return the widget inside the element
+     */
+    protected W getWidget(TableCellElement e) {
+        W w = getWidget(e, null);
+        assert w != null : "Widget not found inside cell";
+        return w;
     }
 
     /**
-     * Returns the cell instance corresponding to the element that the given
-     * event originates from. If the event does not originate from a grid cell,
-     * returns null.
-     *
-     * @param event
-     *            the event
-     * @return the cell or null if no such cell
+     * Returns the widget contained inside the given cell element, or null if it
+     * is not an instance of the given class. Cannot be called for cells that do
+     * not contain a widget.
+     * 
+     * @param e
+     *            the element inside to find a widget
+     * @param klass
+     *            the type of the widget to find
+     * @return the widget inside the element, or null if its type does not match
      */
-    protected static Cell getCell(NativeEvent event) {
-        EventTarget target = event.getEventTarget();
-        if (!Element.is(target)) {
-            return null;
-        }
-
-        Element elem = Element.as(target);
-        Grid<?> grid = Util.findWidget(elem, Grid.class);
-        return GridUtil.findCell(grid, elem);
+    protected static <W extends Widget> W getWidget(TableCellElement e,
+            Class<W> klass) {
+        W w = Util.findWidget(e.getFirstChildElement(), klass);
+        assert w == null || w.getElement() == e.getFirstChildElement() : "Widget not found inside cell";
+        return w;
     }
 }

uitest/src/com/vaadin/tests/components/grid/basicfeatures/GridBasicClientFeaturesTest.java

@@ -15,11 +15,14 @@
  */
 package com.vaadin.tests.components.grid.basicfeatures;
 
-import org.openqa.selenium.By;
 import org.openqa.selenium.Dimension;
 import org.openqa.selenium.WebElement;
 import org.openqa.selenium.interactions.Actions;
 
+import com.vaadin.testbench.By;
+import com.vaadin.testbench.TestBenchElement;
+import com.vaadin.tests.components.grid.GridElement;
+
 /**
  * Variant of GridBasicFeaturesTest to be used with GridBasicClientFeatures.
  * 
@@ -28,11 +31,26 @@ import org.openqa.selenium.interactions.Actions;
  */
 public abstract class GridBasicClientFeaturesTest extends GridBasicFeaturesTest {
 
+    private boolean composite = false;
+
     @Override
     protected Class<?> getUIClass() {
         return GridBasicClientFeatures.class;
     }
 
+    @Override
+    protected String getDeploymentPath() {
+        String path = super.getDeploymentPath();
+        if (composite) {
+            path += (path.contains("?") ? "&" : "?") + "composite";
+        }
+        return path;
+    }
+
+    protected void setUseComposite(boolean useComposite) {
+        composite = useComposite;
+    }
+
     @Override
     protected void selectMenu(String menuCaption) {
         WebElement menuElement = getMenuElement(menuCaption);
@@ -60,4 +78,15 @@ public abstract class GridBasicClientFeaturesTest extends GridBasicFeaturesTest
                 .click().perform();
     }
 
+    @Override
+    protected GridElement getGridElement() {
+        if (composite) {
+            // Composite requires the basic client features widget for subparts
+            return ((TestBenchElement) findElement(By
+                    .vaadin("//GridBasicClientFeaturesWidget")))
+                    .wrap(GridElement.class);
+        } else {
+            return super.getGridElement();
+        }
+    }
 }

uitest/src/com/vaadin/tests/components/grid/basicfeatures/client/GridClientCompositeEditorRowTest.java

@@ -0,0 +1,13 @@
+package com.vaadin.tests.components.grid.basicfeatures.client;
+
+import org.junit.Before;
+
+public class GridClientCompositeEditorRowTest extends GridEditorRowClientTest {
+
+    @Override
+    @Before
+    public void setUp() {
+        setUseComposite(true);
+        super.setUp();
+    }
+}

uitest/src/com/vaadin/tests/components/grid/basicfeatures/client/GridClientCompositeFooterTest.java

@@ -0,0 +1,11 @@
+package com.vaadin.tests.components.grid.basicfeatures.client;
+
+import org.junit.Before;
+
+public class GridClientCompositeFooterTest extends GridFooterTest {
+
+    @Before
+    public void setUp() {
+        setUseComposite(true);
+    }
+}

uitest/src/com/vaadin/tests/components/grid/basicfeatures/client/GridClientCompositeHeaderTest.java

@@ -0,0 +1,11 @@
+package com.vaadin.tests.components.grid.basicfeatures.client;
+
+import org.junit.Before;
+
+public class GridClientCompositeHeaderTest extends GridHeaderTest {
+
+    @Before
+    public void setUp() {
+        setUseComposite(true);
+    }
+}

uitest/src/com/vaadin/tests/components/grid/basicfeatures/client/GridClientCompositeKeyEventsTest.java

@@ -0,0 +1,12 @@
+package com.vaadin.tests.components.grid.basicfeatures.client;
+
+import org.junit.Before;
+
+public class GridClientCompositeKeyEventsTest extends
+        GridClientKeyEventsTest {
+
+    @Before
+    public void setUp() {
+        setUseComposite(true);
+    }
+}

uitest/src/com/vaadin/tests/components/grid/basicfeatures/client/GridClientCompositeSelectionTest.java

@@ -0,0 +1,11 @@
+package com.vaadin.tests.components.grid.basicfeatures.client;
+
+import org.junit.Before;
+
+public class GridClientCompositeSelectionTest extends GridClientSelectionTest {
+
+    @Before
+    public void setUp() {
+        setUseComposite(true);
+    }
+}

uitest/src/com/vaadin/tests/components/grid/basicfeatures/client/GridEditorRowTest.java → uitest/src/com/vaadin/tests/components/grid/basicfeatures/server/GridEditorRowTest.java

@@ -13,7 +13,7 @@
  * License for the specific language governing permissions and limitations under
  * the License.
  */
-package com.vaadin.tests.components.grid.basicfeatures.client;
+package com.vaadin.tests.components.grid.basicfeatures.server;
 
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNotNull;

uitest/src/com/vaadin/tests/widgetset/client/grid/GridBasicClientFeaturesWidget.java

@@ -28,7 +28,9 @@ import com.google.gwt.event.dom.client.ClickEvent;
 import com.google.gwt.event.dom.client.ClickHandler;
 import com.google.gwt.event.shared.HandlerRegistration;
 import com.google.gwt.user.client.Timer;
+import com.google.gwt.user.client.Window;
 import com.google.gwt.user.client.ui.Button;
+import com.google.gwt.user.client.ui.Composite;
 import com.google.gwt.user.client.ui.HTML;
 import com.google.gwt.user.client.ui.Label;
 import com.google.gwt.user.client.ui.TextBox;
@@ -367,7 +369,20 @@ public class GridBasicClientFeaturesWidget extends
         createInternalsMenu();
 
         grid.getElement().getStyle().setZIndex(0);
-        addNorth(grid, 400);
+
+        //
+        // Composite wrapping for grid.
+        //
+        boolean isComposite = Window.Location.getParameter("composite") != null;
+        if (isComposite) {
+            addNorth(new Composite() {
+                {
+                    initWidget(grid);
+                }
+            }, 400);
+        } else {
+            addNorth(grid, 400);
+        }
 
         createKeyHandlers();
     }