Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Grid renders incorrectly cells having Layout components #12608

Closed
mikke-alekstra opened this issue Mar 13, 2024 · 3 comments
Closed

Grid renders incorrectly cells having Layout components #12608

mikke-alekstra opened this issue Mar 13, 2024 · 3 comments
Assignees
Milestone

Comments

@mikke-alekstra
Copy link

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);

    }

}
@Ansku Ansku self-assigned this Mar 14, 2024
@thevaadinman
Copy link
Contributor

thevaadinman commented Apr 3, 2024

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

@mikke-alekstra
Copy link
Author

@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
Copy link
Contributor

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants