Move geometric sort for semantics to the framework side. (flutter#14539)

gspencergoog · web-flow · commit 7be0bd1ce900 · 2018-02-12T20:53:32.000-08:00
This adds geometric sort ordering back in for semantics nodes that don't have a sort order defined.

With this change, widgets that either have no sort order, or have an equivalent sort order, will be compared geometrically. The comparison is between the two starting corners, so it is TextDirection-aware: parent nodes that are set to have LTR text will compare upper left corners of their children, and upper right when set to RTL.

Also fixed a bug in the Transform widget that didn't mark modified nodes as needing a semantics update.
diff --git a/packages/flutter/lib/src/rendering/proxy_box.dart b/packages/flutter/lib/src/rendering/proxy_box.dart
@@ -1895,7 +1895,7 @@ class RenderTransform extends RenderProxyBox {
     this.origin = origin;
   }
 
-  /// The origin of the coordinate system (relative to the upper left corder of
+  /// The origin of the coordinate system (relative to the upper left corner of
   /// this render object) in which to apply the matrix.
   ///
   /// Setting an origin is equivalent to conjugating the transform matrix by a
@@ -1907,6 +1907,7 @@ class RenderTransform extends RenderProxyBox {
       return;
     _origin = value;
     markNeedsPaint();
+    markNeedsSemanticsUpdate();
   }
 
   /// The alignment of the origin, relative to the size of the box.
@@ -1927,6 +1928,7 @@ class RenderTransform extends RenderProxyBox {
       return;
     _alignment = value;
     markNeedsPaint();
+    markNeedsSemanticsUpdate();
   }
 
   /// The text direction with which to resolve [alignment].
@@ -1940,6 +1942,7 @@ class RenderTransform extends RenderProxyBox {
       return;
     _textDirection = value;
     markNeedsPaint();
+    markNeedsSemanticsUpdate();
   }
 
   /// When set to true, hit tests are performed based on the position of the
@@ -1960,42 +1963,49 @@ class RenderTransform extends RenderProxyBox {
       return;
     _transform = new Matrix4.copy(value);
     markNeedsPaint();
+    markNeedsSemanticsUpdate();
   }
 
   /// Sets the transform to the identity matrix.
   void setIdentity() {
     _transform.setIdentity();
     markNeedsPaint();
+    markNeedsSemanticsUpdate();
   }
 
   /// Concatenates a rotation about the x axis into the transform.
   void rotateX(double radians) {
     _transform.rotateX(radians);
     markNeedsPaint();
+    markNeedsSemanticsUpdate();
   }
 
   /// Concatenates a rotation about the y axis into the transform.
   void rotateY(double radians) {
     _transform.rotateY(radians);
     markNeedsPaint();
+    markNeedsSemanticsUpdate();
   }
 
   /// Concatenates a rotation about the z axis into the transform.
   void rotateZ(double radians) {
     _transform.rotateZ(radians);
     markNeedsPaint();
+    markNeedsSemanticsUpdate();
   }
 
   /// Concatenates a translation by (x, y, z) into the transform.
   void translate(double x, [double y = 0.0, double z = 0.0]) {
     _transform.translate(x, y, z);
     markNeedsPaint();
+    markNeedsSemanticsUpdate();
   }
 
   /// Concatenates a scale into the transform.
   void scale(double x, [double y, double z]) {
     _transform.scale(x, y, z);
     markNeedsPaint();
+    markNeedsSemanticsUpdate();
   }
 
   Matrix4 get _effectiveTransform {
diff --git a/packages/flutter/lib/src/semantics/semantics.dart b/packages/flutter/lib/src/semantics/semantics.dart
@@ -1431,18 +1431,74 @@ class SemanticsNode extends AbstractNode with DiagnosticableTreeMixin {
   }
 }
 
-/// A helper class to contain a semantics node and the effective
-/// sort order of that node during the traversal of the tree. This is
-/// what is actually sorted when semantics nodes are sorted.
+/// This class defines the comparison that is used to sort [SemanticsNode]s
+/// before sending them to the platform side.
+///
+/// This is a helper class used to contain a [node], the effective
+/// [order], the globally transformed starting corner [globalStartCorner],
+/// and the containing node's [containerTextDirection] during the traversal of
+/// the semantics node tree. A null value is allowed for [containerTextDirection],
+/// because in that case we want to fall back to ordering by child insertion
+/// order for nodes that are equal after sorting from top to bottom.
 class _TraversalSortNode implements Comparable<_TraversalSortNode> {
-  _TraversalSortNode(this.node, this.order);
+  _TraversalSortNode(this.node, this.order, this.containerTextDirection, Matrix4 transform)
+    : assert(node != null) {
+    // When containerTextDirection is null, this is set to topLeft, but the x
+    // coordinate is also ignored when doing the comparison in that case, so
+    // this isn't actually expressing a directionality opinion.
+    globalStartCorner = _transformPoint(
+      containerTextDirection == TextDirection.rtl ? node.rect.topRight : node.rect.topLeft,
+      transform,
+    );
+  }
+
+  /// The node that this sort node represents.
   SemanticsNode node;
+
+  /// The effective text direction for this node is the directionality that
+  /// its container has.
+  TextDirection containerTextDirection;
+
+  /// This is the effective sort order for this node, taking into account its
+  /// parents.
   SemanticsSortOrder order;
 
+  /// The is the starting corner for the rectangle on this semantics node in
+  /// global coordinates. When the container has the directionality
+  /// [TextDirection.ltr], this is the upper left corner.  When the container
+  /// has the directionality [TextDirection.rtl], this is the upper right
+  /// corner. When the container has no directionality, this is set, but the
+  /// x coordinate is ignored.
+  Offset globalStartCorner;
+
+  static Offset _transformPoint(Offset point, Matrix4 matrix) {
+    final Vector3 result = matrix.transform3(new Vector3(point.dx, point.dy, 0.0));
+    return new Offset(result.x, result.y);
+  }
+
+  /// Compares the node's start corner with that of `other`.
+  ///
+  /// Sorts top to bottom, and then start to end.
+  ///
+  /// This takes into account the container text direction, since the
+  /// coordinate system has zero on the left, and we need to compare
+  /// differently for different text directions.
+  ///
+  /// If no text direction is available (i.e. [containerTextDirection] is
+  /// null), then we sort by vertical position first, and then by child
+  /// insertion order.
   int _compareGeometry(_TraversalSortNode other) {
-    // TODO(gspencer): Move the geometric comparison from the platform side to here.
-    // This involves calculating the globally-transformed quad for the semantics node rect
-    // and then sorting by its bounding box, based on the container's directionality.
+    final int verticalDiff = globalStartCorner.dy.compareTo(other.globalStartCorner.dy);
+    if (verticalDiff != 0) {
+      return verticalDiff;
+    }
+    switch (containerTextDirection) {
+      case TextDirection.rtl:
+        return other.globalStartCorner.dx.compareTo(globalStartCorner.dx);
+      case TextDirection.ltr:
+        return globalStartCorner.dx.compareTo(other.globalStartCorner.dx);
+    }
+    // In case containerTextDirection is null we fall back to child insertion order.
     return 0;
   }
 
@@ -1490,17 +1546,34 @@ class SemanticsOwner extends ChangeNotifier {
   void _updateTraversalOrder() {
     final List<_TraversalSortNode> nodesInSemanticsTraversalOrder = <_TraversalSortNode>[];
     SemanticsSortOrder currentSortOrder = new SemanticsSortOrder(keys: <SemanticsSortKey>[]);
+    Matrix4 currentTransform = new Matrix4.identity();
+    TextDirection currentTextDirection = rootSemanticsNode.textDirection;
     bool visitor(SemanticsNode node) {
       final SemanticsSortOrder previousOrder = currentSortOrder;
+      final Matrix4 previousTransform = currentTransform.clone();
       if (node.sortOrder != null) {
         currentSortOrder = currentSortOrder.merge(node.sortOrder);
       }
-      final _TraversalSortNode traversalNode = new _TraversalSortNode(node, currentSortOrder);
+      if (node.transform != null) {
+        currentTransform.multiply(node.transform);
+      }
+      final _TraversalSortNode traversalNode = new _TraversalSortNode(
+        node,
+        currentSortOrder,
+        currentTextDirection,
+        currentTransform,
+      );
+      // The text direction in force here is the parent's text direction.
       nodesInSemanticsTraversalOrder.add(traversalNode);
       if (node.hasChildren) {
+        final TextDirection previousTextDirection = currentTextDirection;
+        currentTextDirection = node.textDirection;
+        // Now visit the children with this node's text direction in force.
         node.visitChildren(visitor);
+        currentTextDirection = previousTextDirection;
       }
       currentSortOrder = previousOrder;
+      currentTransform = previousTransform;
       return true;
     }
     rootSemanticsNode.visitChildren(visitor);
@@ -2522,7 +2595,9 @@ String _concatStrings({
 /// of keys can co-exist at the same level and not interfere with each other,
 /// allowing for sorting into groups.  Keys that evaluate as equal, or when
 /// compared with Widgets that don't have [Semantics], fall back to the default
-/// upper-start-to-lower-end geometric ordering.
+/// upper-start-to-lower-end geometric ordering if a text directionality
+/// exists, and they sort from top to bottom followed by child insertion order
+/// when no directionality is present.
 ///
 /// Since widgets are globally sorted by their sort key, the order does not have
 /// to conform to the widget hierarchy.
diff --git a/packages/flutter/test/widgets/semantics_test.dart b/packages/flutter/test/widgets/semantics_test.dart
