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

RBG inside polymer template inside a dialog resets the value unexpectedly. #166

Closed
denis-anisimov opened this issue Oct 5, 2020 · 8 comments · Fixed by vaadin/flow-components#275
Assignees
Labels
BFP Bug fix prioritised by a customer waiting for author Further information is requested

Comments

@denis-anisimov
Copy link

denis-anisimov commented Oct 5, 2020

Sorry that I'm using Java example but I don't know how make non Java example easily.
Java based example is used only for setup components. The bug is pure client side.

The original ticket is vaadin/vaadin-radio-button-flow#162

Steps to reproduce:
Use skeleton starter
https://github.com/vaadin/skeleton-starter-flow

Replace MainView with the content:

@Tag("main-form")
@JsModule("src/main-form.js")
public class MainView extends PolymerTemplate<TemplateModel>
        implements HasComponents {

    @Id("kind")
    private RadioButtonGroup<String> radioButtonGroup;

    public MainView() {
        radioButtonGroup = new RadioButtonGroup<String>();
        radioButtonGroup.setItems("a", "b", "c");

        NativeButton showDialog = new NativeButton("Open Dialog",
                event -> showDialog());
        add(showDialog);
    }

    private void showDialog() {
        radioButtonGroup.setValue("b");
        new Dialog(radioButtonGroup).open();
    }
}

Use this template for main-form.js :

import {html, PolymerElement} from '@polymer/polymer/polymer-element.js';
import '@vaadin/vaadin-radio-button/src/vaadin-radio-group.js';

class MainForm extends PolymerElement {
    static get template() {
        return html`
            <div>
                <vaadin-radio-group id="kind" colspan="2"></vaadin-radio-group>
            </div>
            <slot></slot>
        `;
    }

    static get is() {
        return 'main-form';
    }
}

customElements.define(MainForm.is, MainForm);
  • open browser
  • click the button
  • select the radio button group in the Chrome inspector
  • make sure the mouse pointer is inside the inspector
  • Use console in the inspector to set the value: current value "$0.value" should be "2" , set it to "1": $0.value=1.
  • Check visually (don't do anything!) that RBG value is set to "a" (from the previous value "b").
  • Move mouse pointer from inspector to the browser HTML view.
  • In most cases the value in RBG is set to "b" on this move.

So some "focus" event or something trigger value change back unexpectedly.
This is caused by vaadin-radio-button-group web component on the client side since no any server side events happen on move.

@web-padawan
Copy link
Member

Thanks for the code example. I wasn't able to reproduce this issue in Chrome, see the GIF below:

radio

It worked the same in Firefox and Safari.
I'm using the master branch of https://github.com/vaadin/skeleton-starter-flow which has V17.0.2

@web-padawan web-padawan added the waiting for author Further information is requested label Oct 7, 2020
@denis-anisimov
Copy link
Author

17.0.2.

My image:
(as mentioned : sometimes it doesn't happen, but for me in most cases it happens).

bug

Technically, it's not about this bug. This is just a demonstration that something is broken on the client side.
The original bug is vaadin/vaadin-radio-button-flow#162 but it contains a lot of unrelated things so I just simplified the scenario to show that it's not a server side issue: there is some client side listener which triggers the change value back.
The original bug is reproduced for sure.

@web-padawan
Copy link
Member

Could you please check your Chrome version? Also, did you check if it also happens in Firefox or Safari for you?

@denis-anisimov
Copy link
Author

denis-anisimov commented Oct 7, 2020

85.0.4183.

No, I have not checked neither Firefox nor Safari: it's enough that it can be reproduced in Chrome.

Have you checked the original bug ? Were you able to reproduce it ?

@web-padawan
Copy link
Member

Thanks. I have the same version, so no idea what's going on here. Will check the original bug.

@web-padawan
Copy link
Member

web-padawan commented Oct 7, 2020

Regarding the original issue, did some debugging with the project provided by the user and got this:

1st row dialog open

  • all the radio buttons have checked set to false, as expected.
  • value is set to 1 and checked radio button is updated before dialog opens.
  • after dialog opens, FlattenedNodesObserver goes through radio buttons and confirms they have proper checked

Screenshot 2020-10-07 at 14 57 48

2nd row dialog open

  • first radio button has checked set to true, which comes from the attribute - NOT expected.
  • value is set to 2 and checked radio button is updated before dialog opens.
  • after dialog opens, first radio button again has checked attribute
  • as a result, there are two more checked-changed events which reset checked state
  • by the time when FlattenedNodesObserver executes, radio buttons already have incorrect state

Screenshot 2020-10-07 at 14 58 08

Summary

We are talking about different vaadin-radio-group DOM elements for each dialog (see the id which is different).
But some state is apparently not reset which leads to this issue.

Especially, the checked attribute on the first radio button is preserved, although it shouldn't be.

@web-padawan web-padawan assigned yuriy-fix and unassigned web-padawan Oct 8, 2020
@yuriy-fix
Copy link
Contributor

yuriy-fix commented Oct 19, 2020

Wasn't able to reproduce the issue with the example from Denis.

I have done some investigation as well. I have created a minimal reproducible example for the original issue created by user. Can be tested on skeleton-starter or any other project.

MainView.java

import com.vaadin.flow.component.dialog.Dialog;
import com.vaadin.flow.component.html.NativeButton;
import com.vaadin.flow.component.orderedlayout.VerticalLayout;
import com.vaadin.flow.component.select.Select;
import com.vaadin.flow.router.Route;

@Route("test-issue")
public class MainView extends VerticalLayout {
    private final MainForm mainForm;

    public MainView() {
        this.mainForm = new MainForm();

        Select<String> select = new Select<>();
        select.setItems("A", "B", "C");

        NativeButton bt = new NativeButton("Open Dialog", e -> openDialog(select.getValue()));

        add(select, bt);
    }

    private void openDialog(String val) {
        mainForm.setRBGValue(val);
        new Dialog(mainForm).open();
    }
}

MainForm.java

import com.vaadin.flow.component.Tag;
import com.vaadin.flow.component.dependency.JsModule;
import com.vaadin.flow.component.polymertemplate.Id;
import com.vaadin.flow.component.polymertemplate.PolymerTemplate;
import com.vaadin.flow.component.radiobutton.RadioButtonGroup;
import com.vaadin.flow.templatemodel.TemplateModel;

@JsModule("./src/main-form.js")
@Tag("main-form")
public class MainForm extends PolymerTemplate<TemplateModel> {
    @Id("kind")
    RadioButtonGroup<String> kind;

    public MainForm() {
        kind.setItems("A", "B", "C");
    }

    public void setRBGValue(String val) {
        // Workaround here
        // kind.setItems("A", "B", "C");
        kind.setValue(val);
    }
}

frontend/src/main-form.js

import {html, PolymerElement} from '@polymer/polymer/polymer-element.js';
import '@vaadin/vaadin-radio-button/src/vaadin-radio-group.js';

class MainForm extends PolymerElement {
    //language=html
    static get template() {
        return html`            
            <vaadin-radio-group id="kind"></vaadin-radio-group>
        `;
    }

    static get is() {
        return 'main-form';
    }
}

customElements.define(MainForm.is, MainForm);

Steps to reproduce

  1. Select value in Select and open the dialog by clicking Open Dialog
  2. Close the dialog by outside click
  3. Select another value
  4. Open the dialog again (previous value is selected)

How it looks
dialog

Explanation
The issue is coming from the combination of using Dialog, PolymerTemplate and RadioButtonGroup. Whenever the Dialog is opened in this example, it leads to reattaching the existing instances of the vaadin-radio-buttons (that was created with .setItems()) to the RadioButtonGroup specified in the PolymerTemplate. (This is not happening when using only web-components). That triggers the FlattenedNodesObserver on client-side to apply the value based on checked state of the attached vaadin-radio-buttons which resets the value to firstly selected value.

Workaround
A workaround that can be used for this issue is to reset the items on the RadioButtonGroup before changing the value in order to reset the checked state of existing instances. In the example above I have commented it with the line // Workaround here.

Fix
The proper fix could possibly land in PolymerTemplate / @Id.

@tomivirkki
Copy link
Member

This seems to happen because RBG automatically adds RadioButtons inside of itself here. Now, if the RBG's value gets changed (setValue) while the component isn't in the DOM, the internal RB's remain unchanged. And once they're added back to DOM, Flow synchronizes the old RB values which causes the RBG to revert back to the old value. To fix this, you can for example override setValue in RBG and update the RB values respectively:

@Override
public void setValue(T value) {
  super.setValue(value);
  getRadioButtons().forEach(rb -> rb.setChecked(rb.getItem().equals(value)));
}

...but please take a closer look if there's a cleaner fix available (maybe just calling reset() on detach/attach??).

Note that Dialog isn't related to the issue. You can also just add/remove the PolymerTemplate to reproduce the issue.

yuriy-fix added a commit to vaadin/flow-components that referenced this issue Oct 19, 2020
Web-component: radio-button

Fixes: vaadin/vaadin-radio-button#166

Details: RBG automatically adds RadioButtons inside of itself. If the RBG's value gets changed (setValue) while the component isn't in the DOM, the internal RB's remain unchanged. And once they're added back to DOM, Flow synchronizes the old RB values which causes the RBG to revert back to the old value. In order to fix it buttons will be reset on detach.
yuriy-fix added a commit to vaadin/flow-components that referenced this issue Oct 20, 2020
Web-component: radio-button

Fixes: vaadin/vaadin-radio-button#166

Details: RBG automatically adds RadioButtons inside of itself. If the RBG's value gets changed (setValue) while the component isn't in the DOM, the internal RB's remain unchanged. And once they're added back to DOM, Flow synchronizes the old RB values which causes the RBG to revert back to the old value. In order to fix it buttons will be reset on detach.
yuriy-fix added a commit to vaadin/flow-components that referenced this issue Oct 22, 2020
Web-component: radio-button

Fixes: vaadin/vaadin-radio-button#166

Details: RBG automatically adds RadioButtons inside of itself. If the RBG's value gets changed (setValue) while the component isn't in the DOM, the internal RB's remain unchanged. And once they're added back to DOM, Flow synchronizes the old RB values which causes the RBG to revert back to the old value. In order to fix it buttons will be reset on detach.
yuriy-fix added a commit to vaadin/flow-components that referenced this issue Oct 22, 2020
Web-component: radio-button

Fixes: vaadin/vaadin-radio-button#166

Details: RBG automatically adds RadioButtons inside of itself. If the RBG's value gets changed (setValue) while the component isn't in the DOM, the internal RB's remain unchanged. And once they're added back to DOM, Flow synchronizes the old RB values which causes the RBG to revert back to the old value. In order to fix it buttons will be reset on detach.

* Override setValue

* Add test for attach-detach
tomivirkki pushed a commit to vaadin/flow-components that referenced this issue Oct 26, 2020
Web-component: radio-button

Fixes: vaadin/vaadin-radio-button#166

Details: RBG automatically adds RadioButtons inside of itself. If the RBG's value gets changed (setValue) while the component isn't in the DOM, the internal RB's remain unchanged. And once they're added back to DOM, Flow synchronizes the old RB values which causes the RBG to revert back to the old value. In order to fix it buttons will be reset on detach.

* Override setValue

* Add test for attach-detach
tomivirkki pushed a commit to vaadin/flow-components that referenced this issue Oct 26, 2020
Web-component: radio-button

Fixes: vaadin/vaadin-radio-button#166

Details: RBG automatically adds RadioButtons inside of itself. If the RBG's value gets changed (setValue) while the component isn't in the DOM, the internal RB's remain unchanged. And once they're added back to DOM, Flow synchronizes the old RB values which causes the RBG to revert back to the old value. In order to fix it buttons will be reset on detach.

* Override setValue

* Add test for attach-detach
tomivirkki pushed a commit to vaadin/flow-components that referenced this issue Oct 26, 2020
Web-component: radio-button

Fixes: vaadin/vaadin-radio-button#166

Details: RBG automatically adds RadioButtons inside of itself. If the RBG's value gets changed (setValue) while the component isn't in the DOM, the internal RB's remain unchanged. And once they're added back to DOM, Flow synchronizes the old RB values which causes the RBG to revert back to the old value. In order to fix it buttons will be reset on detach.

* Override setValue

* Add test for attach-detach
tomivirkki added a commit to vaadin/flow-components that referenced this issue Oct 26, 2020
* fix: override setValue to set proper value when detached (#275)

Web-component: radio-button

Fixes: vaadin/vaadin-radio-button#166

Details: RBG automatically adds RadioButtons inside of itself. If the RBG's value gets changed (setValue) while the component isn't in the DOM, the internal RB's remain unchanged. And once they're added back to DOM, Flow synchronizes the old RB values which causes the RBG to revert back to the old value. In order to fix it buttons will be reset on detach.

* Override setValue

* Add test for attach-detach

* Update setItems

Co-authored-by: Yuriy Yevstihnyeyev <yuriy@vaadin.com>
tomivirkki added a commit to vaadin/flow-components that referenced this issue Oct 26, 2020
* fix: override setValue to set proper value when detached (#275)

Web-component: radio-button

Fixes: vaadin/vaadin-radio-button#166

Details: RBG automatically adds RadioButtons inside of itself. If the RBG's value gets changed (setValue) while the component isn't in the DOM, the internal RB's remain unchanged. And once they're added back to DOM, Flow synchronizes the old RB values which causes the RBG to revert back to the old value. In order to fix it buttons will be reset on detach.

* Override setValue

* Add test for attach-detach

* Update setItems

Co-authored-by: Yuriy Yevstihnyeyev <yuriy@vaadin.com>
tomivirkki added a commit to vaadin/flow-components that referenced this issue Oct 26, 2020
* fix: override setValue to set proper value when detached (#275)

Web-component: radio-button

Fixes: vaadin/vaadin-radio-button#166

Details: RBG automatically adds RadioButtons inside of itself. If the RBG's value gets changed (setValue) while the component isn't in the DOM, the internal RB's remain unchanged. And once they're added back to DOM, Flow synchronizes the old RB values which causes the RBG to revert back to the old value. In order to fix it buttons will be reset on detach.

* Override setValue

* Add test for attach-detach

* Update setItems

Co-authored-by: Yuriy Yevstihnyeyev <yuriy@vaadin.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BFP Bug fix prioritised by a customer waiting for author Further information is requested
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants