vaadin / framework

Vaadin 6, 7, 8 is a Java framework for modern Java web applications.
http://vaadin.com/
Other
1.77k stars 729 forks source link

Grid renders incorrectly cells having Layout components #12608

Closed mikke-alekstra closed 2 months ago

mikke-alekstra commented 3 months ago

Vaadin 8.24.0 (also tested with 8.14.3) Latest Windows 11 Chrome (also tested with Debian Buster & Chromium) Simple Maven project with archetype vaadin-archetype-application jetty.plugin.version 9.4.53.v20231009

In my test I have a grid with two component rendering columns. Components are HorizontalLayouts with full size, containing two HorizontalLayouts with one aligned to the left and one aligned to the right. I have set the expand ratio for the left one to 1.

In my test when I start scrolling down on the grid suddenly the layout on the right starts to be aligned badly: bad_alignment

Please see below the source code of my test grid.

public class MyGrid extends Grid<Integer> {

    public MyGrid() {

        this.createColumn("column 1", 250);
        this.createColumn("column 2", 250);
        this.setWidth(520, Unit.PIXELS);

        List<Integer> items = new ArrayList<Integer>();

        for (int i = 0; i < 400; i ++) {
            items.add(i);
        }

        this.setItems(items);

    }

    private void createColumn(String caption, int width) {

        this.addColumn(i -> {

            HorizontalLayout leftLayout = new HorizontalLayout();
            leftLayout.setMargin(false);
            leftLayout.setSpacing(true);
            leftLayout.addComponents(new Label("label 1"), new Label("label 2"));

            HorizontalLayout rightLayout = new HorizontalLayout();
            rightLayout.setMargin(false);
            rightLayout.setSpacing(true);
            rightLayout.addComponents(new Label("label 3"));

            HorizontalLayout layout = new HorizontalLayout();
            layout.setMargin(false);
            layout.setSpacing(true);
            layout.setSizeFull();
            layout.addComponents(leftLayout, rightLayout);
            layout.setComponentAlignment(leftLayout, Alignment.MIDDLE_LEFT);
            layout.setComponentAlignment(rightLayout, Alignment.MIDDLE_RIGHT);
            layout.setExpandRatio(leftLayout, 1);

            return layout;

        }, new ComponentRenderer())
        .setWidth(width)
        .setCaption(caption);

    }

}
thevaadinman commented 2 months ago

@mikke-alekstra Could you please try the latest 8.25-SNAPSHOT to verify our fix?

mikke-alekstra commented 2 months ago

@thevaadinman Tested with 8.25-SNAPSHOT and the same test that failed with 8.24.0 passes with 8.25-SNAPSHOT. Rendering takes a bit of time when scrolling down (I assume Javascript is doing some heavy work) but after a small wait everything is rendered as expected. Thank you!

thevaadinman commented 2 months ago

@mikke-alekstra yeah, this is an unfortunate compromise for now. The fix is based on @TatuLund's implementation that hooks into Escalator's scroll event in GridConnector and schedules a re-layout. This induces some overhead, unfortunately; a more proper fix with less overhead would be to remove the scroll listener and instead schedule a partial re-layout for recycled rows during scrolling, either in Grid or in Escalator itself, depending on the architecutre. However, given the complexity of those components, we've opted for fastest deliverable for right now, as this bit of the code was very easy to hook into, while the internals of Grid and especially Escalator are pretty much the polar opposite; we'll re-visit this in the future if the performance hit proves excessive.