vaadin 8.9.3 - maybe grid spacer broken on grid with reusable detail

[alexandru78]

• Vaadin Framework version 8.9.3
• Browser version Chrome 79.0.3945.88
• Web container name and version Jetty 9.4.22
• Description of the bug Grid detail of previous selected item is visible and empty when it should not be visible at all. Steps to reproduce using the grid from the minimal reproducible example.
  1. user selects row 2 (the detail of row2 is good - it does show heavy label)
  2. user selects row 1 (the detail of row1 has a height double than needed - it does show heavy label, the detail of row2 is visible and empty)

Strange enough, the reported behaviour does not happen when user selects row2 after row1 : everything is as expected - row1 has no detail, the detail of row2 has a good height and shows heavy label.

I noticed this behaviour with Vaadin version 8.9.3. With Vaadin version 8.8.6 it is not possible to reproduce this behaviour.

• Minimal reproducible example `

  final ArrayList<ValueWrapper> rows = new ArrayList<>();
  rows.add(new ValueWrapper("ROW1"));
  rows.add(new ValueWrapper("ROW2"));
  
  Grid<ValueWrapper> grid = new Grid<>();
  grid.setSizeFull();
  grid.setDataProvider(new ListDataProvider<>(rows));
  grid.addColumn(ValueWrapper::getValue).setId("value").setCaption("Value");
  
  grid.setSelectionMode(Grid.SelectionMode.SINGLE);
  grid.addSelectionListener(event -> {
      final ListDataProvider<ValueWrapper> dataProvider = (ListDataProvider<ValueWrapper>) grid.getDataProvider();
      //hide details for all rows
      dataProvider.getItems().forEach(item -> grid.setDetailsVisible(item, false));
      //show details only for selected rows
      event.getAllSelectedItems().forEach(selected -> grid.setDetailsVisible(selected, true));
  });
  
  HorizontalLayout heavyLayout = new HorizontalLayout(new Label("heavy label"));
  grid.setDetailsGenerator(valueWrapper -> {
      heavyLayout.setParent(null);
      return heavyLayout;
  });
  
  final VerticalLayout windowContent = new VerticalLayout(grid);
  windowContent.setSizeFull();
  windowContent.setSpacing(false);
  windowContent.setMargin(false);
  
  Window window = new Window("Window caption");
  window.setHeight(80, Unit.PERCENTAGE);
  window.setWidth(80, Unit.PERCENTAGE);
  window.setContent(windowContent);
  UI.getCurrent().addWindow(window);`

`

private class ValueWrapper {
    private final String value;

    public ValueWrapper(String value) {
        super();
        this.value = value;
    }

    public String getValue() {
        return value;
    }
}

`

• Expected behavior Detail of previous selected items should not be visible.
• Actual behavior Sometimes the detail of previous selected items is visible. Sometimes becomes always if the user selects rows in some order.

[Ansku]

I'm afraid Grid doesn't actually support reusable details row contents, you are supposed to generate them from scratch for every row, just like with ComponentRenderer. I poked around a bit to see if the support could be added easily, but it's a timing issue (sometimes the detach is processed before the new details row handling, sometimes after, which causes a client-side exception and interrupts both processes) and I couldn't find any trivial way to enforce the order of operations or to explicitly trigger the detach before the default handling gets there without breaking other things. The client-side assumes that you can have multiple details rows open at the same time and it has no way to know that your server-side is enforcing things otherwise. That reusing contents worked even in 8.8.6 was accidental and presumably caused by a lot of Grid handling happening with a delay, which in turn caused many other layouting and performance issues, so unfortunately reintroducing those delays isn't really an option.

[alexandru78]

I'm afraid Grid doesn't actually support reusable details row contents, you are supposed to generate them from scratch for every row, just like with ComponentRenderer. I poked around a bit to see if the support could be added easily, but it's a timing issue (sometimes the detach is processed before the new details row handling, sometimes after, which causes a client-side exception and interrupts both processes) and I couldn't find any trivial way to enforce the order of operations or to explicitly trigger the detach before the default handling gets there without breaking other things. The client-side assumes that you can have multiple details rows open at the same time and it has no way to know that your server-side is enforcing things otherwise. That reusing contents worked even in 8.8.6 was accidental and presumably caused by a lot of Grid handling happening with a delay, which in turn caused many other layouting and performance issues, so unfortunately reintroducing those delays isn't really an option.

thank you for looking into this. Last interesting bit is the "ordering fact": in my real grid this behaviour does not happen if the user selects item going down - selecting row1, row3, row5 or some other row with an increasing index from the start to the bottom. yet it always happens if the user selects item backwards, starting from bottom to top.

do you think this might be random also? if that's the case I believe you can close this right away.

[Ansku]

Yes, the DetailsChangeHandler.dataUpdated within DetailsManagerConnector is generally called in the index order of all the rows that have been updated within the same server-client roundtrip, so when you move down, the initial spacer is removed before the new spacer is added. Going up it tries to add the new spacer before removing the old one, which causes issues.