fix: ensure row details stay open after refreshing item (#7374)

Author: vursen

vaadin-grid-flow-parent/vaadin-grid-flow-integration-tests/src/main/java/com/vaadin/flow/component/grid/it/GridDetailsRowPage.java

@@ -22,52 +22,57 @@
 import com.vaadin.flow.component.grid.Grid;
 import com.vaadin.flow.component.grid.Grid.SelectionMode;
 import com.vaadin.flow.component.html.Div;
-import com.vaadin.flow.data.bean.Person;
+import com.vaadin.flow.component.html.NativeButton;
 import com.vaadin.flow.data.provider.ListDataProvider;
 import com.vaadin.flow.data.renderer.ComponentRenderer;
 import com.vaadin.flow.router.Route;
 
 @Route("vaadin-grid/grid-details-row")
 public class GridDetailsRowPage extends Div {
 
-    private Grid<Person> grid = new Grid<>();
     private List<Person> items = new ArrayList<>();
+    private Grid<Person> grid = new Grid<>();
 
-    private int nbUpdates;
-    private Person person3 = new Person("Person 3", 2);
-    private Person person4 = new Person("Person 4", 1111);
+    private static record Person(int id, String name) {
+    }
 
     public GridDetailsRowPage() {
+        items.add(new Person(0, "Person 0"));
+        items.add(new Person(1, "Person 1"));
+        items.add(new Person(2, "Person 2"));
 
-        items.add(new Person("Person 1", 99));
-        items.add(new Person("Person 2", 1));
-        items.add(person3);
-        items.add(person4);
-
-        ListDataProvider<Person> ldp = new ListDataProvider<>(items);
-        grid.setItems(ldp);
+        ListDataProvider<Person> dataProvider = new ListDataProvider<>(items) {
+            @Override
+            public Object getId(Person person) {
+                return person.id();
+            };
+        };
+        grid.setItems(dataProvider);
         grid.setSelectionMode(SelectionMode.NONE);
-        grid.addColumn(Person::getFirstName).setHeader("name");
-        grid.setItemDetailsRenderer(new ComponentRenderer<>(
-                item -> new Button(item.getFirstName())));
+        grid.addColumn(person -> person.name()).setHeader("Name");
+        grid.setItemDetailsRenderer(
+                new ComponentRenderer<>(person -> new Button(person.name())));
 
-        add(grid, new Button("click to open details",
-                e -> setFirstAndSecondItemsVisible()));
-        Button updatePerson3 = new Button("update and refresh person 3", e -> {
-            nbUpdates++;
-            person3.setFirstName("Person 3 - updates " + nbUpdates);
-            grid.getDataProvider().refreshItem(person3);
+        NativeButton updatePerson2 = new NativeButton("Update person 2", e -> {
+            Person updatedPerson = new Person(2, "Updated Person 2");
+            items.set(2, updatedPerson);
+            grid.getDataProvider().refreshItem(updatedPerson);
         });
-        updatePerson3.setId("update-button");
-        add(updatePerson3);
+        updatePerson2.setId("update-person-2");
 
-        Button removeButton = new Button("remove person 4", e -> {
-            items.remove(person4);
+        NativeButton removePerson2 = new NativeButton("Remove person 2", e -> {
+            items.remove(2);
             grid.getDataProvider().refreshAll();
         });
-        removeButton.setId("remove-button");
-        add(removeButton);
+        removePerson2.setId("remove-person-2");
+
         setFirstAndSecondItemsVisible();
+
+        NativeButton openDetails = new NativeButton("Open details", e -> {
+            setFirstAndSecondItemsVisible();
+        });
+
+        add(grid, openDetails, updatePerson2, removePerson2);
     }
 
     public void setFirstAndSecondItemsVisible() {

vaadin-grid-flow-parent/vaadin-grid-flow-integration-tests/src/test/java/com/vaadin/flow/component/grid/it/GridDetailsRowIT.java

@@ -18,82 +18,74 @@
 import java.util.List;
 
 import org.junit.Assert;
+import org.junit.Before;
 import org.junit.Test;
 import org.openqa.selenium.By;
 import org.openqa.selenium.WebElement;
 
 import com.vaadin.flow.component.grid.testbench.GridElement;
+import com.vaadin.flow.component.grid.testbench.GridTHTDElement;
 import com.vaadin.flow.testutil.TestPath;
 import com.vaadin.tests.AbstractComponentIT;
 
 @TestPath("vaadin-grid/grid-details-row")
 public class GridDetailsRowIT extends AbstractComponentIT {
+    private GridElement grid;
 
-    @Test
-    public void gridTwoItemsSelectedWhenOpen() {
+    @Before
+    public void init() {
         open();
-        GridElement grid = $(GridElement.class).first();
-
-        // each detail contain a button
-        List<WebElement> detailsElements = getDetailsElements(grid);
+        grid = $(GridElement.class).first();
+    }
 
+    @Test
+    public void initiallyOpenedDetailsDisplayed() {
+        List<WebElement> detailsElements = getDetailsElements();
         Assert.assertEquals(2, detailsElements.size());
-
-        Assert.assertEquals("Person 1", detailsElements.get(0).getText());
-        Assert.assertEquals("Person 2", detailsElements.get(1).getText());
+        Assert.assertEquals("Person 0", detailsElements.get(0).getText());
+        Assert.assertEquals("Person 1", detailsElements.get(1).getText());
     }
 
     @Test
-    public void shouldNotThrowOnDetailsClick() {
-        open();
-        GridElement grid = $(GridElement.class).first();
+    public void clickDetails_doesNotThrow() {
         grid.getRow(1).getDetails().click(0, 0);
         checkLogsForErrors();
     }
 
-    /**
-     * Click on an item, hide the other details
-     */
     @Test
-    public void gridSelectItem4DisplayDetails() {
-        open();
-        GridElement grid = $(GridElement.class).first();
-        // select row 4
-        grid.getCell(3, 0).click();
-        waitUntil(e -> getDetailsElements(grid).size() == 1, 1);
-
-        // detail on row 3 has the correct text
-        Assert.assertEquals("Person 4",
-                getDetailsElements(grid).get(0).getText());
+    public void selectItem_onlyItsDetailsAreDisplayed() {
+        grid.getCell(2, 0).click();
+
+        List<WebElement> detailsElements = getDetailsElements();
+        Assert.assertEquals(1, detailsElements.size());
+        Assert.assertEquals("Person 2", detailsElements.get(0).getText());
     }
 
-    /**
-     * If the details of an item is opened and the item updated then the detail
-     * should be updated
-     */
     @Test
-    public void gridUpdateItemUpdateDetails() {
-        open();
-        GridElement grid = $(GridElement.class).first();
-        // select row 3
+    public void updateItem_detailsUpdated() {
         grid.getCell(2, 0).click();
-        waitUntil(e -> getDetailsElements(grid).size() == 1, 1);
 
-        // detail on row 3 has the correct text
-        Assert.assertEquals("Person 3",
-                getDetailsElements(grid).get(0).getText());
+        GridTHTDElement details = grid.getRow(2).getDetails();
 
-        WebElement updateButton = findElement(By.id("update-button"));
-        updateButton.click();
-        waitUntil(e -> getDetailsElements(grid).size() == 1, 1);
+        Assert.assertFalse(details.hasAttribute("hidden"));
+        Assert.assertEquals("Person 2", details.getText());
 
-        // detail on row 3 has the correct text
-        Assert.assertEquals("Person 3 - updates 1",
-                getDetailsElements(grid).get(0).getText());
+        findElement(By.id("update-person-2")).click();
+
+        Assert.assertFalse(details.hasAttribute("hidden"));
+        Assert.assertEquals("Updated Person 2", details.getText());
+    }
+
+    @Test
+    public void removeItem_detailsRemoved() {
+        grid.getCell(2, 0).click();
+        findElement(By.id("remove-person-2")).click();
 
+        GridTHTDElement details = grid.getRow(1).getDetails();
+        Assert.assertTrue(details.hasAttribute("hidden"));
     }
 
-    private List<WebElement> getDetailsElements(GridElement grid) {
+    private List<WebElement> getDetailsElements() {
         return grid.findElements(By.tagName("vaadin-button"));
     }
 }

vaadin-grid-flow-parent/vaadin-grid-flow/src/main/java/com/vaadin/flow/component/grid/Grid.java

@@ -1264,7 +1264,7 @@ protected Grid<T> getGrid() {
      */
     private class DetailsManager extends AbstractGridExtension<T> {
 
-        private final HashSet<T> detailsVisible = new HashSet<>();
+        private final HashMap<Object, T> detailsVisible = new HashMap<>();
 
         /**
          * Constructs a new details manager for the given grid.
@@ -1286,17 +1286,19 @@ public DetailsManager(Grid<T> grid) {
          *            {@code false} if it should be hidden
          */
         public void setDetailsVisible(T item, boolean visible) {
+            Object itemId = getItemId(item);
+
             boolean refresh = false;
             if (!visible) {
-                refresh = detailsVisible.remove(item);
+                refresh = detailsVisible.remove(itemId) != null;
             } else {
-                detailsVisible.add(item);
+                detailsVisible.put(itemId, item);
                 refresh = true;
             }
 
             if (itemDetailsDataGenerator != null && refresh) {
                 refresh(item);
-                if (!detailsVisible.contains(item)) {
+                if (!detailsVisible.containsKey(itemId)) {
                     itemDetailsDataGenerator.destroyData(item);
                 }
             }
@@ -1313,7 +1315,7 @@ public void setDetailsVisible(T item, boolean visible) {
          */
         public boolean isDetailsVisible(T item) {
             return itemDetailsDataGenerator != null
-                    && detailsVisible.contains(item);
+                    && detailsVisible.containsKey(getItemId(item));
         }
 
         @Override
@@ -1332,7 +1334,7 @@ public void generateData(T item, JsonObject jsonObject) {
          */
         @Override
         public void destroyData(T item) {
-            detailsVisible.remove(item);
+            detailsVisible.remove(getItemId(item));
             if (itemDetailsDataGenerator != null) {
                 itemDetailsDataGenerator.destroyData(item);
             }
@@ -1361,17 +1363,24 @@ public void refreshData(T item) {
 
         private void setDetailsVisibleFromClient(Set<T> items) {
             Set<T> toRefresh = new HashSet<>();
-            toRefresh.addAll(detailsVisible);
+            toRefresh.addAll(detailsVisible.values());
             toRefresh.addAll(items);
 
             detailsVisible.clear();
-            detailsVisible.addAll(items);
+            for (T item : items) {
+                detailsVisible.put(getItemId(item), item);
+            }
+
             if (itemDetailsDataGenerator != null) {
                 for (T item : toRefresh) {
                     refresh(item);
                 }
             }
         }
+
+        private Object getItemId(T item) {
+            return getDataProvider().getId(item);
+        }
     }
 
     private class GridArrayUpdaterImpl implements GridArrayUpdater {