Skip to content

Commit 76da373

Browse files
vursenclaude
andauthored
fix: avoid NPE when popover target is set again during auto-add (#9912)
When `setTarget` is called with a target that is already attached, the popover is auto-added to the UI right away. The auto-add fires the popover's attach listeners, and if one of them calls `setTarget` again, that call fails with a `NullPointerException`. This happens because the first call assigns `targetAttachRegistration` and `targetDetachRegistration` only after the auto-add, so the nested call finds a non-null target but null registrations. `setTarget` now assigns both registrations before running the initial attach logic. Only adding null checks around the `remove()` calls would also avoid the exception, but it would leak listeners: the nested call would register its own listeners on the target, and the first call would then overwrite them without removing them. The leaked detach listener could later remove the popover from the UI even though its target had already changed. A separate test covers this by detaching the first target and checking that the popover stays in the UI. Fixes #7758 Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
1 parent cc15e58 commit 76da373

2 files changed

Lines changed: 38 additions & 1 deletion

File tree

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -747,12 +747,12 @@ public void setTarget(Component target) {
747747

748748
// Target's JavaScript needs to be executed on each attach,
749749
// because Flow creates a new client-side element
750-
target.getUI().ifPresent(this::onTargetAttach);
751750
targetAttachRegistration = target
752751
.addAttachListener(e -> onTargetAttach(e.getUI()));
753752
targetDetachRegistration = target.addDetachListener(e -> {
754753
removeFromUiIfAutoAdded();
755754
});
755+
target.getUI().ifPresent(this::onTargetAttach);
756756
}
757757

758758
private void removeFromUiIfAutoAdded() {

vaadin-popover-flow-parent/vaadin-popover-flow/src/test/java/com/vaadin/flow/component/popover/PopoverAutoAddTest.java

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -155,6 +155,43 @@ void setTarget_changeUI_autoAdded() {
155155
popover.getElement().getParent());
156156
}
157157

158+
@Test
159+
void setTarget_setAnotherTargetDuringAutoAdd_autoAdded() {
160+
Div target = new Div();
161+
Div other = new Div();
162+
ui.add(target);
163+
ui.add(other);
164+
165+
Popover popover = new Popover();
166+
popover.addAttachListener(event -> popover.setTarget(other));
167+
168+
popover.setTarget(target);
169+
170+
ui.fakeClientCommunication();
171+
Assertions.assertEquals(other, popover.getTarget());
172+
Assertions.assertEquals(ui.getUI().getElement(),
173+
popover.getElement().getParent());
174+
}
175+
176+
@Test
177+
void setTarget_setAnotherTargetDuringAutoAdd_detachFirstTarget_notAutoRemoved() {
178+
Div target = new Div();
179+
Div other = new Div();
180+
ui.add(target);
181+
ui.add(other);
182+
183+
Popover popover = new Popover();
184+
popover.addAttachListener(event -> popover.setTarget(other));
185+
186+
popover.setTarget(target);
187+
ui.fakeClientCommunication();
188+
189+
ui.remove(target);
190+
ui.fakeClientCommunication();
191+
Assertions.assertEquals(ui.getUI().getElement(),
192+
popover.getElement().getParent());
193+
}
194+
158195
@Test
159196
void setTarget_openModal_popoverIsAttachedToUi() {
160197
Div target = new Div();

0 commit comments

Comments
 (0)