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.

Sign up for GitHub

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

Jump to bottom

Dialog is not updated when doing hotswap #20473

Open
Artur- opened this issue Nov 14, 2024 · 3 comments · May be fixed by #20540
Open

Dialog is not updated when doing hotswap #20473

Artur- opened this issue Nov 14, 2024 · 3 comments · May be fixed by #20540
Assignees
@mcollovati
Labels
bug Impact: High live reload Severity: Major

Comments

@Artur-
Copy link
Member

Artur- commented Nov 14, 2024

Description of the bug

Given

@Route(value = "overlay", layout = MainLayout.class)
@AnonymousAllowed
public class OverlayView extends VerticalLayout {

    @Override
    protected void onAttach(AttachEvent attachEvent) {
        super.onAttach(attachEvent);

        Dialog dialog = new Dialog("The Dialog");
        dialog.setCloseOnOutsideClick(true);
        dialog.setCloseOnEsc(true);

        dialog.add(new Div("Text nr 1"));

        dialog.open();
    }

}

if you are running using HotswapAgent and edit the div text to "Text nr 2", then a dialog with "Text nr 1" remains on screen and a dialog with "Text nr 2" is not added at all. If you manually refresh the page, the dialog with "Text nr 2" is shown

Expected behavior

The old dialog is closed and the new one opened

Minimal reproducible example

as above

Versions

  • Vaadin / Flow version: 24.5.3
This was referenced Nov 14, 2024
Open
Open
@mshabarov mshabarov moved this to 🪵Product backlog in Vaadin Flow ongoing work (Vaadin 10+) Nov 19, 2024
@mshabarov mshabarov moved this to 🔖 Normal Priority (P2) in Vaadin Flow bugs & maintenance (Vaadin 10+) Nov 19, 2024
@mcollovati
Copy link
Collaborator

Refresh seems to work if the dialog is added to the route component.

add(dialog);
dialog.open();

If not added explicitly to another component, the dialog has UI as a parent, so it is not detached during refresh.
On refresh, we could probably remove modal components, but this might not be correct in all situations.
For example, if the dialog is added by a layout, and we request a refresh for the route component only, the dialog would not be re-opened, since the layout is not detached and attached.

@mcollovati
Copy link
Collaborator

In the case of route chain refresh, if dialogs attached to the UI are forcibly removed, they are not re-added.
Dialog class registers an afterNavigationListener that for programmatic navigation removes the beforeClientResponse callback that should do the attach.

https://github.com/vaadin/flow-components/blob/985c10c20d40bb706326d401bed7be50a45de4eb/vaadin-dialog-flow-parent/vaadin-dialog-flow/src/main/java/com/vaadin/flow/component/dialog/Dialog.java#L899-L925

@mcollovati mcollovati self-assigned this Nov 25, 2024
mcollovati added a commit that referenced this issue Nov 25, 2024
@mcollovati
fix: remove modal component on route refresh
bc0379c 
Modal components attached to the UI were not removed or replaced during
self-navigation triggered by a route refresh.
This change updates navigation handler to ensure modal components are
removed and adds a new navigation trigger for route refresh to differentiate
programmatic navigations (e.g., forward actions).
It also modifies Hotswapper to require a full chain refresh when modal
components are present.

Fixes #20473
@mcollovati mcollovati linked a pull request Nov 25, 2024 that will close this issue
Open
9 tasks
@mcollovati mcollovati moved this from ⚒️ In progress to 🔎Iteration reviews in Vaadin Flow ongoing work (Vaadin 10+) Nov 25, 2024
@sveinnetnordic
Copy link

sveinnetnordic commented Nov 25, 2024 •
edited
Loading

I am working with a big dialog an tough that form field was updated on the "new" hotswap way, but that did not happen.
What about having a way to set the way hotswap should work in Copilot in the browser (set at cookie etc). Mode: Full refresh/"new refresh". A -Dvaadin.hotswapmode=... would also be nice. I assume the "new hotswap" will be default.

EDIT: At the moment of saving this text a fix was created :-)

mcollovati added a commit that referenced this issue Nov 25, 2024
@mcollovati
fix: remove modal component on route refresh
4ef3417 
Modal components attached to the UI were not removed or replaced during
self-navigation triggered by a route refresh.
This change updates navigation handler to ensure modal components are
removed and adds a new navigation trigger for route refresh to differentiate
programmatic navigations (e.g., forward actions).
It also modifies Hotswapper to require a full chain refresh when modal
components are present.

Fixes #20473
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@mcollovati mcollovati

Labels
bug Impact: High live reload Severity: Major
Projects
Vaadin Flow bugs & maintenance (Vaadin 10+)
Status: 🔖 Normal Priority (P2)
Vaadin Flow ongoing work (Vaadin 10+)
Status: 🔎Iteration reviews
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix: remove modal component on route refresh vaadin/flow
4 participants
@Artur- @mcollovati @mshabarov @sveinnetnordic