vaadin / flow

Vaadin Flow is a Java framework binding Vaadin web components to Java. This is part of Vaadin 10+.
Apache License 2.0
616 stars 167 forks source link

Div's addComponentAtIndex() generates wrong client-side code #6113

Open zanonmark opened 5 years ago

zanonmark commented 5 years ago

I'm on 14.0.0rc6 and, as briefly described here, I created a CssGridLayout class extending Div.

Sometimes (not always!), when I use .addComponentAtIndex(), a strange behaviour occurs:

So there is a discrepancy between client-side and server-side HTML (which is the right one). When I do a page refresh (F5), the problem disappears as the client-side HTML is syncronized with the server one.

Is there anything I'm missing, or is it a bug? Is there a way to circumvent it by forcing a refresh of the Div after calling .addComponentAtIndex()?

Thanks, MZ

zanonmark commented 5 years ago

As a workaround, one could .removeAll() and .add() back all of the components. Still the bug remains...

MZ

zanonmark commented 5 years ago

I attach code from my ACssGridLayout.java class.

The lines causing the issue are the last ones in .setComponent():

        this.remove(oldComponent);
        this.addComponentAtIndex((rowIndex * this.getColumnCount()) + columnIndex, newComponent);

I extended this class to create a dynamic filter: when I press the "+" icon a new row is added, but the "-" icon for the row is assigned to the wrong column on client-side (and to the right column on server-side)... until I refresh the browser page with F5.

Thanks, MZ

zanonmark commented 5 years ago

I also attach a couple of screenshots showing the issue with the dynamic filter.

        AButton deleteFilterWidget = new AButton(null, new AImage(AConstants.DELETE_ICON, ""), this::onClick);
        deleteFilterWidget.setWidth(AConstants.ICON_BUTTON_SIZE);
        deleteFilterWidget.addThemeVariants(ButtonVariant.LUMO_TERTIARY);
        deleteFilterWidget.addClassName(AConstants.RIGHT_POSITIONED_CLASS);
        deleteFilterWidget.setEnabled(null == filter ? true : filter.getEnabledMode());
        this.setComponent(3, rowIndex, deleteFilterWidget);

Just after the code execution and before any page refresh: screenshot-before-page-refresh

After a page refresh (F5): screenshot-after-page-refresh

MZ

zanonmark commented 5 years ago

I also attach another couple of screenshots showing the client-side HTML code change.

Just after the code execution and before any page refresh: client-side-html-before-page-refresh

After a page refresh (F5): client-side-html-after-page-refresh

Thanks, MZ

mehdi-vaadin commented 5 years ago

Hi @zanonmark, thanks for reporting this issue. However, it's really difficult to reproduce it especially because you are using your component (ACssGridLayout). Would you please create a minimal reproducible example with Vaadin components that shows this behavior?

zanonmark commented 5 years ago

Hi @mehdi-vaadin,

Unfortunately I can't separate my Vaadin components from the application, as they are too tightly connected. The code for ACssGridLayout is attached though.

Would it be useful if I extended and uploaded a Vaadin starter app with some of my components and a simple view to reproduce the issue?

Thanks, MZ

joheriks commented 5 years ago

Hi @zanonmark , a starter extended with the minimal changes to reproduce the issue would be of great value for further investigation.

zanonmark commented 5 years ago

mztest-buggy.zip

I attach a minimal starter app, extended from Vaadin's SimpleApp with the following changes:

To reproduce the bug, just run mvn jetty:run-war in mztest-ui/. A 5-column grid is displayed in a VerticalLayout above the login form. Pressing a "+" button generates a new row, which in its turn has a "+" button on the 5th column (ok) and - here's the bug! - should have a "-" button on the 4th column, but instead it is displayed in the 3rd. As stated above, this seems to be a misalignment between client-side and server-side code, as debugging the server-side code (see my previous attachments) shows that the generated HTML is correct and pressing F5 (if you can add the @PreserveOnRefresh annotation, which I wasn't able to add here) aligns the client code to the server one for the existing rows.

The problem is thrown by ACssGridLayout lines 197-198, which however seems to be correct code to me (remove the old component in a cell and set another). On lines 200-203 there's a workaround (remove everything and add back everything).

Interestingly, while preparing this starter app I noted that disabling AFilterChain lines 105-106 (i.e.: not adding the combobox on 1st column when pressing the "+" button) hides the bug.

Thanks, MZ

denis-anisimov commented 4 years ago

Too much unrelated information.

What I've got from the example the code is doing something similar to : (use https://github.com/vaadin/skeleton-starter-flow project and copy paste)

public MainView() {
        Div div = new Div();
        add(div);

        List<Div> children = new ArrayList<>();
        for (int i = 0; i < 5; i++) {
            Div child = new Div();
            div.add(child);
            children.add(child);
        }

        div.remove(children.get(0));
        div.addComponentAtIndex(0, createChild("one"));

        div.remove(children.get(3));
        div.addComponentAtIndex(3, createChild("two"));

        div.remove(children.get(4));
        div.addComponentAtIndex(4, createChild("three"));

        div.getChildren().forEach(child -> System.out
                .println("element :" +((Div) child).getText()));

    }

    private Div createChild(String text) {
        Div div = new Div();
        div.setText(text);
        return div;
    }

Console output gives me

element : one
element : 
element : 
element : two
element : three

Same structure is shown in the browser inspector.

I'm using platform 15.0.0.

The attached project uses 14.0.0.

So either the bug is fixed or it's not a bug at all. Please use this code to provide the example of the code which I can use to reproduce the issue. Please avoid thousands of line of totally unrelated code.

zanonmark commented 4 years ago

Thanks all for taking care of this issue.

First of all, I apologize for attaching that project, as it was not my intention to waste anybody's time, but I tried to strip my application to a minimum (merging the necessary files to a Vaadin skeleton project) where the bug could be actually reproduced. Because the point is that simple code like yours certainly works now - as it worked when I opened the ticket (I made many tests then). But the (possible) bug shows up on more complex situations, which I tried to identify by myself with no luck.

Now, I may certainly be wrong and there may be plenty of bugs on my application's side. But, as I only wrote server-side code (no JS, no client-side components), I cannot understand why I come to a point where Vaadin's client-side DOM is different from Vaadin's server-side structure (until you press F5 to force a page refresh with a full request to the server): IMHO, this should be considered as a Vaadin bug, although a rare one maybe.

The problem persists with Vaadin 14.1.28. To be clear, pressing the "+" button always generates:

  1. ComboBox
  2. Span
  3. Span
  4. "-" Button
  5. "+" Button

on server-side (System.out.println), and:

  1. ComboBox
  2. Span
  3. "-" Button // wrong place
  4. Span
  5. "+" Button

on client-side (browser inspector). The result is correct on the server-side (the only one I'm coding in), and I can't control what is sent and synchronized to the client-side, which leads me to still consider this bug report as valid.

Please let me know if I need to provide any additional information.

Thanks, MZ