fix: do not delay auto-adding Popover by beforeClientResponse
#7563
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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
DiegoCardoso
commented
Jun 3, 2025
Comment on lines
139
to
150
| public void openDialogAndSetTarget_closeDialog_popoverOpens() { | ||
| // Clear the target and remove the popover first to ensure the popover | ||
| // is auto-added when the dialog opens | ||
| clickElementWithJs("clear-target"); | ||
| clickElementWithJs("remove-popover"); | ||
|
|
||
| clickElementWithJs("open-dialog"); | ||
| clickElementWithJs("close-dialog"); | ||
|
|
||
| clickTarget(); | ||
| checkPopoverIsOpened(); | ||
| } |
There was a problem hiding this comment.
I tried to avoid adding an IT and use a unit test instead, but with no luck. I tried something like:
@Test
public void targetAddedToPage_modalElementOpenedAndClosed_popoverAutoAddedToPage() {
Popover popover = new Popover();
Div target = new Div();
popover.setTarget(target);
ui.add(target);
Div modalElement = new Div();
ui.setChildComponentModal(modalElement, true);
fakeClientResponse();
Assert.assertEquals(modalElement.getElement(),
popover.getElement().getParent());
ui.remove(modalElement);
modalElement.getElement().removeAllChildren();
modalElement.getElement().removeFromTree();
// popover.getElement().removeFromTree();
fakeClientResponse();
Assert.assertEquals(ui.getElement(), popover.getElement().getParent());
}But the detach listener in the Popover class wasn't being called.
sissbruecker
reviewed
Jun 3, 2025
...flow-parent/vaadin-popover-flow/src/main/java/com/vaadin/flow/component/popover/Popover.java
Outdated
Show resolved
Hide resolved
...-flow-integration-tests/src/test/java/com/vaadin/flow/component/popover/tests/PopoverIT.java
Show resolved
Hide resolved
DiegoCardoso
changed the title
DiegoCardoso
changed the title
beforeClientResponse
sissbruecker
approved these changes
Jun 5, 2025
|
vaadin-bot
pushed a commit
that referenced
this pull request
Jun 5, 2025
* 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 <[email protected]>
vaadin-bot
pushed a commit
that referenced
this pull request
Jun 5, 2025
* 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 <[email protected]>
DiegoCardoso
added a commit
that referenced
this pull request
Jun 5, 2025
… (#7579) * 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: Diego Cardoso <[email protected]> Co-authored-by: Sascha Ißbrücker <[email protected]>
DiegoCardoso
added a commit
that referenced
this pull request
Jun 5, 2025
… (#7578) * 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: Diego Cardoso <[email protected]> Co-authored-by: Sascha Ißbrücker <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
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:
Fixes #7505
Type of change