Oh I am really glad you included this. Does this mean that where developers put the TableCell.none must correlate with the text direction?

As in... if TextDirection.rtl:
[TableCell(colSpan: 2), TableCell.none]

But if TextDirection.ltr developers should do:
[TableCell.none, TableCell(colSpan: 2)]

Also, just a nit, but best to use a switch statement here on the enum for clarity.

Good point, it keeps the same logic in both RTL and LTR. In short [TableCell(colSpan: 2), TableCell.none] works the same way in both directions but will be reversed. I know it’s a bit confusing at first when reading it like you showed, but it actually makes sense cuz in LTR, the first cell starts at the top left, and in RTL, at the top right, so the first cell in the array should be the same. In that example, TableCell.none has to stay at the end.

It maybe feels confusing cuz in your example it's written in a single line, and since we both read from left to right, it looks for us reversed in the table. But if we were using an RTL IDE, the code itself would also appear reversed, and then it would look correct again.

For example, the code above in "Example 1" will look like this in RTL:

Here and where this logic is repeated, instead of the additional case to ensure it does not go negative, can you use clampDouble?

I also struggled to follow the logic here a bit, can you explain a bit more way we set the height, then subtract? Up above on 1475, it only sets the trailing row of a span's height for row spans, with the potential in-betweens set to zero, and we are still in the row loop, so for every row, we iterate through every following row to subtract the height for what purpose?

(Forgive me, when I did merged cell support in TableView is melted my brain 😆 )

Sure, let me try to explain it:

The key idea is that we don’t know the final row heights upfront, but we do know the full height of a spanning cell when it’s first laid out.

So instead of trying to split that height immediately:

We store the full required height only in the last row of the span.
All intermediate rows stay at 0 for now.

Each time a row gets its final height, we subtract that height from all future pending spans.
This keeps track of how much height is still “unresolved” for those row-spanning cells.

When we finally reach the last row of the span, whatever height is left is applied to that row.
That guarantees the total height across all spanned rows matches the cell’s height exactly.

In short: We set once, subtract as we go, and apply the remainder at the end.
This avoids needing to precalculate or retroactively resize earlier rows.

FYI if I didn’t explain it in enough detail, I'm happy to write a more detailed explanation of each step and also point out where it’s important for TableCellVerticalAlignment to work correctly.

Gotcha. Acking this, appreciated the explanation!

the clamping non-negative is not addressed, should use math.max(0.0, ) instead

also would be good to put the explanation as code comment since this is non-trivial

Since TableCell.non is used in place of hidden cells, shouldn't this assertion logic still work? A TableCell for every cell?

Yes, the old assert would still work with TableCell.none. I changed it intentionally to scan every row and report the exact row index and expected column count when there’s a mismatch.
I think for small tables the old message is fine, but for large tables (e.g. 20×40) this makes debugging much faster, cuz you can immediately see which row is wrong and what was expected, instead of just getting a generic error.

So I’d prefer to keep the new logic for better diagnostics, but I’m fine reverting if you feel the simpler assert is more appropriate here.

What happens if the user puts another widget in a hidden cell instead of TableCell.none?

Currently, nothing will happen and it will just use it as a placeholder same as TableCell.none. I did consider adding an assert, but decided to leave it optional and allow the use of TableCell.none for a couple of reasons:

1. Some users might prefer to use SizedBox.shrink() or another widget, so this gives them the flexibility to choose.
2. Imagine a scenario where cells can be dynamically expanded or collapsed. In such cases, developers can simply update the colSpan or rowSpan value without having to switch between TableCell.none and an actual widget.

Anyway if you’d prefer to enforce the use of TableCell.none, I can add an assert, I'm fine with that too :)

Connecting the other thread - we checked against TableView, and overlap like this is allowed. It is in the realm of possibility folks may want to be able to do this for their UI.

I am slightly confused about the reasoning for allowing use of other widget in placed for the hidden cell? If I understand correctly

              TableRow(
                children: <Widget>[
                  TableCell(colSpan: 2, child: Text('Spanning Cell')),
                  Text('hidden'), // This is ignored?
                ],
              ),

This feels like code smell that they should use TableCell.none to be more descriptive.

Should this assert !isHiddenCell instead? Looking at the continue based on isHiddenCell on l#1619, we should not end up here if isHiddenCell is true.

Good point, I removed it directly in 0a3684d since, as you mentioned, we should never reach this code path.
I can still add an assert if you prefer, but the golden tests will fail in the case this path is reached anyway. I’m also fairly sure the widget tests would fail in that case as well. But if you prefer, I can also add an assert here.

we should never reach this code path.

How do we end up reaching it in widget and golden file tests?

This is I think the last outstanding question for me. I am about to go OOO for the holidays, but I'll be lurking around Github tinkering with personal projects. I'd love to help get this in without having to wait for the new year. :)
That being said, we do need a second review. I have sent off or a second reviewer, with the hopes that code review is the perfect pastime for the quiet holiday period. 🙏

How do we end up reaching it in widget and golden file tests?

Oops, my bad I just tested it and you're right. The tests won't fail cuz it seems the logic still keeps the "hidden" cell with a zero size, so it's more of a performance improvement and not something that would cause the logic to crash.

The point is, before, there was an if (!isHiddenCell) directly inside the switch for just some cases. But with the current logic, none of the cases should run if isHiddenCell is missing. So if we add an assert here, it needs to go before the switch, and it would look like this:

final bool isHiddenCell = currentRowHiddenCells.contains(x);
if (isHiddenCell) {
  continue;
}
assert(!isHiddenCell, '...');

I mean, we can do that, but it feels wrong because we'd be directly asserting the if right after it runs. Alternatively, we could add a doc comment like "Important for performance improvement", but on the other hand, I feel like if someone sees the code skipping "hidden" cells, it should already be clear that it's for performance reasons. But I’m open to it, I can add the assert, the doc comment, or both, whatever you prefer :)

This is I think the last outstanding question for me. I am about to go OOO...

Sounds good to me, and thanks a lot for pushing this forward 🙌
Really appreciate you lining up a second review as well.

Enjoy the holidays and OOO time! I’ll keep an eye on GitHub too, so if anything comes up during review I’m happy to jump in and help😊

Here as well?

convert this to be a private getter in TableCellParentData

same this can use a private getter for readability

I am slightly confused about the reasoning for allowing use of other widget in placed for the hidden cell? If I understand correctly

              TableRow(
                children: <Widget>[
                  TableCell(colSpan: 2, child: Text('Spanning Cell')),
                  Text('hidden'), // This is ignored?
                ],
              ),

This feels like code smell that they should use TableCell.none to be more descriptive.

Should we also consider colspan? for example the for colspan == 2 the width will be the sum of widths[x] + widths[x + 1]

the clamping non-negative is not addressed, should use math.max(0.0, ) instead

also would be good to put the explanation as code comment since this is non-trivial

I can understand the code from comment in line 1532, but I am not sure what the code comment in this line mean.

use line 1539 to set this value

final bool isTextDirectionRtl;

switch (...) {
   case ...
     isTextDirectionRtl = true;
}