Skip to content

Commit 7a1c46f

Browse files
fix: do not delay auto-adding Popover by beforeClientResponse (#7563)
* fix: reattach auto-added Popover If a popover tries to auto-add itself when another modal element is opened, it may end up being added as a child of said element. That can be problematic in case the target element is not part of the parent modal element and, when the modal is closed, the popover instance is removed with its parent and interacting with the target element won't work as expected. This fix adds a detach listener to popover and tries to reattach it in case of: - the popover instance has been auto-added - the target element is present - the target element is still attached to the page Fixes #7505 * test: add a test scenario for the use-case being fixed * refactor: auto-add popover to target parent * test: wait for dialog element to be removed --------- Co-authored-by: Sascha Ißbrücker <sissbruecker@vaadin.com>
1 parent 8173a24 commit 7a1c46f

File tree

4 files changed

+58
-20
lines changed
  • vaadin-popover-flow-parent
    • vaadin-popover-flow/src/main/java/com/vaadin/flow/component/popover
    • vaadin-popover-flow-integration-tests

4 files changed

+58
-20
lines changed

vaadin-popover-flow-parent/vaadin-popover-flow-integration-tests/pom.xml

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,11 @@
4949
<groupId>com.vaadin</groupId>
5050
<artifactId>vaadin-dev-server</artifactId>
5151
</dependency>
52+
<dependency>
53+
<groupId>com.vaadin</groupId>
54+
<artifactId>vaadin-dialog-flow</artifactId>
55+
<version>${project.version}</version>
56+
</dependency>
5257
<dependency>
5358
<groupId>com.vaadin</groupId>
5459
<artifactId>vaadin-flow-components-test-util</artifactId>

vaadin-popover-flow-parent/vaadin-popover-flow-integration-tests/src/main/java/com/vaadin/flow/component/popover/tests/PopoverView.java

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
*/
1616
package com.vaadin.flow.component.popover.tests;
1717

18+
import com.vaadin.flow.component.dialog.Dialog;
1819
import com.vaadin.flow.component.html.Div;
1920
import com.vaadin.flow.component.html.NativeButton;
2021
import com.vaadin.flow.component.popover.Popover;
@@ -54,7 +55,22 @@ public PopoverView() {
5455
event -> popover.setCloseOnOutsideClick(false));
5556
disableCloseOnOutsideClick.setId("disable-close-on-outside-click");
5657

58+
NativeButton openDialog = new NativeButton("Open dialog and set target",
59+
event -> {
60+
var dialog = new Dialog();
61+
var close = new NativeButton("Close", e -> dialog.close());
62+
close.setId("close-dialog");
63+
dialog.add(close);
64+
dialog.open();
65+
popover.setTarget(target);
66+
});
67+
openDialog.setId("open-dialog");
68+
69+
NativeButton removePopover = new NativeButton("Remove popover",
70+
event -> remove(popover));
71+
removePopover.setId("remove-popover");
72+
5773
add(popover, clearTarget, detachTarget, attachTarget, disableCloseOnEsc,
58-
disableCloseOnOutsideClick, target);
74+
disableCloseOnOutsideClick, removePopover, openDialog, target);
5975
}
6076
}

vaadin-popover-flow-parent/vaadin-popover-flow-integration-tests/src/test/java/com/vaadin/flow/component/popover/tests/PopoverIT.java

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -135,6 +135,22 @@ public void disableCloseOnEsc_pressEsc_popoverDoesNotClose() {
135135
Assert.assertTrue(popover.isOpen());
136136
}
137137

138+
@Test
139+
public void openDialogAndSetTarget_closeDialog_popoverOpens() {
140+
// Clear the target and remove the popover first to ensure the popover
141+
// is auto-added when the dialog opens
142+
clickElementWithJs("clear-target");
143+
clickElementWithJs("remove-popover");
144+
145+
clickElementWithJs("open-dialog");
146+
clickElementWithJs("close-dialog");
147+
148+
waitForElementNotPresent(By.tagName("vaadin-dialog-overlay"));
149+
150+
clickTarget();
151+
checkPopoverIsOpened();
152+
}
153+
138154
private void clickTarget() {
139155
clickElementWithJs("popover-target");
140156
}

vaadin-popover-flow-parent/vaadin-popover-flow/src/main/java/com/vaadin/flow/component/popover/Popover.java

Lines changed: 20 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -696,10 +696,7 @@ public void setTarget(Component target) {
696696
targetDetachRegistration.remove();
697697
}
698698

699-
if (autoAddedToTheUi) {
700-
getElement().removeFromParent();
701-
autoAddedToTheUi = false;
702-
}
699+
removeFromUiIfAutoAdded();
703700

704701
this.target = target;
705702

@@ -710,27 +707,31 @@ public void setTarget(Component target) {
710707

711708
// Target's JavaScript needs to be executed on each attach,
712709
// because Flow creates a new client-side element
713-
target.getUI().ifPresent(this::onTargetAttach);
710+
if (target.getUI().isPresent()) {
711+
onTargetAttach();
712+
}
714713
targetAttachRegistration = target
715-
.addAttachListener(e -> onTargetAttach(e.getUI()));
714+
.addAttachListener(e -> onTargetAttach());
716715
targetDetachRegistration = target.addDetachListener(e -> {
717-
if (autoAddedToTheUi) {
718-
getElement().removeFromParent();
719-
autoAddedToTheUi = false;
720-
}
716+
removeFromUiIfAutoAdded();
721717
});
722718
}
723719

724-
private void onTargetAttach(UI ui) {
720+
private void removeFromUiIfAutoAdded() {
721+
if (autoAddedToTheUi) {
722+
autoAddedToTheUi = false;
723+
getElement().removeFromParent();
724+
}
725+
}
726+
727+
private void onTargetAttach() {
725728
if (target != null) {
726-
ui.beforeClientResponse(ui, context -> {
727-
if (getElement().getNode().getParent() == null) {
728-
// Remove the popover from its current state tree
729-
getElement().removeFromTree(false);
730-
ui.addToModalComponent(this);
731-
autoAddedToTheUi = true;
732-
}
733-
});
729+
if (getElement().getNode().getParent() == null) {
730+
// Remove the popover from its current state tree
731+
getElement().removeFromTree(false);
732+
target.getElement().getParent().appendChild(getElement());
733+
autoAddedToTheUi = true;
734+
}
734735
getElement().executeJs("this.target = $0", target.getElement());
735736
}
736737
}

0 commit comments

Comments
 (0)