From e5e765e683f7f13327e2f7ab888ee09de8fbf28f Mon Sep 17 00:00:00 2001 From: chunhtai <47866232+chunhtai@users.noreply.github.com> Date: Fri, 7 Apr 2023 10:47:06 -0700 Subject: [PATCH] Reland Refactor reorderable list semantics (#124395) Reland Refactor reorderable list semantics --- .../lib/src/material/reorderable_list.dart | 71 +------------ .../lib/src/widgets/reorderable_list.dart | 58 ++++++++++ .../test/material/reorderable_list_test.dart | 25 +++-- .../test/widgets/reorderable_list_test.dart | 100 ++++++++++++++++++ 4 files changed, 179 insertions(+), 75 deletions(-) diff --git a/packages/flutter/lib/src/material/reorderable_list.dart b/packages/flutter/lib/src/material/reorderable_list.dart index 43959b2a41..4627910251 100644 --- a/packages/flutter/lib/src/material/reorderable_list.dart +++ b/packages/flutter/lib/src/material/reorderable_list.dart @@ -5,13 +5,11 @@ import 'dart:ui' show lerpDouble; import 'package:flutter/gestures.dart'; -import 'package:flutter/rendering.dart'; import 'package:flutter/widgets.dart'; import 'debug.dart'; import 'icons.dart'; import 'material.dart'; -import 'material_localizations.dart'; import 'theme.dart'; /// A list whose items the user can interactively reorder by dragging. @@ -266,64 +264,6 @@ class ReorderableListView extends StatefulWidget { } class _ReorderableListViewState extends State { - Widget _wrapWithSemantics(Widget child, int index) { - void reorder(int startIndex, int endIndex) { - if (startIndex != endIndex) { - widget.onReorder(startIndex, endIndex); - } - } - - // First, determine which semantics actions apply. - final Map semanticsActions = {}; - - // Create the appropriate semantics actions. - void moveToStart() => reorder(index, 0); - void moveToEnd() => reorder(index, widget.itemCount); - void moveBefore() => reorder(index, index - 1); - // To move after, we go to index+2 because we are moving it to the space - // before index+2, which is after the space at index+1. - void moveAfter() => reorder(index, index + 2); - - final MaterialLocalizations localizations = MaterialLocalizations.of(context); - - // If the item can move to before its current position in the list. - if (index > 0) { - semanticsActions[CustomSemanticsAction(label: localizations.reorderItemToStart)] = moveToStart; - String reorderItemBefore = localizations.reorderItemUp; - if (widget.scrollDirection == Axis.horizontal) { - reorderItemBefore = Directionality.of(context) == TextDirection.ltr - ? localizations.reorderItemLeft - : localizations.reorderItemRight; - } - semanticsActions[CustomSemanticsAction(label: reorderItemBefore)] = moveBefore; - } - - // If the item can move to after its current position in the list. - if (index < widget.itemCount - 1) { - String reorderItemAfter = localizations.reorderItemDown; - if (widget.scrollDirection == Axis.horizontal) { - reorderItemAfter = Directionality.of(context) == TextDirection.ltr - ? localizations.reorderItemRight - : localizations.reorderItemLeft; - } - semanticsActions[CustomSemanticsAction(label: reorderItemAfter)] = moveAfter; - semanticsActions[CustomSemanticsAction(label: localizations.reorderItemToEnd)] = moveToEnd; - } - - // We pass toWrap with a GlobalKey into the item so that when it - // gets dragged, the accessibility framework can preserve the selected - // state of the dragging item. - // - // We also apply the relevant custom accessibility actions for moving the item - // up, down, to the start, and to the end of the list. - return MergeSemantics( - child: Semantics( - customSemanticsActions: semanticsActions, - child: child, - ), - ); - } - Widget _itemBuilder(BuildContext context, int index) { final Widget item = widget.itemBuilder(context, index); assert(() { @@ -335,9 +275,6 @@ class _ReorderableListViewState extends State { return true; }()); - // TODO(goderbauer): The semantics stuff should probably happen inside - // _ReorderableItem so the widget versions can have them as well. - final Widget itemWithSemantics = _wrapWithSemantics(item, index); final Key itemGlobalKey = _ReorderableListViewChildGlobalKey(item.key!, this); if (widget.buildDefaultDragHandles) { @@ -350,7 +287,7 @@ class _ReorderableListViewState extends State { return Stack( key: itemGlobalKey, children: [ - itemWithSemantics, + item, Positioned.directional( textDirection: Directionality.of(context), start: 0, @@ -370,7 +307,7 @@ class _ReorderableListViewState extends State { return Stack( key: itemGlobalKey, children: [ - itemWithSemantics, + item, Positioned.directional( textDirection: Directionality.of(context), top: 0, @@ -394,14 +331,14 @@ class _ReorderableListViewState extends State { return ReorderableDelayedDragStartListener( key: itemGlobalKey, index: index, - child: itemWithSemantics, + child: item, ); } } return KeyedSubtree( key: itemGlobalKey, - child: itemWithSemantics, + child: item, ); } diff --git a/packages/flutter/lib/src/widgets/reorderable_list.dart b/packages/flutter/lib/src/widgets/reorderable_list.dart index cf7f027b08..7a650a0b21 100644 --- a/packages/flutter/lib/src/widgets/reorderable_list.dart +++ b/packages/flutter/lib/src/widgets/reorderable_list.dart @@ -10,6 +10,7 @@ import 'basic.dart'; import 'debug.dart'; import 'framework.dart'; import 'inherited_theme.dart'; +import 'localizations.dart'; import 'media_query.dart'; import 'overlay.dart'; import 'scroll_controller.dart'; @@ -928,6 +929,63 @@ class SliverReorderableListState extends State with Ticke key: _ReorderableItemGlobalKey(child.key!, index, this), index: index, capturedThemes: InheritedTheme.capture(from: context, to: overlay.context), + child: _wrapWithSemantics(child, index), + ); + } + + Widget _wrapWithSemantics(Widget child, int index) { + void reorder(int startIndex, int endIndex) { + if (startIndex != endIndex) { + widget.onReorder(startIndex, endIndex); + } + } + + // First, determine which semantics actions apply. + final Map semanticsActions = {}; + + // Create the appropriate semantics actions. + void moveToStart() => reorder(index, 0); + void moveToEnd() => reorder(index, widget.itemCount); + void moveBefore() => reorder(index, index - 1); + // To move after, go to index+2 because it is moved to the space + // before index+2, which is after the space at index+1. + void moveAfter() => reorder(index, index + 2); + + final WidgetsLocalizations localizations = WidgetsLocalizations.of(context); + final bool isHorizontal = _scrollDirection == Axis.horizontal; + // If the item can move to before its current position in the list. + if (index > 0) { + semanticsActions[CustomSemanticsAction(label: localizations.reorderItemToStart)] = moveToStart; + String reorderItemBefore = localizations.reorderItemUp; + if (isHorizontal) { + reorderItemBefore = Directionality.of(context) == TextDirection.ltr + ? localizations.reorderItemLeft + : localizations.reorderItemRight; + } + semanticsActions[CustomSemanticsAction(label: reorderItemBefore)] = moveBefore; + } + + // If the item can move to after its current position in the list. + if (index < widget.itemCount - 1) { + String reorderItemAfter = localizations.reorderItemDown; + if (isHorizontal) { + reorderItemAfter = Directionality.of(context) == TextDirection.ltr + ? localizations.reorderItemRight + : localizations.reorderItemLeft; + } + semanticsActions[CustomSemanticsAction(label: reorderItemAfter)] = moveAfter; + semanticsActions[CustomSemanticsAction(label: localizations.reorderItemToEnd)] = moveToEnd; + } + + // Pass toWrap with a GlobalKey into the item so that when it + // gets dragged, the accessibility framework can preserve the selected + // state of the dragging item. + // + // Also apply the relevant custom accessibility actions for moving the item + // up, down, to the start, and to the end of the list. + return Semantics( + container: true, + customSemanticsActions: semanticsActions, child: child, ); } diff --git a/packages/flutter/test/material/reorderable_list_test.dart b/packages/flutter/test/material/reorderable_list_test.dart index ce3ce475da..fae4e7db87 100644 --- a/packages/flutter/test/material/reorderable_list_test.dart +++ b/packages/flutter/test/material/reorderable_list_test.dart @@ -673,8 +673,23 @@ void main() { // Get the switch tile's semantics: final SemanticsNode semanticsNode = tester.getSemantics(find.byKey(const Key('Switch tile'))); - // Check for properties of both SwitchTile semantics and the ReorderableListView custom semantics actions. + // Check for ReorderableListView custom semantics actions. expect(semanticsNode, matchesSemantics( + customActions: const [ + CustomSemanticsAction(label: 'Move up'), + CustomSemanticsAction(label: 'Move down'), + CustomSemanticsAction(label: 'Move to the end'), + CustomSemanticsAction(label: 'Move to the start'), + ], + )); + + // Check for properties of SwitchTile semantics. + late SemanticsNode child; + semanticsNode.visitChildren((SemanticsNode node) { + child = node; + return false; + }); + expect(child, matchesSemantics( hasToggledState: true, isToggled: true, isEnabled: true, @@ -682,12 +697,6 @@ void main() { hasEnabledState: true, label: 'Switch tile', hasTapAction: true, - customActions: const [ - CustomSemanticsAction(label: 'Move up'), - CustomSemanticsAction(label: 'Move down'), - CustomSemanticsAction(label: 'Move to the end'), - CustomSemanticsAction(label: 'Move to the start'), - ], )); handle.dispose(); }); @@ -1644,7 +1653,7 @@ void main() { DefaultMaterialLocalizations.delegate, DefaultWidgetsLocalizations.delegate, ], - child:SizedBox( + child: SizedBox( width: 100.0, height: 100.0, child: Directionality( diff --git a/packages/flutter/test/widgets/reorderable_list_test.dart b/packages/flutter/test/widgets/reorderable_list_test.dart index 1b5841621a..0876fa7f22 100644 --- a/packages/flutter/test/widgets/reorderable_list_test.dart +++ b/packages/flutter/test/widgets/reorderable_list_test.dart @@ -4,8 +4,11 @@ import 'package:flutter/gestures.dart'; import 'package:flutter/material.dart'; +import 'package:flutter/rendering.dart'; import 'package:flutter_test/flutter_test.dart'; +import 'semantics_tester.dart'; + void main() { testWidgets('SliverReorderableList works well when having gestureSettings', (WidgetTester tester) async { // Regression test for https://github.com/flutter/flutter/issues/103404 @@ -64,6 +67,103 @@ void main() { expect(items, orderedEquals([1, 0, 2, 3, 4])); }); + testWidgets('SliverReorderableList item has correct semantics', (WidgetTester tester) async { + final SemanticsTester semantics = SemanticsTester(tester); + const int itemCount = 5; + int onReorderCallCount = 0; + final List items = List.generate(itemCount, (int index) => index); + + void handleReorder(int fromIndex, int toIndex) { + onReorderCallCount += 1; + if (toIndex > fromIndex) { + toIndex -= 1; + } + items.insert(toIndex, items.removeAt(fromIndex)); + } + // The list has five elements of height 100 + await tester.pumpWidget( + MaterialApp( + home: MediaQuery( + data: const MediaQueryData(gestureSettings: DeviceGestureSettings(touchSlop: 8.0)), + child: CustomScrollView( + slivers: [ + SliverReorderableList( + itemCount: itemCount, + itemBuilder: (BuildContext context, int index) { + return SizedBox( + key: ValueKey(items[index]), + height: 100, + child: ReorderableDragStartListener( + index: index, + child: Text('item ${items[index]}'), + ), + ); + }, + onReorder: handleReorder, + ) + ], + ), + ), + ), + ); + + expect( + semantics, + includesNodeWith( + label: 'item 0', + actions: [SemanticsAction.customAction], + ), + ); + final SemanticsNode node = tester.getSemantics(find.text('item 0')); + + // perform custom action 'move down'. + tester.binding.pipelineOwner.semanticsOwner!.performAction(node.id, SemanticsAction.customAction, 0); + await tester.pumpAndSettle(); + + expect(onReorderCallCount, 1); + expect(items, orderedEquals([1, 0, 2, 3, 4])); + + semantics.dispose(); + }); + + testWidgets('SliverReorderableList custom semantics action has correct label', (WidgetTester tester) async { + const int itemCount = 5; + final List items = List.generate(itemCount, (int index) => index); + // The list has five elements of height 100 + await tester.pumpWidget( + MaterialApp( + home: MediaQuery( + data: const MediaQueryData(gestureSettings: DeviceGestureSettings(touchSlop: 8.0)), + child: CustomScrollView( + slivers: [ + SliverReorderableList( + itemCount: itemCount, + itemBuilder: (BuildContext context, int index) { + return SizedBox( + key: ValueKey(items[index]), + height: 100, + child: ReorderableDragStartListener( + index: index, + child: Text('item ${items[index]}'), + ), + ); + }, + onReorder: (int _, int __) { }, + ) + ], + ), + ), + ), + ); + final SemanticsNode node = tester.getSemantics(find.text('item 0')); + final SemanticsData data = node.getSemanticsData(); + expect(data.customSemanticsActionIds!.length, 2); + final CustomSemanticsAction action1 = CustomSemanticsAction.getAction(data.customSemanticsActionIds![0])!; + expect(action1.label, 'Move down'); + final CustomSemanticsAction action2 = CustomSemanticsAction.getAction(data.customSemanticsActionIds![1])!; + expect(action2.label, 'Move to the end'); + }); + // Regression test for https://github.com/flutter/flutter/issues/100451 testWidgets('SliverReorderableList.builder respects findChildIndexCallback', (WidgetTester tester) async { bool finderCalled = false;