Keyboard scrolling of Scrollable #45019

gspencergoog · 2019-11-16T02:15:57Z

Description

This adds the ability to scroll and page up/down in a Scrollable using the keyboard. Currently, the macOS bindings use Platform.isMacOS as a check, but we'll switch that to be defaultTargetPlatform == TargetPlatform.macOS once that exists.

Related Issues

Fixes Support for scrollables with keyboard bindings #31956

Tests

Added a test for keyboard scrolling both horizontally and vertically.

Breaking Change

No, this is not a breaking change.

goderbauer · 2019-11-18T18:24:23Z

Looks like cirrus is unhappy :(

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

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

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

goderbauer · 2019-11-18T18:47:52Z

packages/flutter/test/widgets/scrollable_test.dart

@@ -267,4 +270,85 @@ void main() {

    expect(getScrollOffset(tester), 20.0);
  });
+
+  testWidgets('Vertical scrollables are scrolled when activated via keyboard.', (WidgetTester tester) async {


Can you add more tests to ensure that it scrolls in the right direction when "reverse: true" and when a "center" sliver is used?

OK, I added some, which highlighted that I needed to change how I specified direction, so it's now an Axis and a reverse bool instead of an AxisDirection, so it can be resolved with the current directional environment.

What do you mean by "center sliver"? I'm not sure what that should look like.

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

gspencergoog · 2019-11-20T22:44:37Z

I've cleaned this up so that there isn't a new public API on ScrollableState, which didn't turn out to be necessary. PTAL.

gspencergoog · 2019-11-20T22:49:03Z

@goderbauer, can you respond to my comment so I can address yours?

dnfield

LGTM, would defer to @goderbauer on the requested test though

gspencergoog · 2019-11-22T20:26:55Z

Hi, @goderbauer, OK, I think I added the sliver test that we talked about. Please take a look.

goderbauer · 2019-11-25T18:03:52Z

Cirrus looks unhappy.

goderbauer · 2019-11-25T18:07:50Z

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

-    LogicalKeySet(LogicalKeyboardKey.arrowUp): const DirectionalFocusIntent(TraversalDirection.up),
+
+    // Directional keyboard traversal. Not available on web.
+    if (!kIsWeb)


That each line gets its own if reads awkward...

I wonder if this would look nicer with just one if and then the spread operator? Not sure, though.

Yes, it does! Done.

goderbauer · 2019-11-25T18:08:24Z

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

+
+    // Keyboard scrolling.
+    // TODO(gspencergoog): Convert all of the Platform.isMacOS checks to be
+    // defaultTargetPlatform == TargetPlatform.macOS, once that exists.


nit: link to the issue for this?

goderbauer · 2019-11-25T18:10:59Z

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

+/// [ScrollIncrementCalculator] function on a [Scrollable].
+///
+/// {@template flutter.widgets.scrollable.scroll_increment_type.intent}
+/// This indicates the *intent* of the scroll, not necessarily the size. Not all


Nice explanation!

goderbauer · 2019-11-25T18:11:48Z

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

+  /// Indicates that the [ScrollIncrementCalculator] should return the scroll
+  /// distance it should move when the user requests to scroll by a "line".
+  ///
+  /// The distance a "line" scrolls is really refers to what should happen when


Does this sentence have a duplicated verb? "..is really refers to..." sounds strange...

Removed "is really" (and yes, it was a typo).

goderbauer · 2019-11-25T18:12:10Z

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

+  /// Indicates that the [ScrollIncrementCalculator] should return the scroll
+  /// distance it should move when the user requests to scroll by a "page".
+  ///
+  /// The distance a "page" scrolls is really refers to what should happen when


"... is really refers to..." ?

goderbauer · 2019-11-25T18:13:01Z

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

+/// specified.
+class ScrollIntent extends Intent {
+  /// Creates a const [ScrollIntent] that requests scrolling in the given
+  /// [axis], with the given [type]. If [reversed] is specified, then the scroll


nit: first paragraph should be a one-sentence summary. Move the second sentence to a separate paragraph?

goderbauer · 2019-11-25T18:14:26Z

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

+  /// widget.
+  final Axis axis;
+
+  /// Whether or not the natural scroll direction for this scroll operation


It's not really clear to me from the doc why we have this property and what it actually means....

Also, what is the "natural scroll direction"? Is that what apple calls natural scrolling? Are we making up our own definition?

goderbauer · 2019-11-25T18:15:35Z

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

+
+  /// The direction in which to scroll the scrollable containing the focused
+  /// widget.
+  final Axis axis;


Don't you need an AxisDirection? e.g. wether to scroll up or down?

Oh, is that supposed to be indicated by the reversed property?

I switched back to AxisDirection and made sure it worked. I never did discover what made me switch away from it in the first place: maybe an old design was flawed? I also did add a test for RTL locales to make sure that the result was correct there too.

goderbauer · 2019-11-25T18:31:15Z

packages/flutter/test/widgets/scrollable_test.dart

+        ),
+        home: CustomScrollView(
+          controller: controller,
+          physics: const NeverScrollableScrollPhysics(),


Hm, this seems odd. It has NeverScrollableScrollPhysics, but you can still scroll it with the keyboard? That sounds like a bug?

Yep, it was. I added a test for this, and fixed the bug.

goderbauer · 2019-11-25T18:32:55Z

packages/flutter/test/widgets/scrollable_test.dart

+          controller: controller,
+          physics: const NeverScrollableScrollPhysics(),
+          slivers: <Widget>[
+            SliverToBoxAdapter(


Instead of making the test based on scrolloffsets (which are difficult to comprehend since the zero scroll offset may be at the top or bottom) maybe use multiple slivers with text (e.g. sliver 1, sliver 2, etc). and then assert that the expected slivers are currently visible?

Done. All based on the rects of the items now.

gspencergoog

OK, @goderbauer, I think I addressed everything we talked about. PTAL.

gspencergoog · 2019-11-25T18:52:20Z

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

-    LogicalKeySet(LogicalKeyboardKey.arrowUp): const DirectionalFocusIntent(TraversalDirection.up),
+
+    // Directional keyboard traversal. Not available on web.
+    if (!kIsWeb)


Yes, it does! Done.

gspencergoog · 2019-11-25T18:53:02Z

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

+
+    // Keyboard scrolling.
+    // TODO(gspencergoog): Convert all of the Platform.isMacOS checks to be
+    // defaultTargetPlatform == TargetPlatform.macOS, once that exists.


gspencergoog · 2019-11-25T18:54:22Z

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

+  /// Indicates that the [ScrollIncrementCalculator] should return the scroll
+  /// distance it should move when the user requests to scroll by a "line".
+  ///
+  /// The distance a "line" scrolls is really refers to what should happen when


Removed "is really" (and yes, it was a typo).

gspencergoog · 2019-11-25T18:54:27Z

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

+  /// Indicates that the [ScrollIncrementCalculator] should return the scroll
+  /// distance it should move when the user requests to scroll by a "page".
+  ///
+  /// The distance a "page" scrolls is really refers to what should happen when


gspencergoog · 2019-11-25T18:54:54Z

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

+/// specified.
+class ScrollIntent extends Intent {
+  /// Creates a const [ScrollIntent] that requests scrolling in the given
+  /// [axis], with the given [type]. If [reversed] is specified, then the scroll


gspencergoog · 2019-11-25T22:17:44Z

packages/flutter/test/widgets/scrollable_test.dart

+        ),
+        home: CustomScrollView(
+          controller: controller,
+          physics: const NeverScrollableScrollPhysics(),


Yep, it was. I added a test for this, and fixed the bug.

goderbauer

LGTM

goderbauer · 2019-11-25T23:35:50Z

packages/flutter/test/widgets/scrollable_test.dart

+    await tester.sendKeyEvent(LogicalKeyboardKey.arrowLeft);
+    await tester.sendKeyUpEvent(modifierKey);
+    await tester.pumpAndSettle();
+


SHould there be one last expect on the rect to ensure that Box 0 is back to where it started?

goderbauer · 2019-11-25T23:37:18Z

packages/flutter/test/widgets/scrollable_test.dart

+    await tester.sendKeyEvent(LogicalKeyboardKey.arrowRight);
+    await tester.sendKeyUpEvent(modifierKey);
+    await tester.pumpAndSettle();
+


gspencergoog added framework flutter/packages/flutter repository. See also f: labels. a: desktop Running on desktop labels Nov 16, 2019

gspencergoog requested a review from goderbauer November 16, 2019 02:15

googlebot added the cla: yes label Nov 16, 2019

gspencergoog changed the title ~~Keyboard scroll~~ Keyboard scrolling of Scrollable Nov 16, 2019

gspencergoog force-pushed the keyboard_scroll branch from 0cf9a29 to 5add30c Compare November 18, 2019 18:38

goderbauer reviewed Nov 18, 2019

View reviewed changes

gspencergoog force-pushed the keyboard_scroll branch 4 times, most recently from 988ba7f to 28c9bb9 Compare November 19, 2019 08:28

dnfield reviewed Nov 20, 2019

View reviewed changes

packages/flutter/lib/src/widgets/scrollable.dart Outdated Show resolved Hide resolved

dnfield reviewed Nov 20, 2019

View reviewed changes

packages/flutter/lib/src/widgets/scrollable.dart Outdated Show resolved Hide resolved

dnfield reviewed Nov 20, 2019

View reviewed changes

packages/flutter/lib/src/widgets/scrollable.dart Show resolved Hide resolved

gspencergoog force-pushed the keyboard_scroll branch from 28c9bb9 to e35888e Compare November 20, 2019 18:50

dnfield approved these changes Nov 20, 2019

View reviewed changes

gspencergoog force-pushed the keyboard_scroll branch from 302bfbf to 9e90aaa Compare November 22, 2019 20:25

gspencergoog force-pushed the keyboard_scroll branch from 9e90aaa to ed6f02b Compare November 25, 2019 16:12

goderbauer reviewed Nov 25, 2019

View reviewed changes

gspencergoog commented Nov 25, 2019

View reviewed changes

gspencergoog force-pushed the keyboard_scroll branch 2 times, most recently from 5ef40bd to ed91145 Compare November 25, 2019 22:29

goderbauer approved these changes Nov 25, 2019

View reviewed changes

Add keyboard control of scrollables.

0145745

gspencergoog added 11 commits November 25, 2019 17:03

Add to scroll configuration

bbf99c6

Switch to using a ScrollIncrementCalculator function.

0960ab3

Add tests

ef51950

Add macOS specific bindings

81d08e9

Review changes

fb304b3

Added tests, changed to using Axis+reverse instead of AxisDirection

7804aa5

Added null checks for being called before first layout

c9f71e6

Cleaning up

4a94d93

Add 'center sliver' test.

180adda

Review Changes

736bda2

Move to AxisDirection instead of Axis, add test for RTL

093509b

gspencergoog force-pushed the keyboard_scroll branch from ed91145 to 093509b Compare November 26, 2019 01:16

gspencergoog merged commit 0190e40 into flutter:master Nov 26, 2019

gspencergoog deleted the keyboard_scroll branch January 16, 2020 22:11

github-actions bot locked as resolved and limited conversation to collaborators Aug 2, 2021

Keyboard scrolling of Scrollable #45019

Keyboard scrolling of Scrollable #45019

Uh oh!

Conversation

gspencergoog commented Nov 16, 2019

Description

Related Issues

Tests

Breaking Change

Uh oh!

goderbauer commented Nov 18, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gspencergoog commented Nov 20, 2019

Uh oh!

gspencergoog commented Nov 20, 2019

Uh oh!

dnfield left a comment

Choose a reason for hiding this comment

Uh oh!

gspencergoog commented Nov 22, 2019

Uh oh!

goderbauer commented Nov 25, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

goderbauer Nov 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gspencergoog left a comment

goderbauer Nov 25, 2019 •

edited

Loading