Use correct indexes in multiselect checkboxes after removing rows (#8072)

Fixes #8011

client/src/main/java/com/vaadin/client/widget/grid/selection/MultiSelectionRenderer.java

@@ -26,6 +26,7 @@ import com.google.gwt.dom.client.BrowserEvents;
 import com.google.gwt.dom.client.Element;
 import com.google.gwt.dom.client.NativeEvent;
 import com.google.gwt.dom.client.TableElement;
+import com.google.gwt.dom.client.TableRowElement;
 import com.google.gwt.dom.client.TableSectionElement;
 import com.google.gwt.event.dom.client.ClickEvent;
 import com.google.gwt.event.dom.client.ClickHandler;
@@ -45,6 +46,7 @@ import com.vaadin.client.widget.grid.RendererCellReference;
 import com.vaadin.client.widget.grid.events.GridEnabledEvent;
 import com.vaadin.client.widget.grid.events.GridEnabledHandler;
 import com.vaadin.client.widget.grid.selection.SelectionModel.Multi.Batched;
+import com.vaadin.client.widgets.Escalator.AbstractRowContainer;
 import com.vaadin.client.widgets.Grid;
 
 /**
@@ -319,7 +321,7 @@ public class MultiSelectionRenderer<T>
 
             int constrainedPageY = Math.max(bodyAbsoluteTop,
                     Math.min(bodyAbsoluteBottom, pageY));
-            int logicalRow = getLogicalRowIndex(WidgetUtil
+            int logicalRow = getLogicalRowIndex(grid, WidgetUtil
                     .getElementFromPoint(initialPageX, constrainedPageY));
 
             int incrementOrDecrement = (logicalRow > lastModifiedLogicalRow) ? 1
@@ -586,8 +588,6 @@ public class MultiSelectionRenderer<T>
         }
     }
 
-    private static final String LOGICAL_ROW_PROPERTY_INT = "vEscalatorLogicalRow";
-
     private final Grid<T> grid;
     private HandlerRegistration nativePreviewHandlerRegistration;
 
@@ -633,8 +633,6 @@ public class MultiSelectionRenderer<T>
             CheckBox checkBox) {
         checkBox.setValue(data, false);
         checkBox.setEnabled(grid.isEnabled() && !grid.isEditorActive());
-        checkBox.getElement().setPropertyInt(LOGICAL_ROW_PROPERTY_INT,
-                cell.getRowIndex());
     }
 
     @Override
@@ -668,7 +666,7 @@ public class MultiSelectionRenderer<T>
 
     private void startDragSelect(NativeEvent event, final Element target) {
         injectNativeHandler();
-        int logicalRowIndex = getLogicalRowIndex(target);
+        int logicalRowIndex = getLogicalRowIndex(grid, target);
         autoScrollHandler.start(logicalRowIndex);
         event.preventDefault();
         event.stopPropagation();
@@ -687,7 +685,7 @@ public class MultiSelectionRenderer<T>
         }
     }
 
-    private int getLogicalRowIndex(final Element target) {
+    private int getLogicalRowIndex(Grid<T> grid, final Element target) {
         if (target == null) {
             return -1;
         }
@@ -707,7 +705,8 @@ public class MultiSelectionRenderer<T>
                 final Element checkbox = td.getFirstChildElement();
                 assert checkbox != null : "Checkbox has disappeared";
 
-                return checkbox.getPropertyInt(LOGICAL_ROW_PROPERTY_INT);
+                return ((AbstractRowContainer) grid.getEscalator().getBody())
+                        .getLogicalRowIndex((TableRowElement) tr);
             }
             tr = tr.getNextSiblingElement();
         }

client/src/main/java/com/vaadin/client/widgets/Escalator.java

@@ -53,7 +53,6 @@ import com.google.gwt.dom.client.TableSectionElement;
 import com.google.gwt.dom.client.Touch;
 import com.google.gwt.event.shared.HandlerRegistration;
 import com.google.gwt.logging.client.LogConfiguration;
-import com.google.gwt.user.client.Command;
 import com.google.gwt.user.client.DOM;
 import com.google.gwt.user.client.Window;
 import com.google.gwt.user.client.ui.RequiresResize;
@@ -469,9 +468,9 @@ public class Escalator extends Widget
                     if (isCurrentBrowserIE11OrEdge()) {
                         return vertical
                                 ? event.getNativeEvent().getClientY()
-                                + Window.getScrollTop()
+                                        + Window.getScrollTop()
                                 : event.getNativeEvent().getClientX()
-                                + Window.getScrollLeft();
+                                        + Window.getScrollLeft();
                     }
                     JsArray<Touch> a = event.getNativeEvent().getTouches();
                     return vertical ? a.get(0).getPageY() : a.get(0).getPageX();
@@ -564,9 +563,11 @@ public class Escalator extends Widget
             // (#18737),
             // otherwise allow touch only if there is a single touch in the
             // event
-            private boolean allowTouch(final TouchHandlerBundle.CustomTouchEvent event) {
+            private boolean allowTouch(
+                    final TouchHandlerBundle.CustomTouchEvent event) {
                 if (isCurrentBrowserIE11OrEdge()) {
-                    return (POINTER_EVENT_TYPE_TOUCH.equals(event.getPointerType()));
+                    return (POINTER_EVENT_TYPE_TOUCH
+                            .equals(event.getPointerType()));
                 } else {
                     return (event.getNativeEvent().getTouches().length() == 1);
                 }
@@ -1025,8 +1026,8 @@ public class Escalator extends Widget
         }-*/;
 
         /**
-         * Using pointerdown, pointermove, pointerup, and pointercancel for IE11 and Edge instead of
-         * touch* listeners (#18737)
+         * Using pointerdown, pointermove, pointerup, and pointercancel for IE11
+         * and Edge instead of touch* listeners (#18737)
          *
          * @param element
          */
@@ -1047,8 +1048,8 @@ public class Escalator extends Widget
         }-*/;
 
         /**
-         * Using pointerdown, pointermove, pointerup, and pointercancel for IE11 and Edge instead of
-         * touch* listeners (#18737)
+         * Using pointerdown, pointermove, pointerup, and pointercancel for IE11
+         * and Edge instead of touch* listeners (#18737)
          *
          * @param element
          */
@@ -1134,7 +1135,7 @@ public class Escalator extends Widget
         }
     }
 
-    protected abstract class AbstractRowContainer implements RowContainer {
+    public abstract class AbstractRowContainer implements RowContainer {
         private EscalatorUpdater updater = EscalatorUpdater.NULL;
 
         private int rows;
@@ -2160,7 +2161,14 @@ public class Escalator extends Widget
          */
         protected abstract double getHeightOfSection();
 
-        protected int getLogicalRowIndex(final TableRowElement tr) {
+        /**
+         * Gets the logical row index for the given table row element.
+         * 
+         * @param tr
+         *            the table row element inside this container.
+         * @return the logical index of the given element
+         */
+        public int getLogicalRowIndex(final TableRowElement tr) {
             return tr.getSectionRowIndex();
         };
 
@@ -3469,7 +3477,7 @@ public class Escalator extends Widget
         }
 
         @Override
-        protected int getLogicalRowIndex(final TableRowElement tr) {
+        public int getLogicalRowIndex(final TableRowElement tr) {
             assert tr
                     .getParentNode() == root : "The given element isn't a row element in the body";
             int internalIndex = visualRowOrder.indexOf(tr);
@@ -6905,6 +6913,7 @@ public class Escalator extends Widget
 
     /**
      * Internal method for checking whether the browser is IE11 or Edge
+     * 
      * @return true only if the current browser is IE11, or Edge
      */
     private static boolean isCurrentBrowserIE11OrEdge() {

client/src/main/java/com/vaadin/client/widgets/Grid.java

@@ -5799,11 +5799,11 @@ public class Grid<T> extends ResizeComposite implements HasSelectionHandlers<T>,
                         public void onStart() {
                             dragStarted();
                             dragger.getElement().appendChild(resizeElement);
-                            resizeElement.getStyle()
-                                    .setLeft((dragger.getElement()
-                                            .getOffsetWidth()
+                            resizeElement.getStyle().setLeft(
+                                    (dragger.getElement().getOffsetWidth()
                                             - resizeElement.getOffsetWidth())
-                                            * .5, Unit.PX);
+                                            * .5,
+                                    Unit.PX);
                             resizeElement.getStyle().setHeight(
                                     col.grid.getOffsetHeight(), Unit.PX);
                         }
@@ -6844,7 +6844,12 @@ public class Grid<T> extends ResizeComposite implements HasSelectionHandlers<T>,
         return editor;
     }
 
-    protected Escalator getEscalator() {
+    /**
+     * Gets the {@link Escalator} used by this Grid instnace.
+     * 
+     * @return the escalator instance, never <code>null</code>
+     */
+    public Escalator getEscalator() {
         return escalator;
     }
 

uitest/src/test/java/com/vaadin/tests/components/grid/basicfeatures/server/GridSelectionTest.java

@@ -18,6 +18,10 @@ package com.vaadin.tests.components.grid.basicfeatures.server;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
 
+import java.util.Arrays;
+import java.util.HashSet;
+
+import org.junit.Assert;
 import org.junit.Test;
 import org.openqa.selenium.Keys;
 import org.openqa.selenium.WebDriver;
@@ -31,6 +35,7 @@ import com.vaadin.testbench.elements.GridElement.GridCellElement;
 import com.vaadin.testbench.elements.GridElement.GridRowElement;
 import com.vaadin.tests.components.grid.basicfeatures.GridBasicFeatures;
 import com.vaadin.tests.components.grid.basicfeatures.GridBasicFeaturesTest;
+import com.vaadin.tests.components.grid.basicfeatures.element.CustomGridElement;
 
 public class GridSelectionTest extends GridBasicFeaturesTest {
 
@@ -396,6 +401,52 @@ public class GridSelectionTest extends GridBasicFeaturesTest {
                         "Exception occured, java.lang.NullPointerException: null"));
     }
 
+    @Test
+    public void testRemoveSelectedRowMulti() {
+        openTestURL();
+
+        setSelectionModelMulti();
+        CustomGridElement grid = getGridElement();
+        grid.getCell(5, 0).click();
+
+        selectMenuPath("Component", "Body rows", "Remove selected rows");
+        assertSelected();
+        grid.getCell(5, 0).click();
+        assertSelected(5);
+        grid.getCell(6, 0).click();
+        assertSelected(5, 6);
+        grid.getCell(5, 0).click();
+        assertSelected(6);
+        grid.getCell(5, 0).click();
+        grid.getCell(4, 0).click();
+        selectMenuPath("Component", "Body rows", "Remove selected rows");
+        assertSelected();
+        grid.getCell(0, 0).click();
+        assertSelected(0);
+        grid.getCell(5, 0).click();
+        assertSelected(0, 5);
+        grid.getCell(6, 0).click();
+        assertSelected(0, 5, 6);
+
+    }
+
+    private void assertSelected(Integer... selected) {
+        CustomGridElement grid = getGridElement();
+        HashSet<Integer> expected = new HashSet<Integer>(
+                Arrays.asList(selected));
+        for (int i = 0; i < 10; i++) {
+            boolean rowSelected = grid.getRow(i).isSelected();
+            if (expected.contains(i)) {
+                Assert.assertTrue("Expected row " + i + " to be selected",
+                        rowSelected);
+            } else {
+                Assert.assertFalse("Expected row " + i + " not to be selected",
+                        rowSelected);
+            }
+        }
+
+    }
+
     private void waitUntilCheckBoxValue(final WebElement checkBoxElememnt,
             final boolean expectedValue) {
         waitUntil(new ExpectedCondition<Boolean>() {