Skip to content

Commit 179ac05

Browse files
vursenclaude
andauthored
fix: hide already-filtered values from other spreadsheet column filters (#9386)
## Description In a `SpreadsheetFilterTable` with filters on multiple columns, opening any column's filter dropdown showed values from rows already hidden by other columns as unchecked, suggesting they were filtered by this column. Re-checking them did nothing because another column still hid the row. Applying a second filter then implicitly added those already-hidden rows to the second column's `filteredRows`, so clearing the first filter did not bring the rows back — the second column now also filtered them. Root cause: `ItemFilter` read `spreadsheet.isRowHidden(r)` for both building option lists and deciding which rows belong in its own `filteredRows`. That property reflects the union of all column filters, so each filter could not tell its own work from a sibling's. Each `ItemFilter` now operates only on its own `filteredRows`. The dropdown option list omits values whose rows are all hidden by other columns via a new `SpreadsheetFilterTable.getRowsHiddenByOtherFilters(self)` helper. `clearAllFilters` does a second pass to refresh option lists so cleared siblings are reflected immediately. Fixes #9329 ## Reproduction ```java @route("spreadsheet-filter-cross-column") public class SpreadsheetFilterCrossColumnPage extends VerticalLayout { public SpreadsheetFilterCrossColumnPage() { Spreadsheet spreadsheet = new Spreadsheet(); spreadsheet.setTheme(SpreadsheetTheme.LUMO); spreadsheet.setHeight("400px"); spreadsheet.createCell(0, 0, "Column A"); spreadsheet.createCell(0, 1, "Column B"); spreadsheet.createCell(0, 2, "Column C"); spreadsheet.createCell(1, 0, "Alpha"); spreadsheet.createCell(1, 1, "Foo"); spreadsheet.createCell(1, 2, "Alice"); spreadsheet.createCell(2, 0, "Beta"); spreadsheet.createCell(2, 1, "Bar"); spreadsheet.createCell(2, 2, "Bob"); spreadsheet.createCell(3, 0, "Gamma"); spreadsheet.createCell(3, 1, "Baz"); spreadsheet.createCell(3, 2, "Carol"); CellRangeAddress range = new CellRangeAddress(0, 3, 0, 2); SpreadsheetFilterTable table = new SpreadsheetFilterTable(spreadsheet, range); spreadsheet.registerTable(table); spreadsheet.refreshAllCellValues(); add(spreadsheet); } } ``` | Column A | Column B | Column C | | -------- | -------- | -------- | | Alpha | Foo | Alice | | Beta | Bar | Bob | | Gamma | Baz | Carol | Steps to reproduce: - Filter `Beta` in column A. - Open the filter dropdown in column B → `Bar` shows up unchecked (before the fix), suggesting column B filtered it. - Filter `Foo` in column B. - Re-check `Beta` in column A → the row does not reappear (before the fix), because column B now also filters it. 🤖 Generated with Claude Code --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 8b77a3d commit 179ac05

5 files changed

Lines changed: 139 additions & 23 deletions

File tree

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

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,12 @@
4747
<artifactId>vaadin-checkbox-flow</artifactId>
4848
<version>${project.version}</version>
4949
</dependency>
50+
<dependency>
51+
<groupId>com.vaadin</groupId>
52+
<artifactId>vaadin-checkbox-testbench</artifactId>
53+
<version>${project.version}</version>
54+
<scope>test</scope>
55+
</dependency>
5056
<dependency>
5157
<groupId>com.vaadin</groupId>
5258
<artifactId>vaadin-combo-box-flow</artifactId>

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

Lines changed: 14 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -18,20 +18,23 @@ public class SpreadsheetTableFixture implements SpreadsheetFixture {
1818

1919
@Override
2020
public void loadFixture(Spreadsheet spreadsheet) {
21-
int maxColumns = 5;
22-
int maxRows = 5;
21+
int rows = 5; // 1 header row + 4 data rows
22+
int columns = 5;
23+
int firstRow = 1;
24+
int firstColumn = 1;
2325

24-
for (int column = 1; column < maxColumns + 1; column++) {
25-
spreadsheet.createCell(1, column, "Column " + column);
26-
}
27-
28-
for (int row = 2; row < maxRows + 2; row++) {
29-
for (int col = 1; col < maxColumns + 1; col++) {
30-
spreadsheet.createCell(row, col, row + col);
26+
// Label every cell with its position within the table, e.g. "Cell 0:0".
27+
// This keeps each value distinct and makes it obvious which row and
28+
// column a filter value belongs to.
29+
for (int row = 0; row < rows; row++) {
30+
for (int col = 0; col < columns; col++) {
31+
spreadsheet.createCell(firstRow + row, firstColumn + col,
32+
"Cell " + row + ":" + col);
3133
}
3234
}
33-
CellRangeAddress range = new CellRangeAddress(1, maxRows, 1,
34-
maxColumns);
35+
36+
CellRangeAddress range = new CellRangeAddress(firstRow,
37+
firstRow + rows - 1, firstColumn, firstColumn + columns - 1);
3538
SpreadsheetTable table = new SpreadsheetFilterTable(spreadsheet, range);
3639
spreadsheet.registerTable(table);
3740
spreadsheet.refreshAllCellValues();

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

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,10 +10,14 @@
1010

1111
import static org.junit.Assert.assertFalse;
1212

13+
import java.util.List;
14+
1315
import org.junit.Assert;
1416
import org.junit.Before;
1517
import org.junit.Test;
18+
import org.openqa.selenium.By;
1619

20+
import com.vaadin.flow.component.checkbox.testbench.CheckboxGroupElement;
1721
import com.vaadin.flow.component.spreadsheet.testbench.SheetCellElement;
1822
import com.vaadin.flow.component.spreadsheet.testbench.SpreadsheetElement;
1923
import com.vaadin.flow.component.spreadsheet.tests.fixtures.TestFixtures;
@@ -61,8 +65,68 @@ public void sheetWithFilterTable_rowIsRemoved_filterOptionsAvailable() {
6165
assertSelectAll(cell);
6266
}
6367

68+
@Test
69+
public void filterColumn_otherColumnOmitsValuesOfHiddenRows() {
70+
loadTestFixture(TestFixtures.SpreadsheetTable);
71+
final SpreadsheetElement spreadsheet = getSpreadsheet();
72+
73+
// Before filtering, the first column offers all of its values
74+
spreadsheet.getCellAt("B2").popupButtonClick();
75+
Assert.assertEquals(
76+
List.of("Cell 1:0", "Cell 2:0", "Cell 3:0", "Cell 4:0"),
77+
getFilterPopup().getOptions());
78+
closeFilterPopup();
79+
80+
// Filter the second column so that row 1 gets hidden
81+
spreadsheet.getCellAt("C2").popupButtonClick();
82+
getFilterPopup().deselectByText("Cell 1:1");
83+
closeFilterPopup();
84+
85+
// The first column no longer offers "Cell 1:0", as its row is hidden
86+
// by the second column
87+
spreadsheet.getCellAt("B2").popupButtonClick();
88+
Assert.assertEquals(List.of("Cell 2:0", "Cell 3:0", "Cell 4:0"),
89+
getFilterPopup().getOptions());
90+
}
91+
92+
@Test
93+
public void filterTwoColumns_eachColumnRetainsOwnFilteredValues() {
94+
loadTestFixture(TestFixtures.SpreadsheetTable);
95+
final SpreadsheetElement spreadsheet = getSpreadsheet();
96+
97+
// Filter the first column so that row 1 gets hidden
98+
spreadsheet.getCellAt("B2").popupButtonClick();
99+
getFilterPopup().deselectByText("Cell 1:0");
100+
closeFilterPopup();
101+
102+
// Filter the second column so that row 2 gets hidden
103+
spreadsheet.getCellAt("C2").popupButtonClick();
104+
getFilterPopup().deselectByText("Cell 2:1");
105+
closeFilterPopup();
106+
107+
// The first column still offers "Cell 1:0" as an unchecked option, so
108+
// its hidden row can be brought back independently of the second
109+
// column's filter. Row 2, hidden by the second column, is excluded.
110+
spreadsheet.getCellAt("B2").popupButtonClick();
111+
Assert.assertEquals(List.of("Cell 1:0", "Cell 3:0", "Cell 4:0"),
112+
getFilterPopup().getOptions());
113+
Assert.assertEquals(List.of("Cell 3:0", "Cell 4:0"),
114+
getFilterPopup().getSelectedTexts());
115+
}
116+
64117
private void assertSelectAll(SheetCellElement cell) {
65118
cell.popupButtonClick();
66119
Assert.assertTrue(hasOption("(Select All)"));
67120
}
121+
122+
private CheckboxGroupElement getFilterPopup() {
123+
return $(CheckboxGroupElement.class).single();
124+
}
125+
126+
private void closeFilterPopup() {
127+
findElement(By.className("v-window-closebox")).click();
128+
// The overlay fades out, so wait until it is actually gone before
129+
// opening the next one, otherwise single() would match two overlays.
130+
waitUntil(driver -> $(CheckboxGroupElement.class).all().isEmpty());
131+
}
68132
}

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

Lines changed: 23 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -251,32 +251,37 @@ public void updateOptions() {
251251
}
252252

253253
/**
254-
* Gets the currently NOT filtered cell values.
254+
* Gets the cell values of this column that are currently visible, i.e.,
255+
* excluding any hidden by this column's filter or whose rows are already
256+
* hidden by other columns' filters.
255257
*
256-
* @return All unique values currently visible (= not filtered) within this
257-
* column
258+
* @return the cell values
258259
*/
259260
protected Set<String> getVisibleValues() {
260-
Set<String> values = new HashSet<>();
261-
for (int r = filterRange.getFirstRow(); r <= filterRange
262-
.getLastRow(); r++) {
263-
if (!filteredRows.contains(r) && !spreadsheet.isRowHidden(r)) {
264-
values.add(spreadsheet.getCellValue(
265-
spreadsheet.getCell(r, filterRange.getFirstColumn())));
266-
}
261+
Set<String> values = getAllValues();
262+
for (int r : filteredRows) {
263+
values.remove(spreadsheet.getCellValue(
264+
spreadsheet.getCell(r, filterRange.getFirstColumn())));
267265
}
268266
return values;
269267
}
270268

271269
/**
272-
* Gets all of the unique values for this filter column.
270+
* Gets the cell values of this column, excluding those whose rows are
271+
* already hidden by other columns' filters.
273272
*
274-
* @return All unique values within this column
273+
* @return the cell values
275274
*/
276275
protected Set<String> getAllValues() {
276+
Set<Integer> otherHidden = filterTable
277+
.getRowsHiddenByOtherFilters(this);
278+
277279
Set<String> values = new HashSet<>();
278280
for (int r = filterRange.getFirstRow(); r <= filterRange
279281
.getLastRow(); r++) {
282+
if (otherHidden.contains(r)) {
283+
continue;
284+
}
280285
values.add(spreadsheet.getCellValue(
281286
spreadsheet.getCell(r, filterRange.getFirstColumn())));
282287
}
@@ -290,9 +295,15 @@ protected Set<String> getAllValues() {
290295
* the values that are NOT filtered
291296
*/
292297
protected void updateFilteredItems(Collection<String> visibleValues) {
298+
Set<Integer> otherHidden = filterTable
299+
.getRowsHiddenByOtherFilters(this);
300+
293301
filteredRows.clear();
294302
for (int r = filterRange.getFirstRow(); r <= filterRange
295303
.getLastRow(); r++) {
304+
if (otherHidden.contains(r)) {
305+
continue;
306+
}
296307
String cellValue = spreadsheet.getCellValue(
297308
spreadsheet.getCell(r, filterRange.getFirstColumn()));
298309
if (!visibleValues.contains(cellValue)) {

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

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -151,6 +151,17 @@ public void clearAllFilters() {
151151
popupButtonToClearButtonMap.get(popupButton).setEnabled(false);
152152
popupButton.markActive(false);
153153
}
154+
155+
// Refresh option lists after all filters cleared, so each ItemFilter
156+
// sees the now-empty filteredRows of its siblings.
157+
for (var filters : popupButtonToFiltersMap.values()) {
158+
for (var filter : filters) {
159+
if (filter instanceof ItemFilter itemFilter) {
160+
itemFilter.updateOptions();
161+
}
162+
}
163+
}
164+
154165
getSpreadsheet().setRowsHidden(IntStream
155166
.range(filteringRegion.getFirstRow(),
156167
filteringRegion.getLastRow() + 1)
@@ -262,6 +273,27 @@ public void onFiltersUpdated() {
262273
filteredRows::contains)));
263274
}
264275

276+
/**
277+
* Gets the union of rows hidden by all filters in this table except the
278+
* given one.
279+
*
280+
* @param self
281+
* Filter to exclude from the union
282+
* @return Rows hidden by other filters
283+
*/
284+
Set<Integer> getRowsHiddenByOtherFilters(SpreadsheetFilter self) {
285+
Set<Integer> hidden = new HashSet<>();
286+
for (HashSet<SpreadsheetFilter> filters : popupButtonToFiltersMap
287+
.values()) {
288+
for (SpreadsheetFilter filter : filters) {
289+
if (!filter.equals(self)) {
290+
hidden.addAll(filter.getFilteredRows());
291+
}
292+
}
293+
}
294+
return hidden;
295+
}
296+
265297
/**
266298
* Registers a new filter to this filter table and adds it inside the given
267299
* pop-up button.

0 commit comments

Comments
 (0)