positionDependentBox (#12232)

Fix the documentation (it was just wrong before).

Add an explicit code path to handle horizontal overflow so that we
don't have a left bias.

packages/flutter/lib/src/painting/geometry.dart

@@ -8,14 +8,32 @@ import 'package:flutter/foundation.dart';
 
 import 'basic_types.dart';
 
-const double _kScreenEdgeMargin = 10.0;
-
-/// Position a box either above or below a target box specified in the global
-/// coordinate system.
+/// Position a child box within a container box, either above or below a target
+/// point.
 ///
-/// The target box is specified by `size` and `target` and the box being
-/// positioned is specified by `childSize`. `verticalOffset` is the amount of
-/// vertical distance between the boxes.
+/// The container's size is described by `size`.
+///
+/// The target point is specified by `target`, as an offset from the top left of
+/// the container.
+///
+/// The child box's size is given by `childSize`.
+///
+/// The return value is the suggested distance from the top left of the
+/// container box to the top left of the child box.
+///
+/// The suggested position will be above the target point if `preferBelow` is
+/// false, and below the target point if it is true, unless it wouldn't fit on
+/// the preferred side but would fit on the other side.
+///
+/// The suggested position will place the nearest side of the child to the
+/// target point `verticalOffset` from the target point (even if it cannot fit
+/// given that constraint).
+///
+/// The suggested position will be at least `margin` away from the edge of the
+/// container. If possible, the child will be positioned so that its center is
+/// aligned with the target point. If the child cannot fit horizontally within
+/// the container given the margin, then the child will be centered in the
+/// container.
 ///
 /// Used by [Tooltip] to position a tooltip relative to its parent.
 ///
@@ -24,32 +42,39 @@ Offset positionDependentBox({
   @required Size size,
   @required Size childSize,
   @required Offset target,
-  @required double verticalOffset,
   @required bool preferBelow,
+  double verticalOffset: 0.0,
+  double margin: 10.0,
 }) {
   assert(size != null);
   assert(childSize != null);
   assert(target != null);
   assert(verticalOffset != null);
   assert(preferBelow != null);
+  assert(margin != null);
   // VERTICAL DIRECTION
-  final bool fitsBelow = target.dy + verticalOffset + childSize.height <= size.height - _kScreenEdgeMargin;
-  final bool fitsAbove = target.dy - verticalOffset - childSize.height >= _kScreenEdgeMargin;
+  final bool fitsBelow = target.dy + verticalOffset + childSize.height <= size.height - margin;
+  final bool fitsAbove = target.dy - verticalOffset - childSize.height >= margin;
   final bool tooltipBelow = preferBelow ? fitsBelow || !fitsAbove : !(fitsAbove || !fitsBelow);
   double y;
   if (tooltipBelow)
-    y = math.min(target.dy + verticalOffset, size.height - _kScreenEdgeMargin);
+    y = math.min(target.dy + verticalOffset, size.height - margin);
   else
-    y = math.max(target.dy - verticalOffset - childSize.height, _kScreenEdgeMargin);
+    y = math.max(target.dy - verticalOffset - childSize.height, margin);
   // HORIZONTAL DIRECTION
-  final double normalizedTargetX = target.dx.clamp(_kScreenEdgeMargin, size.width - _kScreenEdgeMargin);
   double x;
-  if (normalizedTargetX < _kScreenEdgeMargin + childSize.width / 2.0) {
-    x = _kScreenEdgeMargin;
-  } else if (normalizedTargetX > size.width - _kScreenEdgeMargin - childSize.width / 2.0) {
-    x = size.width - _kScreenEdgeMargin - childSize.width;
+  if (size.width - margin * 2.0 < childSize.width) {
+    x = (size.width - childSize.width) / 2.0;
   } else {
-    x = normalizedTargetX - childSize.width / 2.0;
+    final double normalizedTargetX = target.dx.clamp(margin, size.width - margin);
+    final double edge = margin + childSize.width / 2.0;
+    if (normalizedTargetX < edge) {
+      x = margin;
+    } else if (normalizedTargetX > size.width - edge) {
+      x = size.width - margin - childSize.width;
+    } else {
+      x = normalizedTargetX - childSize.width / 2.0;
+    }
   }
   return new Offset(x, y);
 }

packages/flutter/test/painting/geometry_test.dart

@@ -0,0 +1,123 @@
+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+import 'package:flutter/painting.dart';
+import 'package:test/test.dart';
+
+void main() {
+  test('positionDependentBox', () {
+    // For historical reasons, significantly more tests of this function
+    // exist in: ../material/tooltip_test.dart
+    expect(
+      positionDependentBox(
+        size: const Size(100.0, 100.0),
+        childSize: const Size(20.0, 10.0),
+        target: const Offset(50.0, 50.0),
+        preferBelow: false,
+        verticalOffset: 0.0,
+        margin: 0.0,
+      ),
+      const Offset(40.0, 40.0),
+    );
+    expect(
+      positionDependentBox(
+        size: const Size(100.0, 100.0),
+        childSize: const Size(200.0, 10.0),
+        target: const Offset(50.0, 50.0),
+        preferBelow: false,
+        verticalOffset: 0.0,
+        margin: 0.0,
+      ),
+      const Offset(-50.0, 40.0),
+    );
+    expect(
+      positionDependentBox(
+        size: const Size(100.0, 100.0),
+        childSize: const Size(200.0, 10.0),
+        target: const Offset(0.0, 50.0),
+        preferBelow: false,
+        verticalOffset: 0.0,
+        margin: 0.0,
+      ),
+      const Offset(-50.0, 40.0),
+    );
+    expect(
+      positionDependentBox(
+        size: const Size(100.0, 100.0),
+        childSize: const Size(200.0, 10.0),
+        target: const Offset(100.0, 50.0),
+        preferBelow: false,
+        verticalOffset: 0.0,
+        margin: 0.0,
+      ),
+      const Offset(-50.0, 40.0),
+    );
+    expect(
+      positionDependentBox(
+        size: const Size(100.0, 100.0),
+        childSize: const Size(50.0, 10.0),
+        target: const Offset(50.0, 50.0),
+        preferBelow: false,
+        verticalOffset: 0.0,
+        margin: 20.0, // 60.0 left
+      ),
+      const Offset(25.0, 40.0),
+    );
+    expect(
+      positionDependentBox(
+        size: const Size(100.0, 100.0),
+        childSize: const Size(50.0, 10.0),
+        target: const Offset(50.0, 50.0),
+        preferBelow: false,
+        verticalOffset: 0.0,
+        margin: 30.0, // 40.0 left
+      ),
+      const Offset(25.0, 40.0),
+    );
+    expect(
+      positionDependentBox(
+        size: const Size(100.0, 100.0),
+        childSize: const Size(50.0, 10.0),
+        target: const Offset(0.0, 50.0),
+        preferBelow: false,
+        verticalOffset: 0.0,
+        margin: 20.0, // 60.0 left
+      ),
+      const Offset(20.0, 40.0),
+    );
+    expect(
+      positionDependentBox(
+        size: const Size(100.0, 100.0),
+        childSize: const Size(50.0, 10.0),
+        target: const Offset(0.0, 50.0),
+        preferBelow: false,
+        verticalOffset: 0.0,
+        margin: 30.0, // 40.0 left
+      ),
+      const Offset(25.0, 40.0),
+    );
+    expect(
+      positionDependentBox(
+        size: const Size(100.0, 100.0),
+        childSize: const Size(50.0, 10.0),
+        target: const Offset(100.0, 50.0),
+        preferBelow: false,
+        verticalOffset: 0.0,
+        margin: 20.0, // 60.0 left
+      ),
+      const Offset(30.0, 40.0),
+    );
+    expect(
+      positionDependentBox(
+        size: const Size(100.0, 100.0),
+        childSize: const Size(50.0, 10.0),
+        target: const Offset(100.0, 50.0),
+        preferBelow: false,
+        verticalOffset: 0.0,
+        margin: 30.0, // 40.0 left
+      ),
+      const Offset(25.0, 40.0),
+    );
+  });
+}