Skip to content

Commit b2f3404

Browse files
Remove Path.combine call from CupertionoTextSelectionToolbar (flutter#134369)
Hopefully this fixes flutter#110076 by removing the `Path.combine` call. Not sure how I can verify in a test that `Path.combine` is not called.
1 parent 2ea9edc commit b2f3404

File tree

2 files changed

+101
-58
lines changed

2 files changed

+101
-58
lines changed

packages/flutter/lib/src/cupertino/text_selection_toolbar.dart

Lines changed: 95 additions & 53 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
// found in the LICENSE file.
44

55
import 'dart:collection';
6+
import 'dart:math' as math show pi;
67
import 'dart:ui' as ui;
78

89
import 'package:flutter/foundation.dart' show Brightness, clampDouble;
@@ -279,87 +280,127 @@ class _RenderCupertinoTextSelectionToolbarShape extends RenderShiftedBox {
279280
markNeedsLayout();
280281
}
281282

282-
// The child is tall enough to have the arrow clipped out of it on both sides
283-
// top and bottom. Since _kToolbarHeight includes the height of one arrow, the
284-
// total height that the child is given is that plus one more arrow height.
285-
// The extra height on the opposite side of the arrow will be clipped out. By
286-
// using this approach, the buttons don't need any special padding that
287-
// depends on isAbove.
288-
final BoxConstraints _heightConstraint = BoxConstraints.tightFor(
289-
height: _kToolbarHeight + _kToolbarArrowSize.height,
290-
);
291-
292283
@override
293284
void performLayout() {
285+
final RenderBox? child = this.child;
294286
if (child == null) {
295287
return;
296288
}
297289

298-
final BoxConstraints enforcedConstraint = constraints.loosen();
290+
// The child is tall enough to have the arrow clipped out of it on both sides
291+
// top and bottom. Since _kToolbarHeight includes the height of one arrow, the
292+
// total height that the child is given is that plus one more arrow height.
293+
// The extra height on the opposite side of the arrow will be clipped out. By
294+
// using this approach, the buttons don't need any special padding that
295+
// depends on isAbove.
296+
final BoxConstraints heightConstraint = BoxConstraints(
297+
minHeight: _kToolbarHeight + _kToolbarArrowSize.height,
298+
maxHeight: _kToolbarHeight + _kToolbarArrowSize.height,
299+
minWidth: _kToolbarArrowSize.width + _kToolbarBorderRadius.x * 2,
300+
).enforce(constraints.loosen());
299301

300-
child!.layout(_heightConstraint.enforce(enforcedConstraint), parentUsesSize: true);
302+
child.layout(heightConstraint, parentUsesSize: true);
301303

302304
// The height of one arrow will be clipped off of the child, so adjust the
303305
// size and position to remove that piece from the layout.
304-
final BoxParentData childParentData = child!.parentData! as BoxParentData;
306+
final BoxParentData childParentData = child.parentData! as BoxParentData;
305307
childParentData.offset = Offset(
306308
0.0,
307309
_isAbove ? -_kToolbarArrowSize.height : 0.0,
308310
);
309311
size = Size(
310-
child!.size.width,
311-
child!.size.height - _kToolbarArrowSize.height,
312+
child.size.width,
313+
child.size.height - _kToolbarArrowSize.height,
312314
);
313315
}
314316

317+
// Adds the given `rrect` to the current `path`, starting from the last point
318+
// in `path` and ends after the last corner of the rrect (closest corner to
319+
// `startAngle` in the counterclockwise direction), without closing the path.
320+
//
321+
// The `startAngle` argument must be a multiple of pi / 2, with 0 being the
322+
// positive half of the x-axis, and pi / 2 being the negative half of the
323+
// y-axis.
324+
//
325+
// For instance, if `startAngle` equals pi/2 then this method draws a line
326+
// segment to the bottom-left corner of `rrect` from the last point in `path`,
327+
// and follows the `rrect` path clockwise until the bottom-right corner is
328+
// added, then this method returns the mutated path without closing it.
329+
static Path _addRRectToPath(Path path, RRect rrect, { required double startAngle }) {
330+
const double halfPI = math.pi / 2;
331+
assert(startAngle % halfPI == 0);
332+
final Rect rect = rrect.outerRect;
333+
334+
final List<(Offset, Radius)> rrectCorners = <(Offset, Radius)>[
335+
(rect.bottomRight, -rrect.brRadius),
336+
(rect.bottomLeft, Radius.elliptical(rrect.blRadiusX, -rrect.blRadiusY)),
337+
(rect.topLeft, rrect.tlRadius),
338+
(rect.topRight, Radius.elliptical(-rrect.trRadiusX, rrect.trRadiusY)),
339+
];
340+
341+
// Add the 4 corners to the path clockwise. Convert radians to quadrants
342+
// to avoid fp arithmetics. The order is br -> bl -> tl -> tr if the starting
343+
// angle is 0.
344+
final int startQuadrantIndex = startAngle ~/ halfPI;
345+
for (int i = startQuadrantIndex; i < rrectCorners.length + startQuadrantIndex; i += 1) {
346+
final (Offset vertex, Radius rectCenterOffset) = rrectCorners[i % rrectCorners.length];
347+
final Offset otherVertex = Offset(vertex.dx + 2 * rectCenterOffset.x, vertex.dy + 2 * rectCenterOffset.y);
348+
final Rect rect = Rect.fromPoints(vertex, otherVertex);
349+
path.arcTo(rect, halfPI * i, halfPI, false);
350+
}
351+
return path;
352+
}
353+
315354
// The path is described in the toolbar's coordinate system.
316-
Path _clipPath() {
317-
final BoxParentData childParentData = child!.parentData! as BoxParentData;
318-
final Path rrect = Path()
319-
..addRRect(
320-
RRect.fromRectAndRadius(
321-
Offset(0.0, _kToolbarArrowSize.height)
322-
& Size(
323-
child!.size.width,
324-
child!.size.height - _kToolbarArrowSize.height * 2,
325-
),
326-
_kToolbarBorderRadius,
327-
),
328-
);
355+
Path _clipPath(RenderBox child) {
356+
final Rect rect = Offset(0.0, _isAbove ? 0 : _kToolbarArrowSize.height)
357+
& Size(size.width, size.height - _kToolbarArrowSize.height);
358+
final RRect rrect = RRect.fromRectAndRadius(rect, _kToolbarBorderRadius).scaleRadii();
359+
360+
final Path path = Path();
361+
// If there isn't enough width for the arrow + radii, ignore the arrow.
362+
// Because of the constraints we gave children in performLayout, this should
363+
// only happen if the parent isn't wide enough which should be very rare, and
364+
// when that happens the arrow won't be too useful anyways.
365+
if (_kToolbarBorderRadius.x * 2 + _kToolbarArrowSize.width > size.width) {
366+
return path..addRRect(rrect);
367+
}
329368

330369
final Offset localAnchor = globalToLocal(_anchor);
331-
final double centerX = childParentData.offset.dx + child!.size.width / 2;
332-
final double arrowXOffsetFromCenter = localAnchor.dx - centerX;
333-
final double arrowTipX = child!.size.width / 2 + arrowXOffsetFromCenter;
334-
335-
final double arrowBaseY = _isAbove
336-
? child!.size.height - _kToolbarArrowSize.height
337-
: _kToolbarArrowSize.height;
338-
339-
final double arrowTipY = _isAbove ? child!.size.height : 0;
340-
341-
final Path arrow = Path()
342-
..moveTo(arrowTipX, arrowTipY)
343-
..lineTo(arrowTipX - _kToolbarArrowSize.width / 2, arrowBaseY)
344-
..lineTo(arrowTipX + _kToolbarArrowSize.width / 2, arrowBaseY)
345-
..close();
370+
final double arrowTipX = clampDouble(
371+
localAnchor.dx,
372+
_kToolbarBorderRadius.x + _kToolbarArrowSize.width / 2,
373+
size.width - _kToolbarArrowSize.width / 2 - _kToolbarBorderRadius.x,
374+
);
346375

347-
return Path.combine(PathOperation.union, rrect, arrow);
376+
// Draw the path clockwise, starting from the beginning side of the arrow.
377+
if (_isAbove) {
378+
path
379+
..moveTo(arrowTipX + _kToolbarArrowSize.width / 2, rect.bottom) // right side of the arrow triangle
380+
..lineTo(arrowTipX, rect.bottom + _kToolbarArrowSize.height) // The tip of the arrow
381+
..lineTo(arrowTipX - _kToolbarArrowSize.width / 2, rect.bottom); // left side of the arrow triangle
382+
} else {
383+
path
384+
..moveTo(arrowTipX - _kToolbarArrowSize.width / 2, rect.top) // right side of the arrow triangle
385+
..lineTo(arrowTipX, rect.top) // The tip of the arrow
386+
..lineTo(arrowTipX + _kToolbarArrowSize.width / 2, rect.top); // left side of the arrow triangle
387+
}
388+
final double startAngle = _isAbove ? math.pi / 2 : -math.pi / 2;
389+
return _addRRectToPath(path, rrect, startAngle: startAngle)..close();
348390
}
349391

350392
@override
351393
void paint(PaintingContext context, Offset offset) {
394+
final RenderBox? child = this.child;
352395
if (child == null) {
353396
return;
354397
}
355-
356-
final BoxParentData childParentData = child!.parentData! as BoxParentData;
357398
_clipPathLayer.layer = context.pushClipPath(
358399
needsCompositing,
359-
offset + childParentData.offset,
360-
Offset.zero & child!.size,
361-
_clipPath(),
362-
(PaintingContext innerContext, Offset innerOffset) => innerContext.paintChild(child!, innerOffset),
400+
offset,
401+
Offset.zero & size,
402+
_clipPath(child),
403+
super.paint,
363404
oldLayer: _clipPathLayer.layer,
364405
);
365406
}
@@ -376,11 +417,12 @@ class _RenderCupertinoTextSelectionToolbarShape extends RenderShiftedBox {
376417
@override
377418
void debugPaintSize(PaintingContext context, Offset offset) {
378419
assert(() {
420+
final RenderBox? child = this.child;
379421
if (child == null) {
380422
return true;
381423
}
382424

383-
_debugPaint ??= Paint()
425+
final ui.Paint debugPaint = _debugPaint ??= Paint()
384426
..shader = ui.Gradient.linear(
385427
Offset.zero,
386428
const Offset(10.0, 10.0),
@@ -391,8 +433,8 @@ class _RenderCupertinoTextSelectionToolbarShape extends RenderShiftedBox {
391433
..strokeWidth = 2.0
392434
..style = PaintingStyle.stroke;
393435

394-
final BoxParentData childParentData = child!.parentData! as BoxParentData;
395-
context.canvas.drawPath(_clipPath().shift(offset + childParentData.offset), _debugPaint!);
436+
final BoxParentData childParentData = child.parentData! as BoxParentData;
437+
context.canvas.drawPath(_clipPath(child).shift(offset + childParentData.offset), debugPaint);
396438
return true;
397439
}());
398440
}

packages/flutter/test/cupertino/text_field_test.dart

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -6835,8 +6835,9 @@ void main() {
68356835
bottomLeftSelectionPosition.translate(0, 8 + 0.1),
68366836
],
68376837
includes: <Offset> [
6838-
// Expected center of the arrow.
6839-
Offset(26.0, bottomLeftSelectionPosition.dy + 8 + 0.1),
6838+
// Expected center of the arrow. The arrow should stay clear of
6839+
// the edges of the selection toolbar.
6840+
Offset(26.0, bottomLeftSelectionPosition.dy + 7.0 + 8.0 + 0.1),
68406841
],
68416842
),
68426843
),
@@ -6846,7 +6847,7 @@ void main() {
68466847
find.byType(CupertinoTextSelectionToolbar),
68476848
paints..clipPath(
68486849
pathMatcher: PathBoundsMatcher(
6849-
topMatcher: moreOrLessEquals(bottomLeftSelectionPosition.dy + 8, epsilon: 0.01),
6850+
topMatcher: moreOrLessEquals(bottomLeftSelectionPosition.dy + 7 + 8, epsilon: 0.01),
68506851
leftMatcher: moreOrLessEquals(8),
68516852
rightMatcher: lessThanOrEqualTo(400 - 8),
68526853
bottomMatcher: moreOrLessEquals(bottomLeftSelectionPosition.dy + 8 + 45, epsilon: 0.01),
@@ -6897,7 +6898,7 @@ void main() {
68976898
],
68986899
includes: <Offset> [
68996900
// Expected center of the arrow.
6900-
Offset(400 - 26.0, bottomLeftSelectionPosition.dy + 8 + 0.1),
6901+
Offset(400 - 26.0, bottomLeftSelectionPosition.dy + 7 + 8 + 0.1),
69016902
],
69026903
),
69036904
),
@@ -6907,7 +6908,7 @@ void main() {
69076908
find.byType(CupertinoTextSelectionToolbar),
69086909
paints..clipPath(
69096910
pathMatcher: PathBoundsMatcher(
6910-
topMatcher: moreOrLessEquals(bottomLeftSelectionPosition.dy + 8, epsilon: 0.01),
6911+
topMatcher: moreOrLessEquals(bottomLeftSelectionPosition.dy + 7 + 8, epsilon: 0.01),
69116912
rightMatcher: moreOrLessEquals(400.0 - 8),
69126913
bottomMatcher: moreOrLessEquals(bottomLeftSelectionPosition.dy + 8 + 45, epsilon: 0.01),
69136914
leftMatcher: greaterThanOrEqualTo(8),

0 commit comments

Comments
 (0)