diff --git a/packages/devtools_app/lib/src/screens/inspector/inspector_tree_controller.dart b/packages/devtools_app/lib/src/screens/inspector/inspector_tree_controller.dart index 4e772e48c1e..67c39baebfd 100644 --- a/packages/devtools_app/lib/src/screens/inspector/inspector_tree_controller.dart +++ b/packages/devtools_app/lib/src/screens/inspector/inspector_tree_controller.dart @@ -357,7 +357,10 @@ class InspectorTreeController extends DisposableController return true; } if (selectionLocal.parent != null) { - return setSelectedNode(selectionLocal.parent); + return setSelectedNode( + selectionLocal.parent, + notifyFlutterInspector: true, + ); } return false; }, @@ -399,8 +402,7 @@ class InspectorTreeController extends DisposableController _numRows - 1, ), )?.node; - setSelectedNode(nodeToSelect); - return true; + return setSelectedNode(nodeToSelect, notifyFlutterInspector: true); }, ); } @@ -573,7 +575,12 @@ class InspectorTreeController extends DisposableController if (diagnostic != null && diagnostic.groupIsHidden) { diagnostic.hideableGroupLeader?.toggleHiddenGroup(); } - expandPath(node); + // Intentionally do NOT call expandPath(node) here. User clicks happen on + // already-visible rows, so ancestors are already expanded; calling + // expandPath would also re-expand the clicked node itself, undoing any + // collapse the user just performed via the left arrow key. Programmatic + // selection paths (search, on-device pick) call expandPath themselves via + // [InspectorController.syncTreeSelection]. } Rect getBoundingBox(InspectorTreeRow row) { @@ -1142,8 +1149,12 @@ class _InspectorTreeState extends State valueListenable: treeControllerLocal.rowsInTree, builder: (context, rows, _) { // Note: The inspector rows contain only the fake root node when the - // inspector tree is shutdown. - if (rows.length <= 1) { + // inspector tree is shutdown. Only show the loading indicator on the + // initial tree load (before [firstInspectorTreeLoadCompleted] is set); + // after that, a one-row tree is the legitimate result of the user + // collapsing all the way to the root via the keyboard, and we should + // render the tree (with its single row) rather than a spinner. + if (rows.length <= 1 && !controller.firstInspectorTreeLoadCompleted) { // This works around a bug when Scrollbars are present on a short lived // widget. return const SizedBox(child: CenteredCircularProgressIndicator()); diff --git a/packages/devtools_app/release_notes/NEXT_RELEASE_NOTES.md b/packages/devtools_app/release_notes/NEXT_RELEASE_NOTES.md index 639875d1ad6..a53a14bc17c 100644 --- a/packages/devtools_app/release_notes/NEXT_RELEASE_NOTES.md +++ b/packages/devtools_app/release_notes/NEXT_RELEASE_NOTES.md @@ -21,6 +21,9 @@ TODO: Remove this section if there are not any updates. - Deleted the option to use the legacy inspector. [#9782](https://github.com/flutter/devtools/pull/9782) +- Fixed an issue where navigating the Inspector widget tree with the keyboard arrow keys did not update the selected widget in the connected Flutter app. [#9810](https://github.com/flutter/devtools/pull/9810) +- Fixed an issue where clicking a widget row after collapsing a subtree with the left arrow key unexpectedly re-expanded the subtree. [#9810](https://github.com/flutter/devtools/pull/9810) +- Fixed an issue where collapsing the Inspector widget tree to a single row with the left arrow key caused a loading spinner to appear instead of showing the root node. [#9810](https://github.com/flutter/devtools/pull/9810) ## Performance updates diff --git a/packages/devtools_app/test/screens/inspector/inspector_tree_test.dart b/packages/devtools_app/test/screens/inspector/inspector_tree_test.dart index feda195f583..6d87ea56988 100644 --- a/packages/devtools_app/test/screens/inspector/inspector_tree_test.dart +++ b/packages/devtools_app/test/screens/inspector/inspector_tree_test.dart @@ -3,12 +3,12 @@ // found in the LICENSE file or at https://developers.google.com/open-source/licenses/bsd. import 'package:devtools_app/devtools_app.dart'; - import 'package:devtools_app_shared/ui.dart'; import 'package:devtools_app_shared/utils.dart'; import 'package:devtools_test/devtools_test.dart'; import 'package:devtools_test/helpers.dart'; import 'package:flutter/material.dart'; +import 'package:flutter/services.dart'; import 'package:flutter_test/flutter_test.dart' hide Fake; import 'package:mockito/mockito.dart'; @@ -57,6 +57,24 @@ void main() { ); } + InspectorTreeController buildTreeController({ + required void Function({bool notifyFlutterInspector}) onSelectionChange, + }) { + return InspectorTreeController() + ..config = InspectorTreeConfig( + onNodeAdded: (_, _) {}, + onClientActiveChange: (_) {}, + onSelectionChange: onSelectionChange, + ) + ..root = (InspectorTreeNode() + ..appendChild(InspectorTreeNode()) + ..appendChild(InspectorTreeNode())); + } + + List visibleNodes(InspectorTreeController controller) { + return controller.rowsInTree.value.map((row) => row!.node).toList(); + } + group('InspectorTreeController', () { testWidgets('Row with negative index regression test', ( WidgetTester tester, @@ -136,4 +154,425 @@ void main() { expect(find.richText('Text: "Multiline text content"'), findsOneWidget); }); }); + + group('InspectorTreeController keyboard navigation', () { + testWidgets( + 'navigateDown triggers onSelectionChange with notifyFlutterInspector true', + (WidgetTester tester) async { + bool? capturedNotify; + final treeController = buildTreeController( + onSelectionChange: ({bool notifyFlutterInspector = false}) { + capturedNotify = notifyFlutterInspector; + }, + ); + + await pumpInspectorTree(tester, treeController: treeController); + + treeController.navigateDown(); + await tester.pump(); + + expect(capturedNotify, isTrue); + }, + ); + + testWidgets( + 'navigateUp triggers onSelectionChange with notifyFlutterInspector true', + (WidgetTester tester) async { + bool? capturedNotify; + final treeController = buildTreeController( + onSelectionChange: ({bool notifyFlutterInspector = false}) { + capturedNotify = notifyFlutterInspector; + }, + ); + + await pumpInspectorTree(tester, treeController: treeController); + + // Move selection to the second row so navigateUp has somewhere to go. + treeController.navigateDown(); + await tester.pump(); + treeController.navigateDown(); + await tester.pump(); + + capturedNotify = null; + treeController.navigateUp(); + await tester.pump(); + + expect(capturedNotify, isTrue); + }, + ); + + testWidgets( + 'navigateRight triggers onSelectionChange with notifyFlutterInspector true', + (WidgetTester tester) async { + bool? capturedNotify; + final treeController = buildTreeController( + onSelectionChange: ({bool notifyFlutterInspector = false}) { + capturedNotify = notifyFlutterInspector; + }, + ); + + await pumpInspectorTree(tester, treeController: treeController); + + final root = treeController.root!; + final firstChild = root.children.first; + root.isExpanded = false; + treeController.setSelectedNode(root); + + // First right-arrow navigation expands the selected node. + capturedNotify = null; + treeController.navigateRight(); + await tester.pump(); + + expect(root.isExpanded, isTrue); + expect(treeController.selection, root); + expect(capturedNotify, isNull); + + // Once expanded, right-arrow navigation selects the next visible row. + treeController.navigateRight(); + await tester.pump(); + + expect(treeController.selection, firstChild); + expect(capturedNotify, isTrue); + }, + ); + + testWidgets( + 'navigateLeft triggers onSelectionChange with notifyFlutterInspector true', + (WidgetTester tester) async { + bool? capturedNotify; + final treeController = buildTreeController( + onSelectionChange: ({bool notifyFlutterInspector = false}) { + capturedNotify = notifyFlutterInspector; + }, + ); + + await pumpInspectorTree(tester, treeController: treeController); + + final root = treeController.root!; + final firstChild = root.children.first..isExpanded = false; + treeController.setSelectedNode(firstChild); + + capturedNotify = null; + treeController.navigateLeft(); + await tester.pump(); + + expect(treeController.selection, root); + expect(capturedNotify, isTrue); + }, + ); + }); + + group('InspectorTree keyboard events', () { + testWidgets( + 'arrowDown key triggers onSelectionChange with notifyFlutterInspector true', + (WidgetTester tester) async { + bool? capturedNotify; + final treeController = buildTreeController( + onSelectionChange: ({bool notifyFlutterInspector = false}) { + capturedNotify = notifyFlutterInspector; + }, + ); + + await pumpInspectorTree(tester, treeController: treeController); + + await tester.sendKeyEvent(LogicalKeyboardKey.arrowDown); + await tester.pump(); + + expect(capturedNotify, isTrue); + }, + ); + + testWidgets( + 'arrowUp key triggers onSelectionChange with notifyFlutterInspector true', + (WidgetTester tester) async { + bool? capturedNotify; + final treeController = buildTreeController( + onSelectionChange: ({bool notifyFlutterInspector = false}) { + capturedNotify = notifyFlutterInspector; + }, + ); + + await pumpInspectorTree(tester, treeController: treeController); + + // Move selection to the second row first. + await tester.sendKeyEvent(LogicalKeyboardKey.arrowDown); + await tester.pump(); + await tester.sendKeyEvent(LogicalKeyboardKey.arrowDown); + await tester.pump(); + + capturedNotify = null; + await tester.sendKeyEvent(LogicalKeyboardKey.arrowUp); + await tester.pump(); + + expect(capturedNotify, isTrue); + }, + ); + + testWidgets( + 'arrowRight key triggers onSelectionChange with notifyFlutterInspector true', + (WidgetTester tester) async { + bool? capturedNotify; + final treeController = buildTreeController( + onSelectionChange: ({bool notifyFlutterInspector = false}) { + capturedNotify = notifyFlutterInspector; + }, + ); + + await pumpInspectorTree(tester, treeController: treeController); + + final root = treeController.root!; + final firstChild = root.children.first; + root.isExpanded = false; + treeController.setSelectedNode(root); + + // First arrowRight expands the selected node. + capturedNotify = null; + await tester.sendKeyEvent(LogicalKeyboardKey.arrowRight); + await tester.pump(); + + expect(root.isExpanded, isTrue); + expect(treeController.selection, root); + expect(capturedNotify, isNull); + + // Once expanded, arrowRight selects the next visible row. + await tester.sendKeyEvent(LogicalKeyboardKey.arrowRight); + await tester.pump(); + + expect(treeController.selection, firstChild); + expect(capturedNotify, isTrue); + }, + ); + + testWidgets( + 'arrowLeft key triggers onSelectionChange with notifyFlutterInspector true', + (WidgetTester tester) async { + bool? capturedNotify; + final treeController = buildTreeController( + onSelectionChange: ({bool notifyFlutterInspector = false}) { + capturedNotify = notifyFlutterInspector; + }, + ); + + await pumpInspectorTree(tester, treeController: treeController); + + final root = treeController.root!; + final firstChild = root.children.first..isExpanded = false; + treeController.setSelectedNode(firstChild); + + capturedNotify = null; + await tester.sendKeyEvent(LogicalKeyboardKey.arrowLeft); + await tester.pump(); + + expect(treeController.selection, root); + expect(capturedNotify, isTrue); + }, + ); + + testWidgets( + 'arrowLeft key collapses selected node without removing previous rows', + (WidgetTester tester) async { + final treeController = buildTreeController( + onSelectionChange: ({bool notifyFlutterInspector = false}) {}, + ); + + final root = treeController.root!; + final previousSibling = root.children.first; + final selectedSibling = root.children.last + ..appendChild(InspectorTreeNode()) + ..appendChild(InspectorTreeNode()); + final selectedSiblingFirstChild = selectedSibling.children.first; + final selectedSiblingSecondChild = selectedSibling.children.last; + treeController.root = root; + treeController.setSelectedNode(selectedSibling); + + await pumpInspectorTree(tester, treeController: treeController); + + expect(visibleNodes(treeController), [ + root, + previousSibling, + selectedSibling, + selectedSiblingFirstChild, + selectedSiblingSecondChild, + ]); + + await tester.sendKeyEvent(LogicalKeyboardKey.arrowLeft); + await tester.pump(); + + expect(selectedSibling.isExpanded, isFalse); + expect(treeController.selection, selectedSibling); + expect(visibleNodes(treeController), [ + root, + previousSibling, + selectedSibling, + ]); + }, + ); + + testWidgets( + 'arrowLeft key on collapsed child selects parent without changing rows', + (WidgetTester tester) async { + final treeController = buildTreeController( + onSelectionChange: ({bool notifyFlutterInspector = false}) {}, + ); + + final root = treeController.root!; + final previousSibling = root.children.first; + final parent = root.children.last + ..appendChild(InspectorTreeNode()) + ..appendChild(InspectorTreeNode()); + final child = parent.children.first..isExpanded = false; + final nextSibling = parent.children.last; + treeController.root = root; + treeController.setSelectedNode(child); + + await pumpInspectorTree(tester, treeController: treeController); + + final rowsBeforeArrowLeft = visibleNodes(treeController); + expect(rowsBeforeArrowLeft, [ + root, + previousSibling, + parent, + child, + nextSibling, + ]); + + await tester.sendKeyEvent(LogicalKeyboardKey.arrowLeft); + await tester.pump(); + + expect(treeController.selection, parent); + expect(visibleNodes(treeController), rowsBeforeArrowLeft); + }, + ); + + testWidgets('arrowLeft key does not put the tree into the loading state', ( + WidgetTester tester, + ) async { + final treeController = buildTreeController( + onSelectionChange: ({bool notifyFlutterInspector = false}) {}, + ); + + await pumpInspectorTree(tester, treeController: treeController); + + final root = treeController.root!; + treeController.setSelectedNode(root); + await tester.pump(); + + await tester.sendKeyEvent(LogicalKeyboardKey.arrowLeft); + await tester.pump(); + + // After collapsing the root, only the root row remains visible. The + // tree must still render that row instead of a loading indicator. + expect(find.byType(CenteredCircularProgressIndicator), findsNothing); + expect(visibleNodes(treeController), [root]); + }); + + testWidgets( + 'onSelectNode does not re-expand collapsed siblings of the clicked node', + (WidgetTester tester) async { + // Regression test: clicking a still-visible row used to call + // expandPath on the clicked node, which re-expanded the clicked node + // itself and undid any subtree collapses the user had just performed + // via the arrow-left key. + final treeController = buildTreeController( + onSelectionChange: ({bool notifyFlutterInspector = false}) {}, + ); + + final root = treeController.root!; + final firstChild = root.children.first + ..appendChild(InspectorTreeNode()) + ..appendChild(InspectorTreeNode()); + final secondChild = root.children.last; + treeController.root = root; + treeController.setSelectedNode(firstChild); + + await pumpInspectorTree(tester, treeController: treeController); + + // Collapse [firstChild] so its grandchildren are hidden. + await tester.sendKeyEvent(LogicalKeyboardKey.arrowLeft); + await tester.pump(); + expect(firstChild.isExpanded, isFalse); + expect(visibleNodes(treeController), [root, firstChild, secondChild]); + + // Selecting another visible row must not re-expand [firstChild]. + treeController.onSelectNode(secondChild); + await tester.pump(); + + expect(firstChild.isExpanded, isFalse); + expect(visibleNodes(treeController), [root, firstChild, secondChild]); + }, + ); + + testWidgets( + 'onSelectNode does not re-expand a node the user just collapsed by ' + 'clicking it', + (WidgetTester tester) async { + // Regression test: clicking a row used to call expandPath on the + // clicked node itself, so a user could not select a node in its + // collapsed state. + final treeController = buildTreeController( + onSelectionChange: ({bool notifyFlutterInspector = false}) {}, + ); + + final root = treeController.root!; + final firstChild = root.children.first + ..appendChild(InspectorTreeNode()) + ..isExpanded = false; + treeController.root = root; + + await pumpInspectorTree(tester, treeController: treeController); + + treeController.onSelectNode(firstChild); + await tester.pump(); + + expect(treeController.selection, firstChild); + expect(firstChild.isExpanded, isFalse); + }, + ); + }); + + group('InspectorTree loading indicator', () { + testWidgets( + 'shows a loading indicator while the initial tree load is in progress', + (WidgetTester tester) async { + // Before the first inspector tree load completes, a tree with at most + // a single row represents the "still loading" state and should render + // a progress indicator instead of the bare row. + inspectorController.firstInspectorTreeLoadCompleted = false; + + final treeController = InspectorTreeController() + ..config = InspectorTreeConfig( + onNodeAdded: (_, _) {}, + onClientActiveChange: (_) {}, + ) + ..root = InspectorTreeNode(); + + await pumpInspectorTree(tester, treeController: treeController); + + expect(find.byType(CenteredCircularProgressIndicator), findsOneWidget); + }, + ); + + testWidgets( + 'renders a single-row tree (no spinner) after the initial load has ' + 'completed', + (WidgetTester tester) async { + // Regression test: collapsing the root via the arrow-left key shrinks + // the visible rows down to a single row. Before the fix, the + // [InspectorTree] widget treated a one-row tree as "loading" and + // showed a spinner, hiding the user's [root] row. + inspectorController.firstInspectorTreeLoadCompleted = true; + + final treeController = InspectorTreeController() + ..config = InspectorTreeConfig( + onNodeAdded: (_, _) {}, + onClientActiveChange: (_) {}, + ) + ..root = InspectorTreeNode(); + + await pumpInspectorTree(tester, treeController: treeController); + + expect(find.byType(CenteredCircularProgressIndicator), findsNothing); + }, + ); + }); }