Add support for custom accessibility actions to android and iOS. by jonahwilliams · Pull Request #18882 · flutter/flutter

jonahwilliams · 2018-06-27T20:00:41Z

…1y action

jonahwilliams · 2018-07-18T23:29:38Z

@Hixie @goderbauer this PR is blocking customer mulligan from having accessibility for the reorder-able list.

Hixie · 2018-07-19T00:24:34Z

packages/flutter/lib/src/rendering/proxy_box.dart

      config.onDidLoseAccessibilityFocus = _performDidLoseAccessibilityFocus;
+    if (customAccessibilityActions != null) {
+      config.customAccessibilityActions = _customAccessibilityActions;
+    }


please be consistent with nearby code regarding braces, to make the code easier to read

Hixie · 2018-07-19T00:25:14Z

packages/flutter/lib/src/semantics/semantics.dart

  String toString() => '$runtimeType($name)';
 }

+/// A custom accessibility action.


this is not a helpful description since it's exactly the class name. :-)

The first line should be a brief but still useful description. It appears in documentation summaries (e.g. in the library-level docs).

goderbauer · 2018-07-18T23:31:31Z

examples/flutter_gallery/lib/demo/material/leave_behind_demo.dart

    });
  }

+


remove extra line?

goderbauer · 2018-07-18T23:36:25Z

examples/flutter_gallery/lib/demo/material/leave_behind_demo.dart

+    return new Semantics(
+      customAccessibilityActions: <CustomAccessibilityAction, VoidCallback>{
+        const CustomAccessibilityAction(label: 'Archive'): () {
+          setState(() {


Maybe factor this out into a method so the same code can be used for the dismisable (when the user swipes) and the custom a11y action?

goderbauer · 2018-07-18T23:37:02Z

examples/flutter_gallery/lib/demo/material/leave_behind_demo.dart

+          )
        )
-      )
+        ),


nit: is formatting off here somehow? Looks strange to have to closing ) with the same indentation?

goderbauer · 2018-07-18T23:41:13Z

packages/flutter/lib/src/rendering/proxy_box.dart


+  /// A map from each supported [CustomAccessibilityAction] to a provided handler.
+  /// 
+  /// The handler associated with each custom action is invoked whenever a


This doc comment seems to describe impl details? Maybe rephrase it a little and focus on what I as a user of this could do with it?

Hixie · 2018-07-19T00:29:10Z

packages/flutter/lib/src/semantics/semantics.dart

+/// Custom accessibility actions can be provided to make complex user
+/// interactions more accessible. A drag and drop list, for example, may
+/// require a user to press and hold an item to move it.  This may be
+/// difficult for a user interacting with your app using a hardware switch. A


avoid refering to "your" app; the reader may not be the person whose app they're trying to understand
Prefer something like "For instance, if an application has a drag-and-drop list that requires the user to press and hold an item to move it, users interacting with the application using a hardware switch may have difficulty."

I incorporated this suggestion and did a pass on the entire doc comment.

Hixie · 2018-07-19T00:29:32Z

packages/flutter/lib/src/semantics/semantics.dart

+/// interactions more accessible. A drag and drop list, for example, may
+/// require a user to press and hold an item to move it.  This may be
+/// difficult for a user interacting with your app using a hardware switch. A
+/// custom accessibility action allows you to specifiy actions such as 


same here, "allows the application to specify..."

(also, typo on specify)

Hixie · 2018-07-19T00:29:43Z

packages/flutter/lib/src/semantics/semantics.dart

+/// In Android, these actions are presented in the local context menu. In iOS,
+/// these are in the radial context menu.
+class CustomAccessibilityAction {
+


nit: extraneous blank line

Hixie · 2018-07-19T00:30:03Z

packages/flutter/lib/src/semantics/semantics.dart

+
+  /// Creates a new [CustomAccessibilityAction].
+  /// 
+  /// [label] must not be null or the empty string.


consider starting with "The [label] argument must..." to avoid starting with a lowercase letter.

Hixie · 2018-07-19T00:30:25Z

packages/flutter/lib/src/semantics/semantics.dart

+  /// 
+  /// [label] must not be null or the empty string.
+  const CustomAccessibilityAction({@required this.label}) 
+    : assert(label != null && label != '');


prefer splitting asserts into multiple asserts rather than chaining expressions with &&, it makes it easier to see what went wrong when they fire

Hixie · 2018-07-19T00:31:02Z

packages/flutter/lib/src/semantics/semantics.dart

+  static int getIdentifier(CustomAccessibilityAction action) {
+    int result = _ids[action];
+    if (result == null) {
+      result =_nextId++;


nit: missing space after =

Hixie · 2018-07-19T00:35:02Z

packages/flutter/lib/src/semantics/semantics.dart

        && typedOther.scrollExtentMin == scrollExtentMin
-        && typedOther.transform == transform;
+        && typedOther.transform == transform
+        && setEquals(typedOther.customAccessibilityActions, customAccessibilityActions);


yikes, this is one expensive == method

I think replacing the set of custom actions with a sorted list of ints will make this faster...

Hixie · 2018-07-19T00:35:41Z

packages/flutter/lib/src/semantics/semantics.dart

+  /// 
+  /// See also:
+  /// 
+  ///   * [CustomAccessibilityAction], for an explaination of custom actions.


typo: explanation

Hixie · 2018-07-19T00:35:56Z

packages/flutter/lib/src/semantics/semantics.dart

+  /// A map from each supported [CustomAccessibilityAction] to a provided handler.
+  /// 
+  /// The handler associated with each custom action is invoked whenever a
+  /// semantics event of type [SemanticsEvent.customEvent] is recieved. The


typo: received

Hixie · 2018-07-19T00:36:12Z

packages/flutter/lib/src/semantics/semantics.dart


+  /// A map from each supported [CustomAccessibilityAction] to a provided handler.
+  /// 
+  /// The handler associated with each custom action is invoked whenever a


nit: prefer "called" over "invoked" (see style guide)

jonahwilliams · 2018-07-19T16:58:35Z

I think I addressed most of the comments, PTAL

jonahwilliams · 2018-07-19T22:23:16Z

Since we're going to tie these in with renaming hint text for actions on Android (in a future PR), i've updated the name to CustomSemanticsAction to make the relationship with SemanticsAction more obvious.

goderbauer

LGTM

goderbauer · 2018-07-19T22:37:17Z

packages/flutter/lib/src/semantics/semantics.dart


  @override
-  int get hashCode => ui.hashValues(flags, actions, label, value, increasedValue, decreasedValue, hint, textDirection, rect, tags, textSelection, scrollPosition, scrollExtentMax, scrollExtentMin, transform);
+  int get hashCode => ui.hashValues(flags, actions, label, value, increasedValue, decreasedValue, hint, textDirection, rect, tags, textSelection, scrollPosition, scrollExtentMax, scrollExtentMin, transform, customSemanticsActionIds);


nit: maybe its time to add some line breaks to this line?

goderbauer · 2018-07-19T22:38:01Z

packages/flutter/lib/src/semantics/semantics.dart


+  /// A map from each supported [CustomSemanticsAction] to a provided handler.
+  /// 
+  /// The handler associated with each custom action is called whenever a


Should this doc also be updated to the version you are using above?

Do you mean the CustomSemanticsAction doc comment?

goderbauer · 2018-07-19T22:40:26Z

packages/flutter/test/semantics/semantics_test.dart

      '   mergeAllDescendantsIntoThisNode: false\n'
      '   Rect.fromLTRB(0.0, 0.0, 0.0, 0.0)\n'
      '   actions: []\n'
+      '   customActions: []\n'


Maybe add a test that ensures that custom actions are properly printed when the Semantics tree is printed out?

Jonah Williams and others added 10 commits June 22, 2018 14:04

initial support for local context actions

45850c1

wiring of semantics objects

b080b94

wip

70cab09

wip

101bc80

merge

ec4c009

cleanup of debugging features

2f97c79

clean ups and documentation fixes

7708224

remove stray imports and copy pods

e7feade

rename path in copy pods

33e3253

add more docs

8432d76

googlebot added the cla: yes label Jun 27, 2018

Jonah Williams and others added 5 commits June 27, 2018 13:00

Update pubspec.yaml

69e693b

Merge branch 'master' of github.com:flutter/flutter into contextAction2

ca24dea

add custom actions to leave behind list

c5da122

merge with master

731f842

Fix semantics test and add simple canonicalization test for custom a1…

c769ca3

…1y action

jonahwilliams requested a review from goderbauer July 18, 2018 19:15

Hixie reviewed Jul 19, 2018

View reviewed changes

goderbauer reviewed Jul 19, 2018

View reviewed changes

Hixie reviewed Jul 19, 2018

View reviewed changes

jonahwilliams added 5 commits July 18, 2018 20:09

Address review comments

0bce42f

fix typo

65a8a9a

even more typos

5eac906

cleanup custom action documentation

0b47df0

add plural

ea7f34b

DaveShuckerow mentioned this pull request Jul 19, 2018

Reorderable list widget and Material demo #18374

Merged

jonahwilliams added 2 commits July 19, 2018 15:17

renamed to custom semantics actions before it is too late

2193cff

and replace the rest

f642812

add missing test files

7c27ba5

goderbauer approved these changes Jul 19, 2018

View reviewed changes

add test and split up ui values line

b331196

jonahwilliams merged commit d2d17ab into flutter:master Jul 20, 2018

jonahwilliams deleted the contextAction2 branch July 20, 2018 03:47

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

Conversation

jonahwilliams commented Jun 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonahwilliams commented Jul 18, 2018

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!

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!

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!

jonahwilliams commented Jul 19, 2018

Uh oh!

jonahwilliams commented Jul 19, 2018

Uh oh!

goderbauer left a comment

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

jonahwilliams commented Jun 27, 2018 •

edited

Loading