Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix single ToggleButton border painting bugs #73780

Merged
merged 6 commits into from Jan 14, 2021
Merged

Fix single ToggleButton border painting bugs #73780

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
01c61a3
fix toggleButton border paint bug
xu-baolin Jan 12, 2021
37de0f3
fix test
xu-baolin Jan 12, 2021
fb59b76
fix test
xu-baolin Jan 12, 2021
e1400cf
fix test
xu-baolin Jan 12, 2021
9557c92
Update packages/flutter/test/material/toggle_buttons_test.dart
xu-baolin Jan 14, 2021
6dda1a6
Update packages/flutter/test/material/toggle_buttons_test.dart
xu-baolin Jan 14, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
47 changes: 34 additions & 13 deletions packages/flutter/lib/src/material/toggle_buttons.dart
View file
Open in desktop
Expand Up @@ -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,
Expand Down Expand Up @@ -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),
Expand Down Expand Up @@ -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,
Copy link
Member Author

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.

Copy link
Contributor

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly correct.

).scaleRadii();

final Rect tlCorner = Rect.fromLTWH(
rrect.left,
rrect.top,
Expand All @@ -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;
Copy link
Member Author

@xu-baolin xu-baolin Jan 12, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the radius is zero, we should paint a little more to connect the start and stop ends.
Otherwise, there will be a gap like below:
image

Copy link
Contributor

Choose a reason for hiding this comment

The 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) {
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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();
Expand All @@ -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();
Expand All @@ -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();
Expand All @@ -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;
Expand Down
116 changes: 74 additions & 42 deletions packages/flutter/test/material/toggle_buttons_test.dart
View file
Open in desktop
Expand Up @@ -891,13 +891,6 @@ void main() {
expect(
toggleButtonRenderObject,
paints
// trailing side
Copy link
Member Author

Choose a reason for hiding this comment

The 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),
Expand Down Expand Up @@ -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),
Expand All @@ -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),
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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>[true],
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'),
);
});

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>[true],
onPressed: (int index) {},
children: const <Widget>[
Text('First child'),
],
),
),
),
),
);

await expectLater(
find.byType(RepaintBoundary),
matchesGoldenFile('toggle_buttons.oneButton.boardsPaint2.png'),
);
});

testWidgets('ToggleButtons implements debugFillProperties', (WidgetTester tester) async {
final DiagnosticPropertiesBuilder builder = DiagnosticPropertiesBuilder();

Expand Down
21 changes: 0 additions & 21 deletions packages/flutter/test/material/toggle_buttons_theme_test.dart
View file
Open in desktop
Expand Up @@ -476,13 +476,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,
Expand Down Expand Up @@ -516,13 +509,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,
Expand Down Expand Up @@ -555,13 +541,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,
Expand Down