这是indexloc提供的服务，不要输入任何密码
Skip to content

Fix directional focus in nested scrollables with different axis #172875

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Aug 12, 2025
Merged

Fix directional focus in nested scrollables with different axis #172875

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d421f60
fix: DirectionalFocusTraversalPolicyMixin considers scrollables axis …
romaingyh Jul 28, 2025
d2cf298
Add tests to verify correct focus behavior when navigating through ne…
romaingyh Jul 28, 2025
c5bfae5
refactor: Remove test skip on browser
romaingyh Aug 12, 2025
fe86f7b
chore: Add missing periods to comments
romaingyh Aug 12, 2025
378d82d
test: Skip focus traversal navigation in nested lists tests on browse…
romaingyh Aug 12, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 14 additions & 5 deletions packages/flutter/lib/src/widgets/focus_traversal.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -815,7 +815,6 @@ mixin DirectionalFocusTraversalPolicyMixin on FocusTraversalPolicy {
TraversalDirection direction, {
bool forward = true,
}) {
final ScrollableState? focusedScrollable = Scrollable.maybeOf(focusedChild.context!);
switch (direction) {
case TraversalDirection.down:
case TraversalDirection.up:
Expand All @@ -828,9 +827,14 @@ mixin DirectionalFocusTraversalPolicyMixin on FocusTraversalPolicy {
if (eligibleNodes.isEmpty) {
break;
}
if (focusedScrollable != null && !focusedScrollable.position.atEdge) {
final ScrollableState? focusedScrollable = Scrollable.maybeOf(
focusedChild.context!,
axis: Axis.vertical,
);
if (focusedScrollable != null) {
final Iterable<FocusNode> filteredEligibleNodes = eligibleNodes.where(
(FocusNode node) => Scrollable.maybeOf(node.context!) == focusedScrollable,
(FocusNode node) =>
Scrollable.maybeOf(node.context!, axis: Axis.vertical) == focusedScrollable,
);
if (filteredEligibleNodes.isNotEmpty) {
eligibleNodes = filteredEligibleNodes;
Expand Down Expand Up @@ -879,9 +883,14 @@ mixin DirectionalFocusTraversalPolicyMixin on FocusTraversalPolicy {
if (eligibleNodes.isEmpty) {
break;
}
if (focusedScrollable != null && !focusedScrollable.position.atEdge) {
final ScrollableState? focusedScrollable = Scrollable.maybeOf(
focusedChild.context!,
axis: Axis.horizontal,
);
if (focusedScrollable != null) {
final Iterable<FocusNode> filteredEligibleNodes = eligibleNodes.where(
(FocusNode node) => Scrollable.maybeOf(node.context!) == focusedScrollable,
(FocusNode node) =>
Scrollable.maybeOf(node.context!, axis: Axis.horizontal) == focusedScrollable,
);
if (filteredEligibleNodes.isNotEmpty) {
eligibleNodes = filteredEligibleNodes;
Expand Down
263 changes: 259 additions & 4 deletions packages/flutter/test/widgets/focus_traversal_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

import 'dart:ui';

import 'package:collection/collection.dart';
import 'package:flutter/foundation.dart';
import 'package:flutter/material.dart';
import 'package:flutter/services.dart';
Expand Down Expand Up @@ -2562,14 +2563,14 @@ void main() {
expect(controller.offset, equals(0.0));

// Go down until we hit the bottom of the visible area.
for (int i = 1; i <= 4; ++i) {
for (int i = 1; i <= 3; ++i) {
Copy link
Contributor

@Piinks Piinks Jul 29, 2025

Choose a reason for hiding this comment

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

Can you share why these tests change?

Copy link
Contributor Author

@romaingyh romaingyh Jul 30, 2025

Choose a reason for hiding this comment

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

These tests were ok before the PR but they were actually testing a bad behavior. I shared an explaination and two video before/after in the PR description

await tester.sendKeyEvent(LogicalKeyboardKey.arrowDown);
await tester.pump();
expect(controller.offset, equals(0.0), reason: 'Focusing item $i caused a scroll');
}

// Now keep going down, and the scrollable should scroll automatically.
for (int i = 5; i <= 10; ++i) {
for (int i = 4; i <= 10; ++i) {
await tester.sendKeyEvent(LogicalKeyboardKey.arrowDown);
await tester.pump();
final double expectedOffset = 100.0 * (i - 5) + 200.0;
Expand Down Expand Up @@ -2687,14 +2688,14 @@ void main() {
expect(controller.offset, equals(0.0));

// Go right until we hit the right of the visible area.
for (int i = 1; i <= 6; ++i) {
for (int i = 1; i <= 5; ++i) {
await tester.sendKeyEvent(LogicalKeyboardKey.arrowRight);
await tester.pump();
expect(controller.offset, equals(0.0), reason: 'Focusing item $i caused a scroll');
}

// Now keep going right, and the scrollable should scroll automatically.
for (int i = 7; i <= 10; ++i) {
for (int i = 6; i <= 10; ++i) {
await tester.sendKeyEvent(LogicalKeyboardKey.arrowRight);
await tester.pump();
final double expectedOffset = 100.0 * (i - 5);
Expand Down Expand Up @@ -2758,6 +2759,260 @@ void main() {
variant: KeySimulatorTransitModeVariant.all(),
);

testWidgets(
'Focus traversal with horizontal scrollables inside a vertical scrollable handles vertical navigation correctly',
(WidgetTester tester) async {
// Tester view size is 800x600.

const double cellHeight = 100;

const int rowCount = 10;
const int buttonsPerRow = 5;

// Create focus nodes for all elements.
final FocusNode stickyButtonNode = FocusNode(debugLabel: 'Sticky Button');
addTearDown(stickyButtonNode.dispose);

final List<List<FocusNode>> gridNodes = List<List<FocusNode>>.generate(
rowCount,
(int row) => List<FocusNode>.generate(
buttonsPerRow,
(int col) => FocusNode(debugLabel: 'Button $row-$col'),
),
);
addTearDown(() {
for (final FocusNode node in gridNodes.flattened) {
node.dispose();
}
});

final ScrollController verticalController = ScrollController();
addTearDown(verticalController.dispose);

final List<ScrollController> horizontalControllers = List<ScrollController>.generate(
rowCount,
(int index) => ScrollController(debugLabel: 'Horizontal Controller $index'),
);
addTearDown(() {
for (final ScrollController controller in horizontalControllers) {
controller.dispose();
}
});

await tester.pumpWidget(
MaterialApp(
home: Column(
children: <Widget>[
Focus(
focusNode: stickyButtonNode,
child: Container(height: cellHeight, color: Colors.blue),
),
Expanded(
child: ListView.separated(
controller: verticalController,
itemCount: rowCount,
separatorBuilder: (_, _) => const SizedBox(height: 32),
itemBuilder: (BuildContext context, int rowIndex) {
return SizedBox(
height: cellHeight,
child: ListView.builder(
controller: horizontalControllers[rowIndex],
scrollDirection: Axis.horizontal,
itemCount: buttonsPerRow,
itemBuilder: (BuildContext context, int colIndex) {
return Focus(
focusNode: gridNodes[rowIndex][colIndex],
child: Container(
width: cellHeight,
height: cellHeight,
color: Colors.primaries[rowIndex % Colors.primaries.length],
),
);
},
),
);
},
),
),
],
),
),
);

// Start by focusing the sticky button.
stickyButtonNode.requestFocus();
await tester.pump();
expect(stickyButtonNode.hasPrimaryFocus, isTrue);

// Navigate down to the first row - should focus one of the widgets in the first row.
await tester.sendKeyEvent(LogicalKeyboardKey.arrowDown);
await tester.pump();

// Find which column in the first row got focused.
int focusedColumn = -1;
for (int col = 0; col < buttonsPerRow; col++) {
if (gridNodes[0][col].hasPrimaryFocus) {
focusedColumn = col;
break;
}
}
expect(focusedColumn, greaterThanOrEqualTo(0)); // Ensure something in first row is focused.

// Navigate down through the rows.
for (int row = 1; row < rowCount; row++) {
await tester.sendKeyEvent(LogicalKeyboardKey.arrowDown);
await tester.pump();
expect(gridNodes[row][focusedColumn].hasPrimaryFocus, isTrue);
// Verify vertical scroll happened from the 5th row onwards (500px).
if (row >= 5) {
expect(verticalController.offset, greaterThan(0));
}
}

// Navigate back up - should go to previous rows, not sticky button.
for (int row = rowCount - 2; row >= 0; row--) {
await tester.sendKeyEvent(LogicalKeyboardKey.arrowUp);
await tester.pump();
expect(gridNodes[row][focusedColumn].hasPrimaryFocus, isTrue);
}

// Only now should we reach the sticky button.
await tester.sendKeyEvent(LogicalKeyboardKey.arrowUp);
await tester.pump();
expect(stickyButtonNode.hasPrimaryFocus, isTrue);
},
// https://github.com/flutter/flutter/issues/35347
skip: isBrowser,
Copy link
Contributor

@Piinks Piinks Aug 4, 2025

Choose a reason for hiding this comment

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

Why do these skip on browser? The analyzer failure is due to these not being attributed with comments s to why they are being skipped. See other skips for an example.

Copy link
Contributor Author

@romaingyh romaingyh Aug 4, 2025

Choose a reason for hiding this comment

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

I copy/pasted the test above which is a bit similar as a base for my test the one above is skipped on browser because of this #35347

Should I add the same comment ? How can I see If this issue break the new test ?

Copy link
Contributor

@Piinks Piinks Aug 7, 2025

Choose a reason for hiding this comment

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

Web tests run in CI here in the PR, if they are affected they should fail without the skip. Otherwise, there has to be a comment there pointing to the reason why the test is skipped, like the others. :)
This should resolve the analyzer failure here.

variant: KeySimulatorTransitModeVariant.all(),
);

testWidgets(
'Focus traversal with vertical scrollables inside a horizontal scrollable handles horizontal navigation correctly',
(WidgetTester tester) async {
// Tester view size is 800x600.

const double cellWidth = 100;

const int columnCount = 10;
const int buttonsPerColumn = 10;

// Create focus nodes for all elements.
final FocusNode stickyButtonNode = FocusNode(debugLabel: 'Sticky Button');
addTearDown(stickyButtonNode.dispose);

final List<List<FocusNode>> gridNodes = List<List<FocusNode>>.generate(
columnCount,
(int column) => List<FocusNode>.generate(
buttonsPerColumn,
(int row) => FocusNode(debugLabel: 'Button $column-$row'),
),
);
addTearDown(() {
for (final FocusNode node in gridNodes.flattened) {
node.dispose();
}
});

final ScrollController horizontalController = ScrollController();
addTearDown(horizontalController.dispose);

final List<ScrollController> verticalControllers = List<ScrollController>.generate(
columnCount,
(int index) => ScrollController(debugLabel: 'Vertical Controller $index'),
);
addTearDown(() {
for (final ScrollController controller in verticalControllers) {
controller.dispose();
}
});

await tester.pumpWidget(
MaterialApp(
home: Row(
children: <Widget>[
Focus(
focusNode: stickyButtonNode,
child: Container(width: cellWidth, color: Colors.blue),
),
Expanded(
child: ListView.separated(
scrollDirection: Axis.horizontal,
controller: horizontalController,
itemCount: columnCount,
separatorBuilder: (_, _) => const SizedBox(width: 32),
itemBuilder: (BuildContext context, int columnIndex) {
return SizedBox(
width: cellWidth,
child: ListView.builder(
controller: verticalControllers[columnIndex],
itemCount: buttonsPerColumn,
itemBuilder: (BuildContext context, int rowIndex) {
return Focus(
focusNode: gridNodes[columnIndex][rowIndex],
child: Container(
width: cellWidth,
height: cellWidth,
color: Colors.red,
),
);
},
),
);
},
),
),
],
),
),
);

// Start by focusing the sticky button.
stickyButtonNode.requestFocus();
await tester.pump();
expect(stickyButtonNode.hasPrimaryFocus, isTrue);

// Navigate right to the first column - should focus one of the widgets in the first column.
await tester.sendKeyEvent(LogicalKeyboardKey.arrowRight);
await tester.pump();

// Find which row in the first column got focused.
int focusedRow = -1;
for (int row = 0; row < buttonsPerColumn; row++) {
if (gridNodes[0][row].hasPrimaryFocus) {
focusedRow = row;
break;
}
}
expect(focusedRow, greaterThanOrEqualTo(0)); // Ensure something in first column is focused.

// Navigate right through the columns.
for (int column = 1; column < columnCount; column++) {
await tester.sendKeyEvent(LogicalKeyboardKey.arrowRight);
await tester.pump();
expect(gridNodes[column][focusedRow].hasPrimaryFocus, isTrue);
// Verify horizontal scroll happened from the 7th column onwards (700px).
if (column >= 6) {
expect(horizontalController.offset, greaterThan(0));
}
}

// Navigate back left - should go to previous columns, not sticky button.
for (int column = columnCount - 2; column >= 0; column--) {
await tester.sendKeyEvent(LogicalKeyboardKey.arrowLeft);
await tester.pump();
expect(gridNodes[column][focusedRow].hasPrimaryFocus, isTrue);
}

// Only now should we reach the sticky button.
await tester.sendKeyEvent(LogicalKeyboardKey.arrowLeft);
await tester.pump();
expect(stickyButtonNode.hasPrimaryFocus, isTrue);
},
// https://github.com/flutter/flutter/issues/35347
skip: isBrowser,
variant: KeySimulatorTransitModeVariant.all(),
);

testWidgets(
'Arrow focus traversal actions can be re-enabled for text fields.',
(WidgetTester tester) async {
Expand Down