Fix single ToggleButton border painting bugs #73780

xu-baolin · 2021-01-12T10:23:09Z

This patch fixes two bugs:

1, Does not apply border-radius when there is only one button

Expected:

Actual:

2, Border-radius paint incorrect when `Radius.x` or `Radius.y` equal 0.0

  borderRadius: BorderRadius.only(
    topRight: Radius.elliptical(10, 0),
    topLeft: Radius.elliptical(0, 10),
    bottomRight: Radius.elliptical(0, 10),
    bottomLeft: Radius.elliptical(10, 0),
  ),

Expected:

Actual:

Related Issues

Fixes #73725

flutter-dashboard · 2021-01-12T10:23:12Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

xu-baolin · 2021-01-12T11:03:17Z

packages/flutter/lib/src/material/toggle_buttons.dart

+      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,


This change fixes the #2 bug.

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.

Exactly correct.

xu-baolin · 2021-01-12T11:11:39Z

packages/flutter/lib/src/material/toggle_buttons.dart

+    // Only one button.
+    if (isFirstButton && isLastButton) {
+      final Path leadingPath = Path();
+      final double startX = (rrect.brRadiusX == 0.0) ? outer.right : rrect.right - rrect.brRadiusX;


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:

Nice, good catch here

skia-gold · 2021-01-12T11:11:58Z

Gold has detected about 9 untriaged digest(s) on patchset 2.
View them at https://flutter-gold.skia.org/cl/github/73780

xu-baolin · 2021-01-12T11:21:35Z

packages/flutter/test/material/toggle_buttons_test.dart

@@ -891,13 +891,6 @@ void main() {
      expect(
        toggleButtonRenderObject,
        paints
-          // trailing side


If only one button, we can paint the border with one path now.

skia-gold · 2021-01-12T11:29:58Z

Gold has detected about 6 untriaged digest(s) on patchset 3.
View them at https://flutter-gold.skia.org/cl/github/73780

skia-gold · 2021-01-12T11:50:58Z

Gold has detected about 6 untriaged digest(s) on patchset 4.
View them at https://flutter-gold.skia.org/cl/github/73780

flutter-dashboard · 2021-01-12T12:27:04Z

Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change).

If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Changes reported for pull request #73780 at sha e1400cf

shihaohong

LGTM with a small nit. Thanks for the fix!

shihaohong · 2021-01-14T05:39:37Z

packages/flutter/lib/src/material/toggle_buttons.dart

+      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,


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.

shihaohong · 2021-01-14T05:43:11Z

packages/flutter/lib/src/material/toggle_buttons.dart

+    // Only one button.
+    if (isFirstButton && isLastButton) {
+      final Path leadingPath = Path();
+      final double startX = (rrect.brRadiusX == 0.0) ? outer.right : rrect.right - rrect.brRadiusX;


Nice, good catch here

packages/flutter/test/material/toggle_buttons_test.dart

xu-baolin · 2021-01-14T05:58:50Z

Thanks for the review.
Could you triage the golden files for me additional? There seems to be something wrong with my permissions.

Co-authored-by: Shi-Hao Hong <shihaohong@google.com>

skia-gold · 2021-01-14T06:23:58Z

Gold has detected about 9 untriaged digest(s) on patchset 6.
View them at https://flutter-gold.skia.org/cl/github/73780

flutter-dashboard · 2021-01-14T07:00:14Z

Golden file changes are available for triage from new commit, Click here to view.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Changes reported for pull request #73780 at sha 6dda1a6

shihaohong · 2021-01-14T07:13:18Z

Just approved the gold triage 👍

fix toggleButton border paint bug

01c61a3

flutter-dashboard bot added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels Jan 12, 2021

google-cla bot added the cla: yes label Jan 12, 2021

xu-baolin requested a review from shihaohong January 12, 2021 10:23

fix test

37de0f3

xu-baolin commented Jan 12, 2021

View reviewed changes

fix test

fb59b76

xu-baolin commented Jan 12, 2021

View reviewed changes

fix test

e1400cf

flutter-dashboard bot added the will affect goldens Changes to golden files label Jan 12, 2021

shihaohong approved these changes Jan 14, 2021

View reviewed changes

shihaohong changed the title ~~fix toggleButton border paint bug~~ Fix single ToggleButton border painting bugs Jan 14, 2021

xu-baolin and others added 2 commits January 14, 2021 14:12

Update packages/flutter/test/material/toggle_buttons_test.dart

9557c92

Co-authored-by: Shi-Hao Hong <shihaohong@google.com>

Update packages/flutter/test/material/toggle_buttons_test.dart

6dda1a6

Co-authored-by: Shi-Hao Hong <shihaohong@google.com>

shihaohong added the waiting for tree to go green label Jan 14, 2021

fluttergithubbot merged commit 3cd837f into flutter:master Jan 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix single ToggleButton border painting bugs #73780

Fix single ToggleButton border painting bugs #73780

xu-baolin commented Jan 12, 2021

flutter-dashboard bot commented Jan 12, 2021

xu-baolin Jan 12, 2021

shihaohong Jan 14, 2021

xu-baolin Jan 14, 2021

xu-baolin Jan 12, 2021 •

edited

shihaohong Jan 14, 2021

skia-gold commented Jan 12, 2021

xu-baolin Jan 12, 2021

skia-gold commented Jan 12, 2021

skia-gold commented Jan 12, 2021

flutter-dashboard bot commented Jan 12, 2021

shihaohong left a comment

shihaohong Jan 14, 2021

shihaohong Jan 14, 2021

xu-baolin commented Jan 14, 2021

skia-gold commented Jan 14, 2021

flutter-dashboard bot commented Jan 14, 2021

shihaohong commented Jan 14, 2021

Fix single ToggleButton border painting bugs #73780

Fix single ToggleButton border painting bugs #73780

Conversation

xu-baolin commented Jan 12, 2021

1, Does not apply border-radius when there is only one button

Expected:

Actual:

2, Border-radius paint incorrect when Radius.x or Radius.y equal 0.0

Expected:

Actual:

Related Issues

flutter-dashboard bot commented Jan 12, 2021

xu-baolin Jan 12, 2021

Choose a reason for hiding this comment

shihaohong Jan 14, 2021

Choose a reason for hiding this comment

xu-baolin Jan 14, 2021

Choose a reason for hiding this comment

xu-baolin Jan 12, 2021 • edited

Choose a reason for hiding this comment

shihaohong Jan 14, 2021

Choose a reason for hiding this comment

skia-gold commented Jan 12, 2021

xu-baolin Jan 12, 2021

Choose a reason for hiding this comment

skia-gold commented Jan 12, 2021

skia-gold commented Jan 12, 2021

flutter-dashboard bot commented Jan 12, 2021

shihaohong left a comment

Choose a reason for hiding this comment

shihaohong Jan 14, 2021

Choose a reason for hiding this comment

shihaohong Jan 14, 2021

Choose a reason for hiding this comment

xu-baolin commented Jan 14, 2021

skia-gold commented Jan 14, 2021

flutter-dashboard bot commented Jan 14, 2021

shihaohong commented Jan 14, 2021

2, Border-radius paint incorrect when `Radius.x` or `Radius.y` equal 0.0

xu-baolin Jan 12, 2021 •

edited