Skip to content

Commit 5d8a4c4

Browse files
authored
[two_dimensional_scrollables] Fix merged cells unmerging behind pinned spans (#11418)
This PR addresses an issue where merged cells in a `TableView` would "unmerge" or break when the first cell of the merge was scrolled into the area overlaid by a pinned row or column. - Updated `RenderTableViewport` to expand the `_targetLeadingColumnPixel` and `_targetLeadingRowPixel` calculation to include the pinned extent. - Strengthened the documentation in `TableView` and `TableViewCell` to explicitly state that the same child and merge information **must** be returned from every vicinity in the merge to avoid unmerging when cells are culled due to lazy loading. Fixes flutter/flutter#174862 ## Pre-Review Checklist **Note**: The Flutter team is currently trialing the use of [Gemini Code Assist for GitHub](https://developers.google.com/gemini-code-assist/docs/review-github-code). Comments from the `gemini-code-assist` bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed. [^1]: Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling.
1 parent bebbbfd commit 5d8a4c4

File tree

5 files changed

+165
-13
lines changed

5 files changed

+165
-13
lines changed

packages/two_dimensional_scrollables/CHANGELOG.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,7 @@
1+
## 0.4.2
2+
3+
* Fixes an issue where merged cells would unmerge when the first cell was overlaid by a pinned row or column.
4+
15
## 0.4.1
26

37
* Adds warnings for TableView pinned rows and columns that exceed the viewport dimensions.

packages/two_dimensional_scrollables/lib/src/table_view/table.dart

Lines changed: 19 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -28,11 +28,19 @@ import 'table_span.dart';
2828
/// and columns through merging. The table supports lazy rendering and will only
2929
/// instantiate those cells that are currently visible in the table's viewport
3030
/// and those that extend into the [cacheExtent]. Therefore, when merging cells
31-
/// in a [TableView], the same child must be returned from every vicinity the
32-
/// merged cell contains. The `build` method will only be called once for a
33-
/// merged cell, but since the table's children are lazily laid out, returning
34-
/// the same child ensures the merged cell can be built no matter which part of
35-
/// it is visible.
31+
/// in a [TableView], the same child with the same merge information must be
32+
/// returned from every vicinity the merged cell contains. The `build` method
33+
/// will only be called once for a merged cell, but since the table's children
34+
/// are lazily laid out, returning the same child and merge information ensures
35+
/// the merged cell can be built no matter which part of it is visible.
36+
///
37+
/// For example, if a cell is configured to span 3 columns, starting at column 1,
38+
/// the [cellBuilder] must return a [TableViewCell] with the same [child],
39+
/// [columnMergeStart] as 1, and [columnMergeSpan] as 3 for all three
40+
/// [TableVicinity]s (column 1, 2, and 3). If the merge information is only
41+
/// provided for the first vicinity (column 1), and that vicinity is scrolled
42+
/// out of the viewport and [cacheExtent], the table will not know the following
43+
/// vicinities (column 2 and 3) are part of a merge and will "unmerge" them.
3644
///
3745
/// The layout of the table (e.g. how many rows/columns there are and their
3846
/// extents) as well as the content of the individual cells is defined by
@@ -380,7 +388,7 @@ class RenderTableViewport extends RenderTwoDimensionalViewport {
380388
// Where column layout begins, potentially outside of the visible area.
381389
double get _targetLeadingColumnPixel {
382390
return clampDouble(
383-
horizontalOffset.pixels - cacheExtent,
391+
horizontalOffset.pixels - math.max(_pinnedColumnsExtent, cacheExtent),
384392
0,
385393
double.infinity,
386394
);
@@ -398,7 +406,11 @@ class RenderTableViewport extends RenderTwoDimensionalViewport {
398406
bool get _rowsAreInfinite => delegate.rowCount == null;
399407
// Where row layout begins, potentially outside of the visible area.
400408
double get _targetLeadingRowPixel {
401-
return clampDouble(verticalOffset.pixels - cacheExtent, 0, double.infinity);
409+
return clampDouble(
410+
verticalOffset.pixels - math.max(_pinnedRowsExtent, cacheExtent),
411+
0,
412+
double.infinity,
413+
);
402414
}
403415

404416
// How far rows should be laid out in a given frame.

packages/two_dimensional_scrollables/lib/src/table_view/table_cell.dart

Lines changed: 15 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -95,11 +95,21 @@ class TableViewParentData extends TwoDimensionalViewportParentData {
9595
/// Creates a cell of the [TableView], along with information regarding merged
9696
/// cells and [RepaintBoundary]s.
9797
///
98-
/// When merging cells in a [TableView], the same child should be returned from
99-
/// every vicinity the merged cell contains. The `build` method will only be
100-
/// called once for a merged cell, but since the table's children are lazily
101-
/// laid out, returning the same child ensures the merged cell can be built no
102-
/// matter which part of it is visible.
98+
/// When merging cells in a [TableView], the same child with the same merge
99+
/// information must be returned from every vicinity the merged cell contains.
100+
/// The `build` method will only be called once for a merged cell, but since
101+
/// the table's children are lazily laid out, returning the same child and merge
102+
/// information ensures the merged cell can be built no matter which part of it
103+
/// is visible.
104+
///
105+
/// For example, if a cell is configured to span 3 columns, starting at column 1,
106+
/// the `cellBuilder` of the [TableView] must return a [TableViewCell] with the
107+
/// same [child], [columnMergeStart] as 1, and [columnMergeSpan] as 3 for all
108+
/// three [TableVicinity]s (column 1, 2, and 3). If the merge information is
109+
/// only provided for the first vicinity (column 1), and that vicinity is
110+
/// scrolled out of the viewport and [cacheExtent], the table will not know the
111+
/// following vicinities (column 2 and 3) are part of a merge and will "unmerge"
112+
/// them.
103113
class TableViewCell extends StatelessWidget {
104114
/// Creates a widget that controls how a child of a [TableView] spans across
105115
/// multiple rows or columns.

packages/two_dimensional_scrollables/pubspec.yaml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
name: two_dimensional_scrollables
22
description: Widgets that scroll using the two dimensional scrolling foundation.
3-
version: 0.4.1
3+
version: 0.4.2
44
repository: https://github.com/flutter/packages/tree/main/packages/two_dimensional_scrollables
55
issue_tracker: https://github.com/flutter/flutter/issues?q=is%3Aissue+is%3Aopen+label%3A%22p%3A+two_dimensional_scrollables%22+
66

packages/two_dimensional_scrollables/test/table_view/table_test.dart

Lines changed: 126 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4176,6 +4176,132 @@ void main() {
41764176
},
41774177
);
41784178

4179+
testWidgets(
4180+
'Merged cells should not unmerge when the first cell is overlaid by a pinned column',
4181+
(WidgetTester tester) async {
4182+
// Regression test for https://github.com/flutter/flutter/issues/174862
4183+
final horizontalController = ScrollController();
4184+
addTearDown(horizontalController.dispose);
4185+
4186+
await tester.pumpWidget(
4187+
MaterialApp(
4188+
home: Scaffold(
4189+
body: SizedBox(
4190+
width: 400,
4191+
height: 400,
4192+
child: TableView.builder(
4193+
cacheExtent: 0.0,
4194+
horizontalDetails: ScrollableDetails.horizontal(
4195+
controller: horizontalController,
4196+
),
4197+
pinnedColumnCount: 1,
4198+
columnCount: 10,
4199+
rowCount: 10,
4200+
columnBuilder: (int index) => TableSpan(
4201+
extent: FixedTableSpanExtent(index == 0 ? 100 : 50),
4202+
),
4203+
rowBuilder: (int index) =>
4204+
const TableSpan(extent: FixedTableSpanExtent(50)),
4205+
cellBuilder: (BuildContext context, TableVicinity vicinity) {
4206+
final isColumn1 = vicinity.column == 1;
4207+
return TableViewCell(
4208+
columnMergeStart: isColumn1 ? 1 : null,
4209+
columnMergeSpan: isColumn1 ? 3 : null,
4210+
child: Center(
4211+
child: Text('Cell ${vicinity.column},${vicinity.row}'),
4212+
),
4213+
);
4214+
},
4215+
),
4216+
),
4217+
),
4218+
),
4219+
);
4220+
4221+
// Initially, column 1 is visible next to pinned column 0.
4222+
expect(find.text('Cell 1,0'), findsOneWidget);
4223+
// Column 2 and 3 should be part of the merge, so they are not built.
4224+
expect(find.text('Cell 2,0'), findsNothing);
4225+
expect(find.text('Cell 3,0'), findsNothing);
4226+
4227+
// Scroll horizontally so that column 1 is entirely behind pinned column 0.
4228+
// Pinned column 0 is 0 to 100.
4229+
// Unpinned content starts at 100 (Column 1).
4230+
// Scroll 100 pixels.
4231+
// Content at 0 (start of column 1) moves to absolute 100 - 100 = 0.
4232+
// Content at 50 (end of column 1) moves to absolute 100 + (50 - 100) = 50.
4233+
// So column 1 (0 to 50) is entirely covered by pinned column 0.
4234+
// Column 2 (50 to 100) is also covered.
4235+
// Column 3 (100 to 150) is the first visible in unpinned area.
4236+
horizontalController.jumpTo(100);
4237+
await tester.pump();
4238+
4239+
// With the fix, column 1 is still built because it is under the pinned area.
4240+
// Since column 1 is built, its merge info is found and applied to columns 2 and 3.
4241+
expect(find.text('Cell 1,0'), findsOneWidget);
4242+
expect(find.text('Cell 2,0'), findsNothing);
4243+
expect(find.text('Cell 3,0'), findsNothing);
4244+
},
4245+
);
4246+
4247+
testWidgets(
4248+
'Merged cells should not unmerge when the first cell is overlaid by a pinned row',
4249+
(WidgetTester tester) async {
4250+
final verticalController = ScrollController();
4251+
addTearDown(verticalController.dispose);
4252+
4253+
await tester.pumpWidget(
4254+
MaterialApp(
4255+
home: Scaffold(
4256+
body: SizedBox(
4257+
width: 400,
4258+
height: 400,
4259+
child: TableView.builder(
4260+
cacheExtent: 0.0,
4261+
verticalDetails: ScrollableDetails.vertical(
4262+
controller: verticalController,
4263+
),
4264+
pinnedRowCount: 1,
4265+
columnCount: 10,
4266+
rowCount: 10,
4267+
columnBuilder: (int index) =>
4268+
const TableSpan(extent: FixedTableSpanExtent(50)),
4269+
rowBuilder: (int index) => TableSpan(
4270+
extent: FixedTableSpanExtent(index == 0 ? 100 : 50),
4271+
),
4272+
cellBuilder: (BuildContext context, TableVicinity vicinity) {
4273+
// Merged cell spanning rows 1, 2, and 3.
4274+
final isRow1 = vicinity.row == 1;
4275+
return TableViewCell(
4276+
rowMergeStart: isRow1 ? 1 : null,
4277+
rowMergeSpan: isRow1 ? 3 : null,
4278+
child: Center(
4279+
child: Text('Cell ${vicinity.column},${vicinity.row}'),
4280+
),
4281+
);
4282+
},
4283+
),
4284+
),
4285+
),
4286+
),
4287+
);
4288+
4289+
// Initially, row 1 is visible below pinned row 0.
4290+
expect(find.text('Cell 0,1'), findsOneWidget);
4291+
expect(find.text('Cell 0,2'), findsNothing);
4292+
expect(find.text('Cell 0,3'), findsNothing);
4293+
4294+
// Scroll vertically so that row 1 is entirely behind pinned row 0.
4295+
verticalController.jumpTo(100);
4296+
await tester.pump();
4297+
4298+
// Row 1 should still be built, maintaining the merge.
4299+
expect(find.text('Cell 0,1'), findsOneWidget);
4300+
expect(find.text('Cell 0,2'), findsNothing);
4301+
expect(find.text('Cell 0,3'), findsNothing);
4302+
},
4303+
);
4304+
41794305
testWidgets(
41804306
'Table does not crash when focusing outside of the table while focused text field is not in the view',
41814307
(WidgetTester tester) async {

0 commit comments

Comments
 (0)