New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix single ToggleButton border painting bugs #73780
Changes from 3 commits
01c61a3
37de0f3
fb59b76
e1400cf
9557c92
6dda1a6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -424,7 +424,9 @@ class ToggleButtons extends StatelessWidget { | |
?? toggleButtonsTheme.borderRadius | ||
?? BorderRadius.zero; | ||
|
||
if (direction == Axis.horizontal) { | ||
if (length == 1) { | ||
return resultingBorderRadius; | ||
} else if (direction == Axis.horizontal) { | ||
if (_isFirstButton(index, length, textDirection)) { | ||
return BorderRadius.only( | ||
topLeft: resultingBorderRadius.topLeft, | ||
|
@@ -466,7 +468,14 @@ class ToggleButtons extends StatelessWidget { | |
?? toggleButtonsTheme.borderWidth | ||
?? _defaultBorderWidth; | ||
|
||
if (direction == Axis.horizontal) { | ||
if (length == 1) { | ||
return BorderRadius.only( | ||
topLeft: resultingBorderRadius.topLeft - Radius.circular(resultingBorderWidth / 2.0), | ||
bottomLeft: resultingBorderRadius.bottomLeft - Radius.circular(resultingBorderWidth / 2.0), | ||
topRight: resultingBorderRadius.topRight - Radius.circular(resultingBorderWidth / 2.0), | ||
bottomRight: resultingBorderRadius.bottomRight - Radius.circular(resultingBorderWidth / 2.0), | ||
); | ||
} else if (direction == Axis.horizontal) { | ||
if (_isFirstButton(index, length, textDirection)) { | ||
return BorderRadius.only( | ||
topLeft: resultingBorderRadius.topLeft - Radius.circular(resultingBorderWidth / 2.0), | ||
|
@@ -1269,11 +1278,12 @@ class _SelectToggleButtonRenderObject extends RenderShiftedBox { | |
const double sweepAngle = math.pi / 2.0; | ||
final RRect rrect = RRect.fromRectAndCorners( | ||
center, | ||
topLeft: borderRadius.topLeft, | ||
topRight: borderRadius.topRight, | ||
bottomLeft: borderRadius.bottomLeft, | ||
bottomRight: borderRadius.bottomRight, | ||
topLeft: (borderRadius.topLeft.x * borderRadius.topLeft.y != 0.0) ? borderRadius.topLeft : Radius.zero, | ||
topRight: (borderRadius.topRight.x * borderRadius.topRight.y != 0.0) ? borderRadius.topRight : Radius.zero, | ||
bottomLeft: (borderRadius.bottomLeft.x * borderRadius.bottomLeft.y != 0.0) ? borderRadius.bottomLeft : Radius.zero, | ||
bottomRight: (borderRadius.bottomRight.x * borderRadius.bottomRight.y != 0.0) ? borderRadius.bottomRight : Radius.zero, | ||
).scaleRadii(); | ||
|
||
final Rect tlCorner = Rect.fromLTWH( | ||
rrect.left, | ||
rrect.top, | ||
|
@@ -1299,8 +1309,25 @@ class _SelectToggleButtonRenderObject extends RenderShiftedBox { | |
rrect.brRadiusY * 2, | ||
); | ||
|
||
final Paint leadingPaint = leadingBorderSide.toPaint(); | ||
// Only one button. | ||
if (isFirstButton && isLastButton) { | ||
final Path leadingPath = Path(); | ||
final double startX = (rrect.brRadiusX == 0.0) ? outer.right : rrect.right - rrect.brRadiusX; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice, good catch here |
||
leadingPath..moveTo(startX, rrect.bottom) | ||
..lineTo(rrect.left + rrect.blRadiusX, rrect.bottom) | ||
..addArc(blCorner, math.pi / 2.0, sweepAngle) | ||
..lineTo(rrect.left, rrect.top + rrect.tlRadiusY) | ||
..addArc(tlCorner, math.pi, sweepAngle) | ||
..lineTo(rrect.right - rrect.trRadiusX, rrect.top) | ||
..addArc(trCorner, math.pi * 3.0 / 2.0, sweepAngle) | ||
..lineTo(rrect.right, rrect.bottom - rrect.brRadiusY) | ||
..addArc(brCorner, 0, sweepAngle); | ||
context.canvas.drawPath(leadingPath, leadingPaint); | ||
return; | ||
} | ||
|
||
if (direction == Axis.horizontal) { | ||
final Paint leadingPaint = leadingBorderSide.toPaint(); | ||
switch (textDirection) { | ||
case TextDirection.ltr: | ||
if (isLastButton) { | ||
|
@@ -1339,7 +1366,6 @@ class _SelectToggleButtonRenderObject extends RenderShiftedBox { | |
..lineTo(outer.right - rrect.trRadiusX, rrect.top) | ||
..moveTo(rrect.left + borderSide.width / 2.0 + rrect.tlRadiusX, rrect.bottom) | ||
..lineTo(outer.right - rrect.trRadiusX, rrect.bottom); | ||
|
||
context.canvas.drawPath(horizontalPaths, horizontalPaint); | ||
} | ||
break; | ||
|
@@ -1385,14 +1411,12 @@ class _SelectToggleButtonRenderObject extends RenderShiftedBox { | |
break; | ||
} | ||
} else { | ||
final Paint leadingPaint = leadingBorderSide.toPaint(); | ||
switch (verticalDirection) { | ||
case VerticalDirection.down: | ||
if (isLastButton) { | ||
final Path topPath = Path(); | ||
topPath..moveTo(outer.left, outer.top + leadingBorderSide.width / 2) | ||
..lineTo(outer.right, outer.top + leadingBorderSide.width / 2); | ||
|
||
context.canvas.drawPath(topPath, leadingPaint); | ||
|
||
final Paint endingPaint = trailingBorderSide.toPaint(); | ||
|
@@ -1403,7 +1427,6 @@ class _SelectToggleButtonRenderObject extends RenderShiftedBox { | |
..lineTo(rrect.right - rrect.blRadiusX, rrect.bottom) | ||
..addArc(brCorner, math.pi / 2.0, -sweepAngle) | ||
..lineTo(rrect.right, rrect.top + leadingBorderSide.width / 2.0); | ||
|
||
context.canvas.drawPath(endingPath, endingPaint); | ||
} else if (isFirstButton) { | ||
final Path leadingPath = Path(); | ||
|
@@ -1413,7 +1436,6 @@ class _SelectToggleButtonRenderObject extends RenderShiftedBox { | |
..lineTo(rrect.right - rrect.trRadiusX, rrect.top) | ||
..addArc(trCorner, math.pi * 3.0 / 2.0, sweepAngle) | ||
..lineTo(rrect.right, outer.bottom); | ||
|
||
context.canvas.drawPath(leadingPath, leadingPaint); | ||
} else { | ||
final Path topPath = Path(); | ||
|
@@ -1427,7 +1449,6 @@ class _SelectToggleButtonRenderObject extends RenderShiftedBox { | |
..lineTo(rrect.left, outer.bottom) | ||
..moveTo(rrect.right, outer.top + leadingBorderSide.width) | ||
..lineTo(rrect.right, outer.bottom); | ||
|
||
context.canvas.drawPath(paths, paint); | ||
} | ||
break; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -891,13 +891,6 @@ void main() { | |
expect( | ||
toggleButtonRenderObject, | ||
paints | ||
// trailing side | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If only one button, we can paint the border with one path now. |
||
..path( | ||
style: PaintingStyle.stroke, | ||
color: theme.colorScheme.onSurface.withOpacity(0.12), | ||
strokeWidth: defaultBorderWidth, | ||
) | ||
// leading side, top and bottom | ||
..path( | ||
style: PaintingStyle.stroke, | ||
color: theme.colorScheme.onSurface.withOpacity(0.12), | ||
|
@@ -925,13 +918,6 @@ void main() { | |
expect( | ||
toggleButtonRenderObject, | ||
paints | ||
// trailing side | ||
..path( | ||
style: PaintingStyle.stroke, | ||
color: theme.colorScheme.onSurface.withOpacity(0.12), | ||
strokeWidth: defaultBorderWidth, | ||
) | ||
// leading side, top and bottom | ||
..path( | ||
style: PaintingStyle.stroke, | ||
color: theme.colorScheme.onSurface.withOpacity(0.12), | ||
|
@@ -958,13 +944,6 @@ void main() { | |
expect( | ||
toggleButtonRenderObject, | ||
paints | ||
// trailing side | ||
..path( | ||
style: PaintingStyle.stroke, | ||
color: theme.colorScheme.onSurface.withOpacity(0.12), | ||
strokeWidth: defaultBorderWidth, | ||
) | ||
// leading side, top and bottom | ||
..path( | ||
style: PaintingStyle.stroke, | ||
color: theme.colorScheme.onSurface.withOpacity(0.12), | ||
|
@@ -1005,13 +984,6 @@ void main() { | |
expect( | ||
toggleButtonRenderObject, | ||
paints | ||
// trailing side | ||
..path( | ||
style: PaintingStyle.stroke, | ||
color: borderColor, | ||
strokeWidth: customWidth, | ||
) | ||
// leading side, top and bottom | ||
..path( | ||
style: PaintingStyle.stroke, | ||
color: borderColor, | ||
|
@@ -1041,13 +1013,6 @@ void main() { | |
expect( | ||
toggleButtonRenderObject, | ||
paints | ||
// trailing side | ||
..path( | ||
style: PaintingStyle.stroke, | ||
color: selectedBorderColor, | ||
strokeWidth: customWidth, | ||
) | ||
// leading side, top and bottom | ||
..path( | ||
style: PaintingStyle.stroke, | ||
color: selectedBorderColor, | ||
|
@@ -1076,13 +1041,6 @@ void main() { | |
expect( | ||
toggleButtonRenderObject, | ||
paints | ||
// trailing side | ||
..path( | ||
style: PaintingStyle.stroke, | ||
color: disabledBorderColor, | ||
strokeWidth: customWidth, | ||
) | ||
// leading side, top and bottom | ||
..path( | ||
style: PaintingStyle.stroke, | ||
color: disabledBorderColor, | ||
|
@@ -1510,6 +1468,80 @@ void main() { | |
}, | ||
); | ||
|
||
// Regression test for https://github.com/flutter/flutter/issues/73725 | ||
testWidgets('Border radius paint test when there is only one button', (WidgetTester tester) async { | ||
final ThemeData theme = ThemeData(); | ||
await tester.pumpWidget( | ||
Material( | ||
child: boilerplate( | ||
child: RepaintBoundary( | ||
child: ToggleButtons( | ||
borderRadius: const BorderRadius.all(Radius.circular(7.0)), | ||
isSelected: const <bool>[false], | ||
onPressed: (int index) {}, | ||
children: const <Widget>[ | ||
Text('First child'), | ||
], | ||
), | ||
), | ||
), | ||
), | ||
); | ||
|
||
// The only button should be laid out at the center of the screen. | ||
expect(tester.getCenter(find.text('First child')), const Offset(400.0, 300.0)); | ||
|
||
final List<RenderObject> toggleButtonRenderObject = tester.allRenderObjects.where((RenderObject object) { | ||
return object.runtimeType.toString() == '_SelectToggleButtonRenderObject'; | ||
}).toSet().toList(); | ||
|
||
// The first button paints the left, top and right sides with a path. | ||
expect( | ||
toggleButtonRenderObject[0], | ||
paints | ||
// left side, top and right - enabled. | ||
..path( | ||
style: PaintingStyle.stroke, | ||
color: theme.colorScheme.onSurface.withOpacity(0.12), | ||
strokeWidth: _defaultBorderWidth, | ||
), | ||
); | ||
|
||
await expectLater( | ||
find.byType(RepaintBoundary), | ||
matchesGoldenFile('toggle_buttons.oneButton.boardsPaint.png'), | ||
); | ||
},); | ||
xu-baolin marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
testWidgets('Border radius paint test when Radius.x or Radius.y equal 0.0', (WidgetTester tester) async { | ||
await tester.pumpWidget( | ||
Material( | ||
child: boilerplate( | ||
child: RepaintBoundary( | ||
child: ToggleButtons( | ||
borderRadius: const BorderRadius.only( | ||
topRight: Radius.elliptical(10, 0), | ||
topLeft: Radius.elliptical(0, 10), | ||
bottomRight: Radius.elliptical(0, 10), | ||
bottomLeft: Radius.elliptical(10, 0), | ||
), | ||
isSelected: const <bool>[false], | ||
onPressed: (int index) {}, | ||
children: const <Widget>[ | ||
Text('First child'), | ||
], | ||
), | ||
), | ||
), | ||
), | ||
); | ||
|
||
await expectLater( | ||
find.byType(RepaintBoundary), | ||
matchesGoldenFile('toggle_buttons.oneButton.boardsPaint2.png'), | ||
); | ||
},); | ||
xu-baolin marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
testWidgets('ToggleButtons implements debugFillProperties', (WidgetTester tester) async { | ||
final DiagnosticPropertiesBuilder builder = DiagnosticPropertiesBuilder(); | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change fixes the #2 bug.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My first instinct was that some kind of elliptical rounded corner to be drawn anyway instead of having none at all. Is this because of the
drawArc
API that we're using? For now, this looks good.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly correct.