From 1a3379d5714d3242336cf27d5ad194a4c5886a9e Mon Sep 17 00:00:00 2001
From: Greg Spencer <gspencergoog@users.noreply.github.com>
Date: Mon, 27 Jan 2020 16:13:02 -0800
Subject: [PATCH] Fix semantic sort name (#48920)

---
 .../flutter/lib/src/semantics/semantics.dart  | 79 +++++++++++--------
 .../test/semantics/semantics_test.dart        | 40 +++++++---
 2 files changed, 74 insertions(+), 45 deletions(-)

diff --git a/packages/flutter/lib/src/semantics/semantics.dart b/packages/flutter/lib/src/semantics/semantics.dart
index 609219e360..148a473aa8 100644
--- a/packages/flutter/lib/src/semantics/semantics.dart
+++ b/packages/flutter/lib/src/semantics/semantics.dart
@@ -3981,30 +3981,18 @@ String _concatStrings({
   return '$thisString\n$nestedLabel';
 }
 
-/// Base class for all sort keys for [Semantics] accessibility traversal order
-/// sorting.
+/// Base class for all sort keys for [SemanticsProperties.sortKey] accessibility
+/// traversal order sorting.
 ///
-/// Only keys of the same type and having matching [name]s are compared. If a
-/// list of sibling [SemanticsNode]s contains keys that are not comparable with
-/// each other the list is first sorted using the default sorting algorithm.
-/// Then the nodes are broken down into groups by moving comparable nodes
-/// towards the _earliest_ node in the group. Finally each group is sorted by
-/// sort key and the resulting list is made by concatenating the sorted groups
-/// back.
+/// Sort keys are sorted by [name], then by the comparison that the subclass
+/// implements. If [SemanticsProperties.sortKey] is specified, sort keys within
+/// the same semantic group must all be of the same type.
 ///
-/// For example, let's take nodes (C, D, B, E, A, F). Let's assign node A key 1,
-/// node B key 2, node C key 3. Let's also assume that the default sort order
-/// leaves the original list intact. Because nodes A, B, and C, have comparable
-/// sort key, they will form a group by pulling all nodes towards the earliest
-/// node, which is C. The result is group (C, B, A). The remaining nodes D, E,
-/// F, form a second group with sort key being `null`. The first group is sorted
-/// using their sort keys becoming (A, B, C). The second group is left as is
-/// because it does not specify sort keys. Then we concatenate the two groups -
-/// (A, B, C) and (D, E, F) - into the final (A, B, C, D, E, F).
+/// Keys with no [name] are compared to other keys with no [name], and will
+/// be traversed before those with a [name].
 ///
-/// Because of the complexity introduced by incomparable sort keys among sibling
-/// nodes, it is recommended to either use comparable keys for all nodes, or
-/// use null for all of them, leaving the sort order to the default algorithm.
+/// If no sort key is applied to a semantics node, then it will be ordered using
+/// a platform dependent default algorithm.
 ///
 /// See also:
 ///
@@ -4014,17 +4002,35 @@ abstract class SemanticsSortKey extends Diagnosticable implements Comparable<Sem
   /// const constructors so that they can be used in const expressions.
   const SemanticsSortKey({this.name});
 
-  /// An optional name that will make this sort key only order itself
-  /// with respect to other sort keys of the same [name], as long as
-  /// they are of the same [runtimeType].
+  /// An optional name that will group this sort key with other sort keys of the
+  /// same [name].
+  ///
+  /// Sort keys must have the same `runtimeType` when compared.
+  ///
+  /// Keys with no [name] are compared to other keys with no [name], and will
+  /// be traversed before those with a [name].
   final String name;
 
   @override
   int compareTo(SemanticsSortKey other) {
-    // The sorting algorithm must not compare incomparable keys.
-    assert(runtimeType == other.runtimeType);
-    assert(name == other.name);
-    return doCompare(other);
+    // Sort by name first and then subclass ordering.
+    assert(runtimeType == other.runtimeType, 'Semantics sort keys can only be compared to other sort keys of the same type.');
+
+    // Defer to the subclass implementation for ordering only if the names are
+    // identical (or both null).
+    if (name == other.name) {
+      return doCompare(other);
+    }
+
+    // Keys that don't have a name are sorted together and come before those with
+    // a name.
+    if (name == null && other.name != null) {
+      return -1;
+    } else if (name != null && other.name == null) {
+      return 1;
+    }
+
+    return name.compareTo(other.name);
   }
 
   /// The implementation of [compareTo].
@@ -4052,14 +4058,22 @@ abstract class SemanticsSortKey extends Diagnosticable implements Comparable<Sem
 /// The [OrdinalSortKey] compares itself with other [OrdinalSortKey]s
 /// to sort based on the order it is given.
 ///
-/// The ordinal value `order` is typically a whole number, though it can be
+/// [OrdinalSortKey]s are sorted by the optional [name], then by their [order].
+/// If [SemanticsProperties.sortKey] is a [OrdinalSortKey], then all the other
+/// speficied sort keys in the same semantics group must also be
+/// [OrdinalSortKey]s.
+///
+/// Keys with no [name] are compared to other keys with no [name], and will
+/// be traversed before those with a [name].
+///
+/// The ordinal value [order] is typically a whole number, though it can be
 /// fractional, e.g. in order to fit between two other consecutive whole
 /// numbers. The value must be finite (it cannot be [double.nan],
 /// [double.infinity], or [double.negativeInfinity]).
 class OrdinalSortKey extends SemanticsSortKey {
-  /// Creates a semantics sort key that uses a [double] as its key value.
+  /// Creates a const semantics sort key that uses a [double] as its key value.
   ///
-  /// The [order] must be a finite number.
+  /// The [order] must be a finite number, and must not be null.
   const OrdinalSortKey(
     this.order, {
     String name,
@@ -4073,7 +4087,8 @@ class OrdinalSortKey extends SemanticsSortKey {
   /// the order in which this node is traversed by the platform's accessibility
   /// services.
   ///
-  /// Lower values will be traversed first.
+  /// Lower values will be traversed first. Keys with the same [name] will be
+  /// grouped together and sorted by name first, and then sorted by [order].
   final double order;
 
   @override
diff --git a/packages/flutter/test/semantics/semantics_test.dart b/packages/flutter/test/semantics/semantics_test.dart
index 527c336514..2aa8965bc4 100644
--- a/packages/flutter/test/semantics/semantics_test.dart
+++ b/packages/flutter/test/semantics/semantics_test.dart
@@ -247,36 +247,50 @@ void main() {
     expect(() {
       const OrdinalSortKey(0.0).compareTo(const CustomSortKey(0.0));
     }, throwsAssertionError);
-
-    // Different names.
-    expect(() {
-      const OrdinalSortKey(0.0, name: 'a').compareTo(const OrdinalSortKey(0.0, name: 'b'));
-    }, throwsAssertionError);
   });
 
-  test('OrdinalSortKey compares correctly', () {
+  test('OrdinalSortKey compares correctly when names are the same', () {
     const List<List<SemanticsSortKey>> tests = <List<SemanticsSortKey>>[
       <SemanticsSortKey>[OrdinalSortKey(0.0), OrdinalSortKey(0.0)],
       <SemanticsSortKey>[OrdinalSortKey(0.0), OrdinalSortKey(1.0)],
       <SemanticsSortKey>[OrdinalSortKey(1.0), OrdinalSortKey(0.0)],
       <SemanticsSortKey>[OrdinalSortKey(1.0), OrdinalSortKey(1.0)],
+      <SemanticsSortKey>[OrdinalSortKey(0.0, name: 'a'), OrdinalSortKey(0.0, name: 'a')],
+      <SemanticsSortKey>[OrdinalSortKey(0.0, name: 'a'), OrdinalSortKey(1.0, name: 'a')],
+      <SemanticsSortKey>[OrdinalSortKey(1.0, name: 'a'), OrdinalSortKey(0.0, name: 'a')],
+      <SemanticsSortKey>[OrdinalSortKey(1.0, name: 'a'), OrdinalSortKey(1.0, name: 'a')],
     ];
-    final List<int> expectedResults = <int>[0, -1, 1, 0];
+    final List<int> expectedResults = <int>[0, -1, 1, 0, 0, -1, 1, 0];
     assert(tests.length == expectedResults.length);
     final List<int> results = <int>[
       for (final List<SemanticsSortKey> tuple in tests) tuple[0].compareTo(tuple[1]),
     ];
     expect(results, orderedEquals(expectedResults));
+
+    // Differing types should throw an assertion.
+    expect(() => const OrdinalSortKey(0.0).compareTo(const CustomSortKey(0.0)), throwsAssertionError);
   });
 
-  test('OrdinalSortKey compares correctly', () {
+  test('OrdinalSortKey compares correctly when the names are different', () {
     const List<List<SemanticsSortKey>> tests = <List<SemanticsSortKey>>[
-      <SemanticsSortKey>[OrdinalSortKey(0.0), OrdinalSortKey(0.0)],
-      <SemanticsSortKey>[OrdinalSortKey(0.0), OrdinalSortKey(1.0)],
-      <SemanticsSortKey>[OrdinalSortKey(1.0), OrdinalSortKey(0.0)],
-      <SemanticsSortKey>[OrdinalSortKey(1.0), OrdinalSortKey(1.0)],
+      <SemanticsSortKey>[OrdinalSortKey(0.0), OrdinalSortKey(0.0, name: 'bar')],
+      <SemanticsSortKey>[OrdinalSortKey(0.0), OrdinalSortKey(1.0, name: 'bar')],
+      <SemanticsSortKey>[OrdinalSortKey(1.0), OrdinalSortKey(0.0, name: 'bar')],
+      <SemanticsSortKey>[OrdinalSortKey(1.0), OrdinalSortKey(1.0, name: 'bar')],
+      <SemanticsSortKey>[OrdinalSortKey(0.0, name: 'foo'), OrdinalSortKey(0.0)],
+      <SemanticsSortKey>[OrdinalSortKey(0.0, name: 'foo'), OrdinalSortKey(1.0)],
+      <SemanticsSortKey>[OrdinalSortKey(1.0, name: 'foo'), OrdinalSortKey(0.0)],
+      <SemanticsSortKey>[OrdinalSortKey(1.0, name: 'foo'), OrdinalSortKey(1.0)],
+      <SemanticsSortKey>[OrdinalSortKey(0.0, name: 'foo'), OrdinalSortKey(0.0, name: 'bar')],
+      <SemanticsSortKey>[OrdinalSortKey(0.0, name: 'foo'), OrdinalSortKey(1.0, name: 'bar')],
+      <SemanticsSortKey>[OrdinalSortKey(1.0, name: 'foo'), OrdinalSortKey(0.0, name: 'bar')],
+      <SemanticsSortKey>[OrdinalSortKey(1.0, name: 'foo'), OrdinalSortKey(1.0, name: 'bar')],
+      <SemanticsSortKey>[OrdinalSortKey(0.0, name: 'bar'), OrdinalSortKey(0.0, name: 'foo')],
+      <SemanticsSortKey>[OrdinalSortKey(0.0, name: 'bar'), OrdinalSortKey(1.0, name: 'foo')],
+      <SemanticsSortKey>[OrdinalSortKey(1.0, name: 'bar'), OrdinalSortKey(0.0, name: 'foo')],
+      <SemanticsSortKey>[OrdinalSortKey(1.0, name: 'bar'), OrdinalSortKey(1.0, name: 'foo')],
     ];
-    final List<int> expectedResults = <int>[0, -1, 1, 0];
+    final List<int> expectedResults = <int>[ -1, -1, -1, -1, 1, 1, 1, 1, 1, 1, 1, 1, -1, -1, -1, -1];
     assert(tests.length == expectedResults.length);
     final List<int> results = <int>[
       for (final List<SemanticsSortKey> tuple in tests) tuple[0].compareTo(tuple[1]),