Button in a Dialog with LitTemplate: Button caption gets duplicated when dialog is closed and reopened

[TomRauchenwald38]

Description of the bug / feature

The caption of Buttons in a Dialog that contains a LitTemplate get duplicated if the dialog is opened more than once (if the Button is mapped server-side with @id).
Short gif highlighting the issue:

Minimal reproducible example

The code is based on vaadin/skeleton-starter-flow-spring - v18.
Client side template:

@customElement('test-form')
export class TestForm extends LitElement {
    render() {
        return html`
        <vaadin-button id="submitButton" theme="primary">OK</vaadin-button>
        `;
    }
}

Server side template:

@Tag("test-form")
@JsModule("./views/test-form.ts")
public class TestForm extends LitTemplate {
    
    @Id("submitButton")
    Button submitButton;
}

View:

public class MainView extends VerticalLayout {

    public MainView() {
        TestForm testForm = new TestForm();
        Dialog dialog = new Dialog(testForm);

        Button button = new Button("Click");
        button.addClickListener(ce -> dialog.open());
        add( button);
    }
}

Expected behavior

I would expect the caption to stay the same.

Actual behavior

The caption is rendered twice.

Versions

- Vaadin / Flow version: 18.0.6
- Java version: 8
- OS version: Linux
- Browser version (if applicable): Chrome
- Application Server (if applicable): Tomcat embedded
- IDE (if applicable): 

[mshabarov]

This issue occurs for other components as well. For example, if I use Notification and Label components, like so:

// template:
<label id="myLabel">Hi</label>

// java-side:
@Id("myLabel")
Label myCoolLabel;

// main view:
button.addClickListener(ce -> {
            Notification notification = new Notification(testForm);
            notification.setDuration(3000);
            notification.open();
});

then it shows Hi on the first click and HiHi on the second.

Thus, this is most probably an issue of template functionality, and not of components per se.

[mshabarov]

Reproduced with 14.5.0.alpha3 as well.

[mshabarov]

To workaround this problem you may wrap the button text into span if you only want to render the initial button text:

<vaadin-button id="submitButton" theme="primary"><span id="innerTextSpan">OK</span></vaadin-button>

but getting/setting the button text will be then problematic:

• adding @Id("innerTextSpan") Span innerTextSpan; will raise the issue again
• submitButton.getChildren() will return empty list.

[denis-anisimov]

This is the known bug in notification/dialog : https://github.com/vaadin/vaadin-notification/issues/117 and vaadin/vaadin-notification#97.
The difference in this scenario is:

• Dialog is used instead of Notification (which doesn't matter because in both cases template element is used inside the implementation).
• there is no setText call but there is an element mapped via @Id.

In fact the latter is quite similar to setText : there was a feature request to set attributes/properties/text values for the Java template instance based on the JS template content (as a result of parsing).
So when submitButton is created it gets OK value as a text value.
So mapping an element via @Id causes also setting text to that component.

As a result some modification happens for the @Id mapped component and everything else is just the same as in vaadin/vaadin-notification#97.

Both Dialog and Notification uses template DOM element to render the child component via flow-component-renderer. The template's innerHTML may not be updated once being set: vaadin/vaadin-notification#97 (comment).

So this is the problem in Dialog/Notification implementation and it became more visible now because of new feature which sets the text value for @Id mapped components .

The workaround for this specific issue is quite simple: don't reuse dialog instance, use the new one all the time:

        Button button = new Button("Click");
        button.addClickListener(ce -> {
              TestForm testForm = new TestForm();
              Dialog dialog = new Dialog(testForm);

               dialog.open();
       });
        add( button);

[tomivirkki]

The issue is not related to Dialog.

Modified the provided snippets by removing the Dialog and the Button from inside the template:

@customElement('test-form')
export class TestForm extends LitElement {
  render() {
    return html` <div id="submitButton">OK</div> `;
  }
}

Server side template:

@Tag("test-form")
@JsModule("./views/test-form.ts")
public class TestForm extends LitTemplate {

    @Id("submitButton")
    Div submitButton;
}

View:

public class MainView extends VerticalLayout {

    public MainView() {
        TestForm testForm = new TestForm();

        Button button = new Button("Click");
        button.addClickListener(ce -> {
            if (testForm.getParent().isPresent()) {
                remove(testForm);
            } else {
                add(testForm);
            }
        });
        add(button);
    }
    
}

[denis-anisimov]

Thanks @tomivirkki , apparently it was my mistake to identify this ticket with the existing Notification ticket (which is a genius bug in Dialog/Notification).
I had to reduce it to non -dialog case by myself but the description looked too similar.

Anyway, I will take this and proceed further investigation .

[Artur-]

This issue is as I see it the same as discussed at length in #10112

The difference is that in this case, no "clear all" change is sent before a text change event so the text node is appended instead of the content being overwritten.

The difference from the code perspective seems to be that it hits

        if (isAttached != wasAttached) {

in StateNode.collectChanges

[denis-anisimov]

It's similar indeed but not the same.

"Clear all" change event is sent indeed in the first case and not sent in the second case.
The problem is : "remove all" /"clear all" is not a state. It's one time action.
But it's logically treated as a state and that breaks everything.

[denis-anisimov]

Quite similar to #3771.

At the moment I don't see any reasonable way to fix this.

It's again caused by the changes in the templates behavior : set the text on the server side which matches the text in the template content.
That not the cause though. It reveals the problem (the problem is : clear all is not the state).

The same can be reproduced by manual setText call for the template: it removes the children and add a Text node child.
Then when the template is removed and added back the state is reapplied to the client side but since there is no "clear all" state it's not preserved anyhow and it's not re-applied. This is the main cause if the issue : hacky removeAllChildren method which already cased the problem and will cause more apparently.

I would revert the setting text for the Java template from template content which at least hides the original problem (the original problem has been introduced with removeAllChildren method). This is at least somehow working approach.
I don't see any other option at the moment.

[Legioth]

What if LitTemplate would do an explicit removeAllChildren() on @Id mapped elements when the template component is reattached?

[denis-anisimov]

What if LitTemplate would do an explicit removeAllChildren() on @Id mapped elements when the template component is reattached?

I don't see any problems with that.
Please provide the test which I can consider here.

Shortly: removeAllChildren is almost the same as remove every child . The latter one is a state: no need to care about it.
The only difference is: it also removes the client side children. And the fact that it doesn't matter when and how many times you remove the client side children allows to keep this removeAllChildren() method in API which should be removed otherwise completely since it breaks everything.

[denis-anisimov]

The fix changes removeAllChildren behavior : this needs to be mentioned in the release notes (reattached node persists its state "removed children": previously it wasn't a case).

[vaadin-bot]

This ticket/PR has been released with platform 19.0.0. For prerelease versions, it will be included in its final version.