Skip to content

Commit 72bbf00

Browse files
authored
fix: prevent infinite focus loop and stale selection in custom editors (CP: 25.1) (#9120)
This PR cherry-picks changes from the original PR #9113 to branch 25.1. --- > ## Summary > - Fix infinite `cellSelected` feedback loop triggered by programmatic focus changes (e.g. `inputElement.select()`) in `onCustomEditorDisplayed` callbacks > - Prevent delayed server responses from hijacking the cell selection when the user has already navigated away, including via keyboard navigation through cells with custom editors > - Add server-side reentrancy guard to prevent recursive `onCustomEditorDisplayed` calls when callback code calls `refreshCells()` > > ## Root cause > `CustomEditorEventListener.ONFOCUS` unconditionally sent `cellSelected` to the server and updated the client-side selection on every focus event — including programmatic focus from `inputElement.select()` called in `onCustomEditorDisplayed`. When the callback had non-trivial processing time, the delayed response would steal focus and selection from whatever cell the user had since navigated to, and could trigger an infinite client→server→client loop. > > ## Approach > **Client-side** (`CustomEditorEventListener`): track `mousedown`/`touchstart` events to distinguish user-initiated focus from programmatic focus. Only update the selection and send `cellSelected` for user-initiated focus. For programmatic focus on a cell the user has already left, return focus to the sheet. Keyboard navigation is unaffected because `SelectionHandler` already handles selection updates and `cellSelected` independently. > > **Server-side** (`Spreadsheet`): reentrancy guard (`insideCustomEditorCallback`) on `loadCustomEditorOnSelectedCell()` to prevent recursive `onCustomEditorDisplayed` calls when user code in the callback calls `refreshCells()`. > > Fixes #9036 > > ## Test plan > - [ ] `CustomEditorIT#comboBoxEditorWithSelect_clickCell_callbackFiredOnce` — verifies single click triggers exactly 1 callback (no loop) > - [ ] `CustomEditorIT#comboBoxEditorWithSelect_switchBetweenCells_noInfiniteLoop` — rapidly clicks through editor cells to a non-editor cell, verifies selection stays on the final cell and callback count is bounded > - [ ] All 26 existing `CustomEditorIT` tests pass (keyboard navigation, Tab, Shift+Tab, F2, Enter, ESC, shared editors, frozen panes, always-visible mode) > > 🤖 Generated with [Claude Code](https://claude.com/claude-code)
1 parent 5158862 commit 72bbf00

5 files changed

Lines changed: 201 additions & 14 deletions

File tree

vaadin-spreadsheet-flow-parent/vaadin-spreadsheet-flow-client/src/main/java/com/vaadin/addon/spreadsheet/client/CustomEditorEventListener.java

Lines changed: 37 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -30,13 +30,14 @@ public class CustomEditorEventListener implements EventListener {
3030
private Slot slot;
3131
private String cellAddress;
3232
private SpreadsheetWidget widget;
33+
private boolean userInitiatedFocus;
3334

3435
public void init(Slot slot, String cellAddress) {
3536
this.slot = slot;
3637
this.cellAddress = cellAddress;
3738
Event.setEventListener(slot.getAssignedElement(), this);
38-
DOM.sinkEvents(slot.getAssignedElement(),
39-
Event.ONKEYDOWN | Event.FOCUSEVENTS);
39+
DOM.sinkEvents(slot.getAssignedElement(), Event.ONKEYDOWN
40+
| Event.FOCUSEVENTS | Event.ONMOUSEDOWN | Event.ONTOUCHSTART);
4041
}
4142

4243
public void setCellAddress(String cellAddress) {
@@ -63,20 +64,45 @@ public void onBrowserEvent(Event event) {
6364
break;
6465
}
6566
break;
67+
case Event.ONMOUSEDOWN:
68+
case Event.ONTOUCHSTART:
69+
userInitiatedFocus = true;
70+
break;
6671
case Event.ONFOCUS:
67-
var jsniUtil = getSheetWidget().getSheetJsniUtil();
68-
jsniUtil.parseColRow(cellAddress);
69-
var col = jsniUtil.getParsedCol();
70-
var row = jsniUtil.getParsedRow();
71-
getSheetWidget().setSelectedCell(col, row);
72-
getSheetWidget().updateSelectionOutline(col, col, row, row);
73-
getSheetWidget().updateSelectedCellStyles(col, col, row, row, true);
74-
getSpreadsheetWidget().getSpreadsheetHandler().cellSelected(row,
75-
col, true);
7672
slot.setElementFocused(true);
73+
// Only update selection and notify the server if this focus was
74+
// triggered by a user interaction (mouse/touch). Programmatic
75+
// focus changes (e.g. inputElement.select() from
76+
// onCustomEditorDisplayed) must not update the selection or send
77+
// cellSelected — doing so would move the selection to a stale
78+
// cell when the delayed server response arrives, and could
79+
// create an infinite feedback loop between client and server.
80+
// Keyboard navigation already handles selection updates and
81+
// cellSelected through SelectionHandler independently.
82+
if (userInitiatedFocus) {
83+
userInitiatedFocus = false;
84+
var jsniUtil = getSheetWidget().getSheetJsniUtil();
85+
jsniUtil.parseColRow(cellAddress);
86+
var col = jsniUtil.getParsedCol();
87+
var row = jsniUtil.getParsedRow();
88+
getSheetWidget().setSelectedCell(col, row);
89+
getSheetWidget().updateSelectionOutline(col, col, row, row);
90+
getSheetWidget().updateSelectedCellStyles(col, col, row, row,
91+
true);
92+
getSpreadsheetWidget().getSpreadsheetHandler().cellSelected(row,
93+
col, true);
94+
} else if (!cellAddress
95+
.equals(getSheetWidget().getSelectedCellKey())) {
96+
// Programmatic focus (e.g. inputElement.select() from a
97+
// delayed server response) on a cell the user has already
98+
// left. Return focus to the sheet so keyboard input isn't
99+
// captured by a stale editor.
100+
getSheetWidget().focusSheet();
101+
}
77102
break;
78103
case Event.ONBLUR:
79104
slot.setElementFocused(false);
105+
userInitiatedFocus = false;
80106
break;
81107
}
82108
}
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,102 @@
1+
/**
2+
* Copyright 2000-2026 Vaadin Ltd.
3+
*
4+
* This program is available under Vaadin Commercial License and Service Terms.
5+
*
6+
* See {@literal <https://vaadin.com/commercial-license-and-service-terms>} for the full
7+
* license.
8+
*/
9+
package com.vaadin.flow.component.spreadsheet.tests.fixtures;
10+
11+
import org.apache.poi.ss.usermodel.Cell;
12+
import org.apache.poi.ss.usermodel.Sheet;
13+
14+
import com.vaadin.flow.component.Component;
15+
import com.vaadin.flow.component.combobox.ComboBox;
16+
import com.vaadin.flow.component.html.Span;
17+
import com.vaadin.flow.component.spreadsheet.Spreadsheet;
18+
import com.vaadin.flow.component.spreadsheet.SpreadsheetComponentFactory;
19+
20+
/**
21+
* Test fixture for verifying that custom editors calling
22+
* {@code inputElement.select()} in {@code onCustomEditorDisplayed} do not cause
23+
* an infinite focus loop between client and server.
24+
* <p>
25+
* Exposes a callback counter as a custom component in cell A1 (id
26+
* "callbackCount") so tests can verify the number of
27+
* {@code onCustomEditorDisplayed} invocations.
28+
*/
29+
public class CustomEditorSelectFixture implements SpreadsheetFixture {
30+
31+
@Override
32+
public void loadFixture(Spreadsheet spreadsheet) {
33+
spreadsheet.setColumnWidth(1, 150);
34+
spreadsheet.setColumnWidth(2, 150);
35+
spreadsheet.setColumnWidth(3, 150);
36+
spreadsheet.setColumnWidth(4, 150);
37+
38+
spreadsheet.setSpreadsheetComponentFactory(new SelectEditorFactory());
39+
}
40+
41+
private static class SelectEditorFactory
42+
implements SpreadsheetComponentFactory {
43+
44+
private static final String[] FRUITS = { "Apple", "Banana", "Cherry" };
45+
46+
private int callbackCount;
47+
private Span counterLabel;
48+
49+
@Override
50+
public Component getCustomComponentForCell(Cell cell, int rowIndex,
51+
int columnIndex, Spreadsheet spreadsheet, Sheet sheet) {
52+
if (rowIndex == 0 && columnIndex == 0) {
53+
if (counterLabel == null) {
54+
counterLabel = new Span("0");
55+
counterLabel.setId("callbackCount");
56+
}
57+
return counterLabel;
58+
}
59+
return null;
60+
}
61+
62+
@Override
63+
public Component getCustomEditorForCell(Cell cell, final int rowIndex,
64+
final int columnIndex, final Spreadsheet spreadsheet,
65+
Sheet sheet) {
66+
if (rowIndex == 1 && columnIndex >= 1 && columnIndex <= 4) {
67+
ComboBox<String> comboBox = new ComboBox<>();
68+
comboBox.setItems(FRUITS);
69+
comboBox.setWidthFull();
70+
return comboBox;
71+
}
72+
return null;
73+
}
74+
75+
@Override
76+
public void onCustomEditorDisplayed(Cell cell, int rowIndex,
77+
int columnIndex, Spreadsheet spreadsheet, Sheet sheet,
78+
Component customEditor) {
79+
callbackCount++;
80+
if (counterLabel != null) {
81+
counterLabel.setText(String.valueOf(callbackCount));
82+
}
83+
84+
if (customEditor instanceof ComboBox<?>) {
85+
try {
86+
// Simulate server-side processing time (DB lookups,
87+
// business logic). This delay causes the client to queue
88+
// subsequent cellSelected RPCs, reproducing the scenario
89+
// where the user navigates away before the response
90+
// arrives.
91+
Thread.sleep(200);
92+
} catch (InterruptedException e) {
93+
Thread.currentThread().interrupt();
94+
}
95+
@SuppressWarnings("unchecked")
96+
ComboBox<String> comboBox = (ComboBox<String>) customEditor;
97+
comboBox.setValue(FRUITS[columnIndex % FRUITS.length]);
98+
comboBox.getElement().executeJs("this.inputElement.select();");
99+
}
100+
}
101+
}
102+
}

vaadin-spreadsheet-flow-parent/vaadin-spreadsheet-flow-integration-tests/src/main/java/com/vaadin/flow/component/spreadsheet/tests/fixtures/TestFixtures.java

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
* Test fixtures for server-side Spreadsheet manipulation
1313
*
1414
*/
15+
@SuppressWarnings("java:S115") // Enum constants use PascalCase by convention
1516
public enum TestFixtures {
1617
FirstColumnWidth(FirstColumnWidthFixture.class),
1718
PopupButton(PopupButtonFixture.class),
@@ -39,6 +40,7 @@ public enum TestFixtures {
3940
AdjacentCustomEditors(AdjacentCustomEditorsFixture.class),
4041
CustomEditorShared(CustomEditorSharedFixture.class),
4142
CustomEditorRow(CustomEditorRowFixture.class),
43+
CustomEditorSelect(CustomEditorSelectFixture.class),
4244
Styles(StylesFixture.class),
4345
LockCell(LockCellFixture.class),
4446
LockSheet(LockSheetFixture.class),

vaadin-spreadsheet-flow-parent/vaadin-spreadsheet-flow-integration-tests/src/test/java/com/vaadin/flow/component/spreadsheet/test/CustomEditorIT.java

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -572,6 +572,45 @@ private WebElement getActiveElement() {
572572
return getDriver().switchTo().activeElement();
573573
}
574574

575+
@Test
576+
public void comboBoxEditorWithSelect_clickCell_callbackFiredOnce() {
577+
createNewSpreadsheet();
578+
loadTestFixture(TestFixtures.CustomEditorSelect);
579+
580+
clickCell("B2");
581+
getCommandExecutor().waitForVaadin();
582+
583+
Assert.assertEquals("1", getCallbackCount());
584+
}
585+
586+
@Test
587+
public void comboBoxEditorWithSelect_switchBetweenCells_noInfiniteLoop() {
588+
createNewSpreadsheet();
589+
loadTestFixture(TestFixtures.CustomEditorSelect);
590+
591+
// Click rapidly through editor cells and stop at a non-editor cell.
592+
// The server-side callback has a 200ms delay, so responses arrive
593+
// after the user has already moved past those cells.
594+
clickCell("B2");
595+
clickCell("C2");
596+
clickCell("F2");
597+
getCommandExecutor().waitForVaadin();
598+
599+
// Selection must stay on the cell where the user stopped, not jump
600+
// back to a stale editor cell when delayed responses arrive.
601+
assertAddressFieldValue("F2");
602+
603+
// Each editor cell visited should trigger exactly one callback.
604+
// Without the fix, this would cause an infinite loop.
605+
int count = Integer.parseInt(getCallbackCount());
606+
Assert.assertTrue("Expected at most 2 callbacks but got " + count
607+
+ " — possible infinite loop", count <= 2);
608+
}
609+
610+
private String getCallbackCount() {
611+
return $(TestBenchElement.class).id("callbackCount").getText();
612+
}
613+
575614
private void clickToggleCellVisibleButton() {
576615
waitUntil(driver -> {
577616
try {

vaadin-spreadsheet-flow-parent/vaadin-spreadsheet-flow/src/main/java/com/vaadin/flow/component/spreadsheet/Spreadsheet.java

Lines changed: 21 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -253,6 +253,8 @@ public void setId(String id) {
253253

254254
private Registration spreadsheetHandlerRegistration;
255255

256+
private boolean insideCustomEditorCallback;
257+
256258
int getCols() {
257259
return cols;
258260
}
@@ -3748,6 +3750,13 @@ private void markAsDirty() {
37483750
* cell.
37493751
*/
37503752
protected void loadCustomEditorOnSelectedCell() {
3753+
// Guard against reentrancy: if user code in onCustomEditorDisplayed
3754+
// calls refreshCells() -> updateMarkedCells() ->
3755+
// reloadVisibleCellContents() -> loadCells() ->
3756+
// loadCustomEditorOnSelectedCell(), skip the recursive call.
3757+
if (insideCustomEditorCallback) {
3758+
return;
3759+
}
37513760
CellReference selectedCellReference = selectionManager
37523761
.getSelectedCellReference();
37533762
if (selectedCellReference != null && customComponentFactory != null) {
@@ -3761,9 +3770,18 @@ protected void loadCustomEditorOnSelectedCell() {
37613770
String componentId = currentCellKeysToEditorIdMap.get(key);
37623771
for (Component c : customComponents) {
37633772
if (getComponentNodeId(c).equals(componentId)) {
3764-
customComponentFactory.onCustomEditorDisplayed(
3765-
getCell(row, col), row, col, this,
3766-
getActiveSheet(), c);
3773+
insideCustomEditorCallback = true;
3774+
try {
3775+
customComponentFactory.onCustomEditorDisplayed(
3776+
getCell(row, col), row, col, this,
3777+
getActiveSheet(), c);
3778+
} catch (Exception e) {
3779+
LOGGER.warn(
3780+
"Error in onCustomEditorDisplayed for cell ({}, {})",
3781+
col + 1, row + 1, e);
3782+
} finally {
3783+
insideCustomEditorCallback = false;
3784+
}
37673785
return;
37683786
}
37693787
}

0 commit comments

Comments
 (0)