Table widget rendering bug (#78081)

packages/flutter/lib/src/widgets/framework.dart

@@ -5495,12 +5495,15 @@ abstract class RenderObjectElement extends Element {
   /// acts as if the child was not in `oldChildren`.
   ///
   /// This function is a convenience wrapper around [updateChild], which updates
-  /// each individual child. When calling [updateChild], this function uses an
+  /// each individual child. If `slots` is non-null, the value for the `newSlot`
-  /// [IndexedSlot<Element>] as the value for the `newSlot` argument.
+  /// argument of [updateChild] is retrieved from that list using the index that
-  /// [IndexedSlot.index] is set to the index that the currently processed
+  /// the currently processed `child` corresponds to in the `newWidgets` list
-  /// `child` corresponds to in the `newWidgets` list and [IndexedSlot.value] is
+  /// (`newWidgets` and `slots` must have the same length). If `slots` is null,
-  /// set to the [Element] of the previous widget in that list (or null if it is
+  /// an [IndexedSlot<Element>] is used as the value for the `newSlot` argument.
-  /// the first child).
+  /// In that case, [IndexedSlot.index] is set to the index that the currently
+  /// processed `child` corresponds to in the `newWidgets` list and
+  /// [IndexedSlot.value] is set to the [Element] of the previous widget in that
+  /// list (or null if it is the first child).
   ///
   /// When the [slot] value of an [Element] changes, its
   /// associated [renderObject] needs to move to a new position in the child
@@ -5525,14 +5528,21 @@ abstract class RenderObjectElement extends Element {
   /// knows where a child needs to move to in a linked list by providing its new
   /// previous sibling.
   @protected
-  List<Element> updateChildren(List<Element> oldChildren, List<Widget> newWidgets, { Set<Element>? forgottenChildren }) {
+  List<Element> updateChildren(List<Element> oldChildren, List<Widget> newWidgets, { Set<Element>? forgottenChildren, List<Object?>? slots }) {
     assert(oldChildren != null);
     assert(newWidgets != null);
+    assert(slots == null || newWidgets.length == slots.length);
 
     Element? replaceWithNullIfForgotten(Element child) {
       return forgottenChildren != null && forgottenChildren.contains(child) ? null : child;
     }
 
+    Object? slotFor(int newChildIndex, Element? previousChild) {
+      return slots != null
+        ? slots[newChildIndex]
+        : IndexedSlot<Element?>(newChildIndex, previousChild);
+    }
+
     // This attempts to diff the new child list (newWidgets) with
     // the old child list (oldChildren), and produce a new list of elements to
     // be the new list of child elements of this element. The called of this
@@ -5581,7 +5591,7 @@ abstract class RenderObjectElement extends Element {
       assert(oldChild == null || oldChild._lifecycleState == _ElementLifecycle.active);
       if (oldChild == null || !Widget.canUpdate(oldChild.widget, newWidget))
         break;
-      final Element newChild = updateChild(oldChild, newWidget, IndexedSlot<Element?>(newChildrenTop, previousChild))!;
+      final Element newChild = updateChild(oldChild, newWidget, slotFor(newChildrenTop, previousChild))!;
       assert(newChild._lifecycleState == _ElementLifecycle.active);
       newChildren[newChildrenTop] = newChild;
       previousChild = newChild;
@@ -5639,7 +5649,7 @@ abstract class RenderObjectElement extends Element {
         }
       }
       assert(oldChild == null || Widget.canUpdate(oldChild.widget, newWidget));
-      final Element newChild = updateChild(oldChild, newWidget, IndexedSlot<Element?>(newChildrenTop, previousChild))!;
+      final Element newChild = updateChild(oldChild, newWidget, slotFor(newChildrenTop, previousChild))!;
       assert(newChild._lifecycleState == _ElementLifecycle.active);
       assert(oldChild == newChild || oldChild == null || oldChild._lifecycleState != _ElementLifecycle.active);
       newChildren[newChildrenTop] = newChild;
@@ -5661,7 +5671,7 @@ abstract class RenderObjectElement extends Element {
       assert(oldChild._lifecycleState == _ElementLifecycle.active);
       final Widget newWidget = newWidgets[newChildrenTop];
       assert(Widget.canUpdate(oldChild.widget, newWidget));
-      final Element newChild = updateChild(oldChild, newWidget, IndexedSlot<Element?>(newChildrenTop, previousChild))!;
+      final Element newChild = updateChild(oldChild, newWidget, slotFor(newChildrenTop, previousChild))!;
       assert(newChild._lifecycleState == _ElementLifecycle.active);
       assert(oldChild == newChild || oldChild == null || oldChild._lifecycleState != _ElementLifecycle.active);
       newChildren[newChildrenTop] = newChild;

packages/flutter/lib/src/widgets/table.dart

@@ -348,37 +348,50 @@ class _TableElement extends RenderObjectElement {
   @override
   RenderTable get renderObject => super.renderObject as RenderTable;
 
-  // This class ignores the child's slot entirely.
-  // Instead of doing incremental updates to the child list, it replaces the entire list each frame.
-
   List<_TableElementRow> _children = const<_TableElementRow>[];
 
+  bool _doingMountOrUpdate = false;
+
   @override
-  void mount(Element? parent, dynamic newSlot) {
+  void mount(Element? parent, Object? newSlot) {
+    assert(!_doingMountOrUpdate);
+    _doingMountOrUpdate = true;
     super.mount(parent, newSlot);
+    int rowIndex = -1;
     _children = widget.children.map<_TableElementRow>((TableRow row) {
+      int columnIndex = 0;
+      rowIndex += 1;
       return _TableElementRow(
         key: row.key,
         children: row.children!.map<Element>((Widget child) {
           assert(child != null);
-          return inflateWidget(child, null);
+          return inflateWidget(child, _TableSlot(columnIndex++, rowIndex));
         }).toList(growable: false),
       );
     }).toList(growable: false);
     _updateRenderObjectChildren();
+    assert(_doingMountOrUpdate);
+    _doingMountOrUpdate = false;
   }
 
   @override
-  void insertRenderObjectChild(RenderObject child, dynamic slot) {
+  void insertRenderObjectChild(RenderBox child, _TableSlot slot) {
     renderObject.setupParentData(child);
+    // Once [mount]/[update] are done, the children are getting set all at once
+    // in [_updateRenderObjectChildren].
+    if (!_doingMountOrUpdate) {
+      renderObject.setChild(slot.column, slot.row, child);
+    }
   }
 
   @override
-  void moveRenderObjectChild(RenderObject child, dynamic oldSlot, dynamic newSlot) {
+  void moveRenderObjectChild(RenderBox child, _TableSlot oldSlot, _TableSlot newSlot) {
+    assert(_doingMountOrUpdate);
+    // Child gets moved at the end of [update] in [_updateRenderObjectChildren].
   }
 
   @override
-  void removeRenderObjectChild(RenderObject child, dynamic slot) {
+  void removeRenderObjectChild(RenderBox child, _TableSlot slot) {
     final TableCellParentData childParentData = child.parentData! as TableCellParentData;
     renderObject.setChild(childParentData.x!, childParentData.y!, null);
   }
@@ -387,6 +400,8 @@ class _TableElement extends RenderObjectElement {
 
   @override
   void update(Table newWidget) {
+    assert(!_doingMountOrUpdate);
+    _doingMountOrUpdate = true;
     final Map<LocalKey, List<Element>> oldKeyedRows = <LocalKey, List<Element>>{};
     for (final _TableElementRow row in _children) {
       if (row.key != null) {
@@ -396,7 +411,8 @@ class _TableElement extends RenderObjectElement {
     final Iterator<_TableElementRow> oldUnkeyedRows = _children.where((_TableElementRow row) => row.key == null).iterator;
     final List<_TableElementRow> newChildren = <_TableElementRow>[];
     final Set<List<Element>> taken = <List<Element>>{};
-    for (final TableRow row in newWidget.children) {
+    for (int rowIndex = 0; rowIndex < newWidget.children.length; rowIndex++) {
+      final TableRow row = newWidget.children[rowIndex];
       List<Element> oldChildren;
       if (row.key != null && oldKeyedRows.containsKey(row.key)) {
         oldChildren = oldKeyedRows[row.key]!;
@@ -406,9 +422,13 @@ class _TableElement extends RenderObjectElement {
       } else {
         oldChildren = const <Element>[];
       }
+      final List<_TableSlot> slots = List<_TableSlot>.generate(
+        row.children!.length,
+        (int columnIndex) => _TableSlot(columnIndex, rowIndex),
+      );
       newChildren.add(_TableElementRow(
         key: row.key,
-        children: updateChildren(oldChildren, row.children!, forgottenChildren: _forgottenChildren),
+        children: updateChildren(oldChildren, row.children!, forgottenChildren: _forgottenChildren, slots: slots),
       ));
     }
     while (oldUnkeyedRows.moveNext())
@@ -421,6 +441,8 @@ class _TableElement extends RenderObjectElement {
     _forgottenChildren.clear();
     super.update(newWidget);
     assert(widget == newWidget);
+    assert(_doingMountOrUpdate);
+    _doingMountOrUpdate = false;
   }
 
   void _updateRenderObjectChildren() {
@@ -489,3 +511,30 @@ class TableCell extends ParentDataWidget<TableCellParentData> {
     properties.add(EnumProperty<TableCellVerticalAlignment>('verticalAlignment', verticalAlignment));
   }
 }
+
+@immutable
+class _TableSlot with Diagnosticable {
+  const _TableSlot(this.column, this.row);
+
+  final int column;
+  final int row;
+
+  @override
+  bool operator ==(Object other) {
+    if (other.runtimeType != runtimeType)
+      return false;
+    return other is _TableSlot
+        && column == other.column
+        && row == other.row;
+  }
+
+  @override
+  int get hashCode => hashValues(column, row);
+
+  @override
+  void debugFillProperties(DiagnosticPropertiesBuilder properties) {
+    super.debugFillProperties(properties);
+    properties.add(IntProperty('x', column));
+    properties.add(IntProperty('y', row));
+  }
+}

packages/flutter/test/widgets/table_test.dart

@@ -964,5 +964,41 @@ void main() {
       }
   });
 
+  testWidgets('Can replace child with a different RenderObject type', (WidgetTester tester) async {
+    // Regression test for https://github.com/flutter/flutter/issues/69395.
+    await tester.pumpWidget(
+      Directionality(
+        textDirection: TextDirection.ltr,
+        child: Table(children: const <TableRow>[
+          TableRow(children: <Widget>[
+            TestChildWidget(),
+            TestChildWidget(),
+            TestChildWidget(),
+          ]),
+          TableRow(children: <Widget>[
+            TestChildWidget(),
+            TestChildWidget(),
+            TestChildWidget(),
+          ]),
+        ]),
+      ),
+    );
+    final RenderTable table = tester.renderObject(find.byType(Table));
+
+    expect(find.text('CRASHHH'), findsNothing);
+    expect(find.byType(SizedBox), findsNWidgets(3 * 2));
+    final Type toBeReplaced = table.column(2).last.runtimeType;
+
+    final TestChildState state = tester.state(find.byType(TestChildWidget).last);
+    state.toggleMe();
+    await tester.pump();
+
+    expect(find.byType(SizedBox), findsNWidgets(5));
+    expect(find.text('CRASHHH'), findsOneWidget);
+
+    // The RenderObject got replaced by a different type.
+    expect(table.column(2).last.runtimeType, isNot(toBeReplaced));
+  });
+
   // TODO(ianh): Test handling of TableCell object
 }