Sitelet https://github.com/flutter/packages/pull/12666
Skip to content

[two_dimensional_scrollables] Exclude trailing pinned spans from the non-pinned range - #12666

Merged
auto-submit[bot] merged 5 commits into
flutter:mainfrom
m1roxx:tableview-trailing-pinned-double-layout
Sep 25, 2026
Merged

auto-submit[bot] merged 5 commits into
flutter:mainfrom
m1roxx:tableview-trailing-pinned-double-layout

Conversation

@m1roxx

@m1roxx m1roxx commented Aug 28, 2026 •

Copy link
Copy Markdown
Contributor

RenderTableViewport lays out and paints the table as nine regions: leading pinned, regular and
trailing pinned rows, each crossed with the same three column categories.
_updateFirstAndLastVisibleCell binary searches for the last regular row and column of the visible
range, and when no regular span reaches the trailing edge of the layout target it fell back to the
last index in the metrics map:

_lastNonPinnedColumn ??= _columnMetrics.length - 1;

Whenever trailingPinnedRowCount or trailingPinnedColumnCount is greater than zero, that last
index is a trailing pinned span. The regular range then overlaps the trailing pinned range, and
the same TableVicinity is visited by two regions in a single layout and paint pass.

_updateColumnMetrics and _updateRowMetrics already cap the same value correctly, with the rule
that _lastRegularColumnIndex and _lastRegularRowIndex express, so the two code paths disagreed.
This PR makes _updateFirstAndLastVisibleCell use those getters. The old fallback is kept for the
case where they are null — infinite spans with no null terminator — where trailing pinned spans
cannot exist, since _firstTrailingPinnedColumn and _firstTrailingPinnedRow both require a
non-null span count.

Fixes flutter/flutter#185842
Fixes flutter/flutter#190800

Beyond the duplicated work

The overlap is not only wasted layout. Scrolling a table to the end with a trailing pinned span
throws, because two regions claim the same cell:

Expected to re-use an element at (row: 0, column: 19), but none was found.
  package:flutter/src/widgets/two_dimensional_viewport.dart:377  'elementToReuse != null'

and, once the spans carry a decoration, the non-pinned region reaches a cell whose paintOffset was
never set for it:

Null check operator used on a null value
  RenderTableViewport._paintCells.getColumnRect  (table.dart:1888)

Dragging through a 60x60 table with pinned and trailing pinned rows and columns throws
Expected to re-use an element at ... and TableViewCell for (row: 1, column: 58) could not be found before this change, and nothing after it. That is the same subsystem and trigger as
flutter/flutter#190800, but the specific assertion reported there did not
reproduce in my runs, so I am not claiming this fixes it.

Tests

Two regression tests are added to table_test.dart, one per axis. Each scrolls a table with a
trailing pinned span to the end, asserts that no exception is thrown, and asserts that the trailing
pinned span's decoration is painted exactly once — by the trailing pinned region only. Both fail
before this change.

Counting cellBuilder calls does not work as a test here: buildOrObtainChildFor caches children
for the frame and RenderObject.layout early-returns on unchanged constraints, so the duplicated
visit is invisible from the delegate. The new CountingSpanDecoration helper observes it through
the public TableSpanDecoration.paint API instead.

Pre-Review Checklist

Footnotes

  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. ↩ ↩2

…non-pinned range

RenderTableViewport lays out and paints the table as nine regions: leading
pinned, regular and trailing pinned rows crossed with the same three column
categories. _updateFirstAndLastVisibleCell binary searches for the last
regular row and column of the visible range, and when no regular span
reaches the trailing edge of the layout target it fell back to the last
index in the metrics map, which is a trailing pinned span whenever
trailingPinnedRowCount or trailingPinnedColumnCount is greater than zero.

The regular range then overlapped the trailing pinned range, so the same
vicinity was visited by two regions in a single layout and paint pass. That
wastes work, and it also throws: "Expected to re-use an element at ...",
"TableViewCell for ... could not be found", and a null paintOffset while
computing span decoration bounds.

_updateColumnMetrics and _updateRowMetrics already cap the range with the
same rule that _lastRegularColumnIndex and _lastRegularRowIndex express, so
this reuses those getters and keeps the old fallback for the infinite case
where they are null and trailing pinned spans cannot exist.

Fixes flutter/flutter#185842
@github-actions github-actions Bot added p: two_dimensional_scrollables Issues pertaining to the two_dimensional_scrollables package triage-framework Should be looked at in framework triage labels Aug 28, 2026
@m1roxx
m1roxx marked this pull request as ready for review September 3, 2026 06:43
@gemini-code-assist

Copy link
Copy Markdown
Contributor

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

@Piinks
Piinks self-requested a review September 8, 2026 20:34
@m1roxx

m1roxx commented Sep 9, 2026

Copy link
Copy Markdown
Contributor Author

@Piinks friendly ping — this has been open since Aug 28 with green CI. It's a small fix, happy to rebase or adjust if anything is needed.

@Piinks

Piinks commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

@Piinks friendly ping — this has been open since Aug 28 with green CI. It's a small fix, happy to rebase or adjust if anything is needed.

And just assigned for review yesterday. Thanks for your patience. 🙂

@Piinks Piinks left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this! Can you add test coverage for when there is both pinned trailing rows and columns?

…ogether

Covers a table with both trailingPinnedRowCount and
trailingPinnedColumnCount scrolled to the end on both axes, as requested
in review. Without the fix this throws "Expected to re-use an element at
(row: 13, column: 19)".
@m1roxx

m1roxx commented Sep 17, 2026

Copy link
Copy Markdown
Contributor Author

Added in aedd88c — covers both trailing pinned rows and columns scrolled to the end; it fails without the fix.

…le range

_binarySearchFirstFromMap searched the whole metrics map, but trailing
pinned spans sit at its end and fail the regular-span condition, so the
condition is not monotonic across the map. Whenever the search probed a
trailing pinned span it discarded the regular spans before it: with
columnCount: 3 and trailingPinnedColumnCount: 2, a scroll left no regular
column in the visible range and its cells disappeared.

Restrict the search to the indices of the regular spans, and cover more
than one trailing pinned row and column.
…nned-double-layout

# Conflicts:
#	packages/two_dimensional_scrollables/CHANGELOG.md
@m1roxx

m1roxx commented Sep 22, 2026

Copy link
Copy Markdown
Contributor Author

Also pushed 2826fb1: _binarySearchFirstFromMap now only searches the regular spans' index range, since trailing pinned spans at the end of the metrics made the condition non-monotonic (with columnCount: 3, trailingPinnedColumnCount: 2, the regular column disappeared on scroll). Added tests with two trailing pinned rows and columns, and merged main.

@Piinks Piinks left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM thank you!

@Piinks Piinks added the CICD Run CI/CD label Sep 24, 2026

@tarrinneal tarrinneal left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might just be dumb, but that comment was hard to read. I won't prevent this from landing over it, but it might be worth a quick rewrite.

Comment thread packages/two_dimensional_scrollables/lib/src/table_view/table.dart Outdated
@flutter-dashboard flutter-dashboard Bot removed the CICD Run CI/CD label Sep 25, 2026
@m1roxx
m1roxx force-pushed the tableview-trailing-pinned-double-layout branch from b734779 to 835ab6f Compare September 25, 2026 10:34
@Piinks Piinks added CICD Run CI/CD autosubmit Merge PR when tree becomes green via auto submit App labels Sep 25, 2026
@auto-submit
auto-submit Bot merged commit c983254 into flutter:main Sep 25, 2026
14 checks passed
github-merge-queue Bot pushed a commit to flutter/flutter that referenced this pull request Sep 28, 2026
flutter/packages@e55e7ac...ba0364a

2026-09-26 stuartmorgan@google.com [google_maps_flutter] Indicate that
default iOS impl is discoraged (flutter/packages#12872)
2026-09-26 36861262+QuncCccccc@users.noreply.github.com [material_ui]
Add Material 3 Expressive IconButton (flutter/packages#12832)
2026-09-25 jessiewong401@gmail.com Plugin example apps to 9.3.1
(flutter/packages#13019)
2026-09-25 36861262+QuncCccccc@users.noreply.github.com [material_ui]
Don't clip MenuItemButton.leadingIcon (flutter/packages#12986)
2026-09-25 36861262+QuncCccccc@users.noreply.github.com [material_ui]
Migrate M3 ExpansionTile template to use new gen_defaults
(flutter/packages#12920)
2026-09-25 149176071+m1roxx@users.noreply.github.com
[two_dimensional_scrollables] Exclude trailing pinned spans from the
non-pinned range (flutter/packages#12666)
2026-09-25 36861262+QuncCccccc@users.noreply.github.com [material_ui]
Migrate M3 Drawer template to use new gen_defaults
(flutter/packages#12916)
2026-09-25 36861262+QuncCccccc@users.noreply.github.com [material_ui]
Migrate M3 Divider template to use new gen_defaults
(flutter/packages#12915)
2026-09-25 instantni.med@gmail.com [google_maps_flutter_web] Fix
AdvancedMarker anchors on web (flutter/packages#11966)

If this roll has caused a breakage, revert this CL and set the roller
to dry run mode using the controls here:
https://autoroll.skia.org/r/flutter-packages-flutter-autoroll
Please CC flutter-ecosystem@google.com on the revert to ensure that a
human
is aware of the problem.

To file a bug in Flutter:
https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

autosubmit Merge PR when tree becomes green via auto submit App CICD Run CI/CD p: two_dimensional_scrollables Issues pertaining to the two_dimensional_scrollables package triage-framework Should be looked at in framework triage

Projects

None yet

3 participants