flutter / flutter

Flutter makes it easy and fast to build beautiful apps for mobile and beyond
https://flutter.dev
BSD 3-Clause "New" or "Revised" License
166.2k stars 27.49k forks source link

Circular-shaped decorations are not properly clipped #13675

Closed sroddy closed 2 months ago

sroddy commented 6 years ago

Steps to Reproduce

I'm getting visual glitches using this widget structure:

new Container(
      decoration: new BoxDecoration(
        color: color,
        shape: BoxShape.circle,
        border: new Border.all(
          color: Colors.white,
          width: 2.5,
        ),
      ),
      child: new Center(
        child: new Text(
          "$index",
          style: theme.textTheme.body1.apply(color: textColor),
        ),
      ),
    );

image

As you can notice, the background color of the decoration is slightly overflowing the circular border. I've tried in different ways (e.g. using ClipOval) but the result is always the same.

I think this behaviour, even if intended, is not what a developer/designer would expect.

Is there a way to properly achieve the expected result?

Thanks

Hixie commented 6 years ago

Yeah that's not good.

sroddy commented 6 years ago

Btw, this issue is happening almost in the same way using native iOS Sdk. The reason there is because of anti-aliasing, I don't know if in this case it's the same.

Hixie commented 6 years ago

Oh it's definitely just antialiasing. But we should do better. (For example, maybe we can shrink the background color paint by half the border width or something.)

marcglasberg commented 6 years ago

@Hixie If you shrink the background color paint by half the border width, this will be visible when the border is semi-transparent. You should, instead, shrink it by a single real pixel. The antialias problem always happens within one single real pixel (real device pixel, not logical pixel). That's also why this problem is less visible when the device resolution is larger.

Hixie commented 6 years ago

We can't really shrink by one pixel; we don't know what the transformation matrix is.

marcglasberg commented 6 years ago

I see. We've been fixing antialias problems, usually gaps, in some places where there is no transformation, or where we know the transformation. That's not fixable then, since if you do what you said the shrinking of the background color will be visible when the border is semi-transparent. Or maybe you only do this if the border color is not transparent. Or you add a flag.

Hixie commented 6 years ago

Yeah...

We might be able to improve this at a lower level maybe, too.

Mahmoud2B commented 6 years ago

this issue still there, @Hixie did you guys find a fix?

fralways commented 5 years ago

Very annoying bug. I often want to do something like this in order to make circular button but cant do because of this problem with edges:

return InkWell(
      onTap: callback,
      child: Container(
        alignment: Alignment.center,
        decoration: BoxDecoration(
          color: Colors.red,
          shape: BoxShape.circle,
        ),
        child: Image.asset(
          'assets/images/someimage.png',
          fit: BoxFit.cover,
        ),
      ),
    );

photo_2019-04-03 15 49 12

Similar thing happens if using flutter's CircleAvatar - there are still image leftovers over Container's border

CircleAvatar(
  backgroundImage: AssetImage(
      "assets/someimage.png"),
  child: Container(
    decoration: BoxDecoration(
      shape: BoxShape.circle,
      border: Border.all(
        color: Colors.white,
        width: 5.0,
      ),
    ),
  ),
)

2019-04-03 15 54 06

amreniouinnovent commented 5 years ago

@Hixie I have non-smooth round corners in Android Tablet is it the same issue or I need to open a different issue? Check "Continue button" and the CustomClipper for green arrow and red arrow Tablet Info: API 22, DPI 210, Width 800px, Height 1280

My button code:

RaisedButton(
            padding: EdgeInsets.symmetric(
                vertical: AppFonts.localePadding(context, 10),
                horizontal: AppFonts.localePadding(context, 100)),
            color: AppColors.primary,
            shape: RoundedRectangleBorder(
                borderRadius: BorderRadius.circular(30),
                side: BorderSide(color: AppColors.textBlack, width: 1,style: BorderStyle.solid)),
            child: Text(
              'Continue',
              style: TextStyle(
                  color: AppColors.textBlack,
                  fontSize: AppFonts.localeSize(context, 18),
                  fontFamily: AppFonts.localeFont(context)),
            ),
            onPressed: () {},
          )

flutter_01

Omi231 commented 4 years ago

Wow! 2 years and still no fix

amreniouinnovent commented 4 years ago

@Omi231 They are working hard on stabilizing the system and they provide Flutter totally free. Try to investigate the issue and make a pull request you see how much the engine is complex.

artyomdevyatov commented 4 years ago

I've just faced the same issue...

Easy workaround:

return Container(
  width: 28,
  height: 28,
  decoration: BoxDecoration(
    color: Colors.grey, // border color
    shape: BoxShape.circle,
  ),
  child: Padding(
    padding: EdgeInsets.all(2), // border width
    child: Container( // or ClipRRect if you need to clip the content
      decoration: BoxDecoration(
        shape: BoxShape.circle,
        color: Colors.orange, // inner circle color
      ),
      child: Container(), // inner content
    ),
  ),
);

Note: inner content won't be clipped. If you want to clip inner content, use ClipRRect.

natebot13 commented 4 years ago

Glad to see there was a little activity here two weeks ago. I ran into this issue today while trying to make a simple color picker. Seeing the color outside the white border is kinda ugly. I know I could use the above workaround, but I would not have expected this to be an issue in the first place. I assume it's an anti-aliasing issue and I hope it gets some improvement soon!

TahaTesser commented 4 years ago

image

code sample ```dart import 'package:flutter/material.dart'; void main() => runApp(MyApp()); class MyApp extends StatelessWidget { @override Widget build(BuildContext context) { return MaterialApp( title: 'Material App', home: Home(), ); } } class Home extends StatelessWidget { @override Widget build(BuildContext context) { return Scaffold( appBar: AppBar( title: Text('Material App Bar'), ), body: Center( child: Container( height: 50, width: 100, decoration: new BoxDecoration( color: Colors.blue, shape: BoxShape.circle, border: new Border.all( color: Colors.white, width: 2.5, ), ), child: new Center( child: new Text( "2", style: Theme.of(context).textTheme.body1.apply(color: Colors.white), ), ), ), ), floatingActionButton: FloatingActionButton( child: Icon(Icons.add), onPressed: () {}, ), ); } } ```
flutter doctor -v ``` [✓] Flutter (Channel master, 1.22.0-10.0.pre.87, on Linux, locale en_US.UTF-8) • Flutter version 1.22.0-10.0.pre.87 at /home/taha/Code/flutter_master • Framework revision 4732a214a7 (3 days ago), 2020-09-04 21:05:02 -0400 • Engine revision ac8b9c4c52 • Dart version 2.10.0 (build 2.10.0-86.0.dev) [✓] Android toolchain - develop for Android devices (Android SDK version 30.0.2) • Android SDK at /home/taha/Code/sdk • Platform android-30, build-tools 30.0.2 • ANDROID_HOME = /home/taha/Code/sdk • Java binary at: /home/taha/Code/android-studio/jre/bin/java • Java version OpenJDK Runtime Environment (build 1.8.0_242-release-1644-b3-6222593) • All Android licenses accepted. [✓] Chrome - develop for the web • Chrome at google-chrome [✓] Linux toolchain - develop for Linux desktop • clang version 10.0.0-4ubuntu1 • cmake version 3.16.3 • ninja version 1.10.0 • pkg-config version 0.29.1 [✓] Android Studio (version 4.0) • Android Studio at /home/taha/Code/android-studio • Flutter plugin version 48.1.2 • Dart plugin version 193.7547 • Java version OpenJDK Runtime Environment (build 1.8.0_242-release-1644-b3-6222593) [✓] VS Code (version 1.48.2) • VS Code at /usr/share/code • Flutter extension version 3.14.1 [✓] Connected device (4 available) • Android SDK built for x86 (mobile) • emulator-5554 • android-x86 • Android 10 (API 29) (emulator) • Linux (desktop) • linux • linux-x64 • Linux • Web Server (web) • web-server • web-javascript • Flutter Tools • Chrome (web) • chrome • web-javascript • Google Chrome 85.0.4183.83 • No issues found! ``` ```bash [✓] Flutter (Channel master, 1.24.0-8.0.pre.375, on macOS 11.0.1 20B29 darwin-x64, locale en-GB) • Flutter version 1.24.0-8.0.pre.375 at /Users/tahatesser/Code/flutter_master • Framework revision a7f5fd5360 (27 hours ago), 2020-11-30 13:14:13 +0100 • Engine revision 20caf54969 • Dart version 2.12.0 (build 2.12.0-76.0.dev) [✓] Android toolchain - develop for Android devices (Android SDK version 30.0.2) • Android SDK at /Users/tahatesser/Code/sdk • Platform android-30, build-tools 30.0.2 • ANDROID_HOME = /Users/tahatesser/Code/sdk • Java binary at: /Applications/Android Studio.app/Contents/jre/jdk/Contents/Home/bin/java • Java version OpenJDK Runtime Environment (build 1.8.0_242-release-1644-b3-6915495) • All Android licenses accepted. [✓] Xcode - develop for iOS and macOS (Xcode 12.2) • Xcode at /Volumes/Extreme/Xcode.app/Contents/Developer • Xcode 12.2, Build version 12B45b • CocoaPods version 1.10.0 [✓] Chrome - develop for the web • Chrome at /Applications/Google Chrome.app/Contents/MacOS/Google Chrome [✓] Android Studio (version 4.1) • Android Studio at /Applications/Android Studio.app/Contents • Flutter plugin can be installed from: 🔨 https://plugins.jetbrains.com/plugin/9212-flutter • Dart plugin can be installed from: 🔨 https://plugins.jetbrains.com/plugin/6351-dart • Java version OpenJDK Runtime Environment (build 1.8.0_242-release-1644-b3-6915495) [✓] VS Code (version 1.51.1) • VS Code at /Applications/Visual Studio Code.app/Contents • Flutter extension version 3.16.0 [✓] Connected device (4 available) • RMX2001 (mobile) • EUYTFEUSQSRGDA6D • android-arm64 • Android 10 (API 29) • macOS (desktop) • macos • darwin-x64 • macOS 11.0.1 20B29 darwin-x64 • Web Server (web) • web-server • web-javascript • Flutter Tools • Chrome (web) • chrome • web-javascript • Google Chrome 87.0.4280.67 • No issues found! ```
Omi231 commented 4 years ago

Guys please give this issue some priority 😰 It screws almost every design which require rounded corners.

bsbilal commented 3 years ago

is there any solution? image

wess commented 3 years ago

Hitting this now as well and our "major fintech" app uses a lot of circles, do we know the status?

naamapps commented 3 years ago

Hey, the issue still exists in version 2.2 Is there progress on this?

Mixpeal commented 3 years ago

@naamapps check this little hack

Container(
  width: 63.0,
  height: 63.0,
  decoration: BoxDecoration(
      shape: BoxShape.circle,
      border: Border.all(
        color: Color(0xFFF1DEC1),
        width: 3,
      )),
  child: Container(
    width: 60.0,
    height: 60.0,
    decoration: BoxDecoration(
        image: DecorationImage(
            fit: BoxFit.fitWidth,
            image: Image.asset(
              "assets/images/sample.jpg",
            ).image),
        shape: BoxShape.circle),
  ),
),
CodingAleCR commented 2 years ago

This issue also affects the CircularProgressIndicator widget. I had to go at it in a different way, I placed a Stack and clipped the widget using a Container to clip the inside shadow and another CircularProgressIndicator to clip the outside shadow. It is a bit hard to explain and definitely not a robust way to handle it, but due to urgency, I had to go at it this way.

A couple of improvements that could be done would be to replace the CircularProgressIndicator used for the outer shadow and the Container used for the inner shadow with a CustomPaint or even create a custom clipper for it.

Anyways, you can see a DartPad of the problem here and you can see the code in detail of how I solved it here.

naamapps commented 2 years ago

Still exists in 2.5.3 😕 Flutter team, this issue exists for 4 years now. Please can someone take a look at this? Such a small thing but it degrades the UI and makes it unprofessional.

I know there are alternatives and hacks to prevent it from happening but it should anyway be fixed as part of the framework and behave as expected - without the color bleed beyond the borders.

Carl-CWX commented 2 years ago

Just had the same problem, with Flutter 2.5.3 Stable Container with a boarder showing a white outline image

Shall try a work around!

alireza7991 commented 2 years ago

4 Years, no fix! TY flutter.

orangeswim commented 2 years ago

wanted to bump this issue. image

Container(
                            margin:
                                const EdgeInsets.symmetric(horizontal: 16.0),
                            width: 130,
                            height: 130,
                            decoration: BoxDecoration(
                              image: DecorationImage(
                                image: CachedNetworkImageProvider(
                                    content.imageUrl),
                                fit: BoxFit.cover,
                              ),
                              border: Border.all(width: 4.0),
                              shape: BoxShape.circle,
                            )),
bernaferrari commented 2 years ago

I think I can solve this. As 100% of reported issues use no opacity, what if I made the background paint until 50% the side.width? Is this a good solution? If there is opacity, it paints 100% like before, and the bug happens. But for 90% of people, the bug would be gone forever.

image

The new StrokeAlign property (the PR is open) would also help you, with a StrokeAlign value of -0.95.

Edit: solved. What do you think?

image

Edit 2: we might need to add isShadow to getInnerPath/getOuterPath so the paint (color/gradient) gets reduced while the shadow doesn't. Slightly tricky, potentially breaking (goldens), but fairly doable.

danagbemava-nc commented 1 year ago

Reproduces on the latest versions of flutter.

Screenshot 2023-03-10 at 10 20 44
updated sample ```dart import 'package:flutter/material.dart'; void main() => runApp(const MyApp()); class MyApp extends StatelessWidget { const MyApp({super.key}); @override Widget build(BuildContext context) { return const MaterialApp( title: 'Material App', home: Home(), ); } } class Home extends StatelessWidget { const Home({super.key}); @override Widget build(BuildContext context) { return Scaffold( appBar: AppBar( title: const Text('Material App Bar'), ), body: Center( child: Container( height: 150, width: 150, decoration: BoxDecoration( color: Colors.blue, shape: BoxShape.circle, border: Border.all( color: Colors.white, width: 2.5, ), ), ), ), floatingActionButton: FloatingActionButton( child: const Icon(Icons.add), onPressed: () {}, ), ); } } ```
flutter doctor -v ``` [✓] Flutter (Channel stable, 3.7.7, on macOS 13.2.1 22D68 darwin-arm64, locale en-GB) • Flutter version 3.7.7 on channel stable at /Users/nexus/dev/sdks/flutter • Upstream repository https://github.com/flutter/flutter.git • Framework revision 2ad6cd72c0 (2 days ago), 2023-03-08 09:41:59 -0800 • Engine revision 1837b5be5f • Dart version 2.19.4 • DevTools version 2.20.1 [✓] Android toolchain - develop for Android devices (Android SDK version 33.0.0) • Android SDK at /Users/nexus/Library/Android/sdk • Platform android-33, build-tools 33.0.0 • Java binary at: /Users/nexus/Library/Application Support/JetBrains/Toolbox/apps/AndroidStudio/ch-0/221.6008.13.2211.9477386/Android Studio.app/Contents/jbr/Contents/Home/bin/java • Java version OpenJDK Runtime Environment (build 11.0.15+0-b2043.56-8887301) • All Android licenses accepted. [✓] Xcode - develop for iOS and macOS (Xcode 14.2) • Xcode at /Applications/Xcode.app/Contents/Developer • Build 14C18 • CocoaPods version 1.11.3 [✓] Chrome - develop for the web • Chrome at /Applications/Google Chrome.app/Contents/MacOS/Google Chrome [✓] Android Studio (version 2022.1) • Android Studio at /Applications/Android Studio.app/Contents • Flutter plugin can be installed from: 🔨 https://plugins.jetbrains.com/plugin/9212-flutter • Dart plugin can be installed from: 🔨 https://plugins.jetbrains.com/plugin/6351-dart • Java version OpenJDK Runtime Environment (build 11.0.15+0-b2043.56-8887301) [✓] Android Studio (version 2022.1) • Android Studio at /Users/nexus/Library/Application Support/JetBrains/Toolbox/apps/AndroidStudio/ch-0/221.6008.13.2211.9477386/Android Studio.app/Contents • Flutter plugin can be installed from: 🔨 https://plugins.jetbrains.com/plugin/9212-flutter • Dart plugin can be installed from: 🔨 https://plugins.jetbrains.com/plugin/6351-dart • Java version OpenJDK Runtime Environment (build 11.0.15+0-b2043.56-8887301) [✓] IntelliJ IDEA Community Edition (version 2022.3.3) • IntelliJ at /Applications/IntelliJ IDEA CE.app • Flutter plugin version 72.1.4 • Dart plugin version 223.8836.5 [✓] VS Code (version 1.76.1) • VS Code at /Applications/Visual Studio Code.app/Contents • Flutter extension version 3.60.0 [✓] Connected device (4 available) • M2007J20CG (mobile) • adb-5dd3be00-17AYzd._adb-tls-connect._tcp. • android-arm64 • Android 12 (API 31) • Nexus (mobile) • 00008020-001875E83A38002E • ios • iOS 16.3.1 20D67 • macOS (desktop) • macos • darwin-arm64 • macOS 13.2.1 22D68 darwin-arm64 • Chrome (web) • chrome • web-javascript • Google Chrome 111.0.5563.64 [✓] HTTP Host Availability • All required HTTP hosts are available • No issues found! ``` ``` [!] Flutter (Channel master, 3.9.0-1.0.pre.144, on macOS 13.2.1 22D68 darwin-arm64, locale en-GB) • Flutter version 3.9.0-1.0.pre.144 on channel master at /Users/nexus/dev/sdks/flutters ! Warning: `flutter` on your path resolves to /Users/nexus/dev/sdks/flutter/bin/flutter, which is not inside your current Flutter SDK checkout at /Users/nexus/dev/sdks/flutters. Consider adding /Users/nexus/dev/sdks/flutters/bin to the front of your path. ! Warning: `dart` on your path resolves to /Users/nexus/dev/sdks/flutter/bin/dart, which is not inside your current Flutter SDK checkout at /Users/nexus/dev/sdks/flutters. Consider adding /Users/nexus/dev/sdks/flutters/bin to the front of your path. • Upstream repository https://github.com/flutter/flutter.git • Framework revision ac550c6356 (7 hours ago), 2023-03-09 21:59:21 -0500 • Engine revision e9ca7b2c45 • Dart version 3.0.0 (build 3.0.0-313.0.dev) • DevTools version 2.22.2 • If those were intentional, you can disregard the above warnings; however it is recommended to use "git" directly to perform update checks and upgrades. [✓] Android toolchain - develop for Android devices (Android SDK version 33.0.0) • Android SDK at /Users/nexus/Library/Android/sdk • Platform android-33, build-tools 33.0.0 • Java binary at: /Users/nexus/Library/Application Support/JetBrains/Toolbox/apps/AndroidStudio/ch-0/221.6008.13.2211.9477386/Android Studio.app/Contents/jbr/Contents/Home/bin/java • Java version OpenJDK Runtime Environment (build 11.0.15+0-b2043.56-8887301) • All Android licenses accepted. [✓] Xcode - develop for iOS and macOS (Xcode 14.2) • Xcode at /Applications/Xcode.app/Contents/Developer • Build 14C18 • CocoaPods version 1.11.3 [✓] Chrome - develop for the web • Chrome at /Applications/Google Chrome.app/Contents/MacOS/Google Chrome [✓] Android Studio (version 2022.1) • Android Studio at /Applications/Android Studio.app/Contents • Flutter plugin can be installed from: 🔨 https://plugins.jetbrains.com/plugin/9212-flutter • Dart plugin can be installed from: 🔨 https://plugins.jetbrains.com/plugin/6351-dart • Java version OpenJDK Runtime Environment (build 11.0.15+0-b2043.56-8887301) [✓] Android Studio (version 2022.1) • Android Studio at /Users/nexus/Library/Application Support/JetBrains/Toolbox/apps/AndroidStudio/ch-0/221.6008.13.2211.9477386/Android Studio.app/Contents • Flutter plugin can be installed from: 🔨 https://plugins.jetbrains.com/plugin/9212-flutter • Dart plugin can be installed from: 🔨 https://plugins.jetbrains.com/plugin/6351-dart • Java version OpenJDK Runtime Environment (build 11.0.15+0-b2043.56-8887301) [✓] IntelliJ IDEA Community Edition (version 2022.3.3) • IntelliJ at /Applications/IntelliJ IDEA CE.app • Flutter plugin version 72.1.4 • Dart plugin version 223.8836.5 [✓] VS Code (version 1.76.1) • VS Code at /Applications/Visual Studio Code.app/Contents • Flutter extension version 3.60.0 [✓] Connected device (4 available) • M2007J20CG (mobile) • adb-5dd3be00-17AYzd._adb-tls-connect._tcp. • android-arm64 • Android 12 (API 31) • Nexus (mobile) • 00008020-001875E83A38002E • ios • iOS 16.3.1 20D67 • macOS (desktop) • macos • darwin-arm64 • macOS 13.2.1 22D68 darwin-arm64 • Chrome (web) • chrome • web-javascript • Google Chrome 111.0.5563.64 [✓] Network resources • All expected network resources are available. ! Doctor found issues in 1 category. ```
sabatage3 commented 7 months ago

Almost 7 years have passed, any updates on this? i really love Flutter, but that weird outline makes the UI look so cheap

bernaferrari commented 7 months ago

I made a PR but it broke internal Google stuff that I can't test because I don't have access and I never tried again. Right now with customer testing it would be waaay harder to land this change :(

erlangparasu commented 3 months ago

2017-2024

Any updates?

bernaferrari commented 3 months ago

I gave up because I don't have access to Google internal testing

erlangparasu commented 3 months ago

Maybe SVG as alternative?

bernaferrari commented 3 months ago

Well, 1) flutter doesn't support svg, 2) you don't want this to happen in basic shapes.

gnprice commented 3 months ago

Right now with customer testing it would be waaay harder to land this change :(

FWIW I doubt customer testing will add much to the difficulty of fixing this.

Failures in Google internal tests are a bummer, though.


OTOH I took a look just now at the previous PR, and it reads like it got pretty close to merge — in particular it seems like the Google internal changes were deemed to be OK: https://github.com/flutter/flutter/pull/122317#issuecomment-1526027980

So for anyone who wants to try to further take this forward (whether @bernaferrari again or someone else), I think it may not be a lot of work to start from #122317, rebase it, handle the couple of nits starting at https://github.com/flutter/flutter/pull/122317#pullrequestreview-1489212518 , and get it merged.

bernaferrari commented 3 months ago

The issue was that Google testing problem seemed bad (it was affecting some apparently unintended things) and I have no way to debug. I can submit again if you want to debug for me, or point me in the right direction.

The issue with customer testing is with super apps and Cupertino macOS testing that got a lot of bounding checks, so I think they might be affected and the only way to "solve" would be commenting the test, landing this, fixing the test. There is no partial migration.

For Google testing it is going to fail in 200 places, but as long as you are willing to update the hard coded values, it is not a problem.

gnprice commented 3 months ago

My reading of the thread is that after spending a bit of time looking at the Google-internal changes, @gspencergoog concluded they were fine. He wrote:

They're pretty trivial, so as long as we can say that they are more correct now than they were, I'm OK with approving them.


The issue with customer testing is with super apps and Cupertino macOS testing that got a lot of bounding checks, so I think they might be affected and the only way to "solve" would be commenting the test, landing this, fixing the test. There is no partial migration.

Yeah, that sort of effect is possible. I think the usual solution is to temporarily disable the affected suite in flutter/tests: rename the file registry/foo.test to registry/foo.test.disabled, then land the framework change, then land the test update downstream, then rename the file back.

If by "Cupertino macOS testing" you're referring to the suite registry/macos_ui.test, you may be glad to hear that that one is disabled since last December: https://github.com/flutter/tests/pull/317 .

bernaferrari commented 3 months ago

Greg said the changes were ok, but he manually checked some files and there were some small things that seemed "odd" and unintended, like Chips/Buttons were being affected somehow (possibly Material State, but I really don't know). They weren't supposed to be affected in the way he saw, but I was never able to reproduce locally, and I can't trigger the golden of what he saw, so it is hard.

gnprice commented 3 months ago

Hmm, I see, dang. Did that discussion happen somewhere else, then (perhaps Discord)? It doesn't seem like any reference to it made it onto the PR thread.

bernaferrari commented 3 months ago

Here: https://github.com/flutter/flutter/pull/122317#issuecomment-1526026433

gnprice commented 3 months ago

I see. Well, that's the comment that was followed by this one:

They're pretty trivial, so as long as we can say that they are more correct now than they were, I'm OK with approving them.

So I think those (as well as the other changes like them) are the changes that he was referring to in that later comment.

If the reply to those changes is, "I tried and couldn't reproduce a change like those locally; but they are visually trivial, and the new versions don't look any worse to me than the old versions, and I think in principle they should be more correct because the new logic in the PR is more correct", then my guess based on the thread is that @gspencergoog (and any other Googlers involved) would have been happy with that answer.

Looking at those screenshots myself, I'd definitely say they're visually trivial and that the new versions don't look any worse than the old. I haven't closely read the logic in the PR, but presumably you think it is indeed more correct. So if you tried to reproduce similar changes and couldn't, then I think that's probably all the debugging that is needed.

bernaferrari commented 3 months ago

The main issue is not that they look odd or wrong, just "are you sure your change should happen here?" and I'm not sure. I wish I could debug to be sure, there is a 75% chance that's a bug, but I can't debug it.

gnprice commented 3 months ago

OK, I guess it comes down to that estimate of whether it's a bug. If you think it probably really is a bug in the PR, then I agree it should be fixed before merging, and it's frustrating that the repro case for the bug isn't accessible.

FWIW my guess would be that it's not a bug. Those three gray "1", "2", "3" boxes don't visibly have borders… but they might still have a _decoration.shape that's an OutlinedBorder, say, just with a border that's the same color as the interior. Based on the code in the PR, it seems like something like that has to be the case in order for it to have an effect.

And then once the new logic gets involved, it's expected that it may cause small rounding differences that make visually imperceptible changes in the output.

gspencergoog commented 3 months ago

We were actually pretty close, I was OK with just approving those diffs, I just wanted to make sure they were expected. If you want help with the Google testing side, I can look at them and approve if they're still trivial. I doubt many other customers do golden tests, but we can deal with them if they do (and if they do, you can reproduce it there).

bernaferrari commented 3 months ago

Could be. If @gnprice thinks that's not a bug, I can resubmit the PR. My brain melted thinking "is that a bug or not?" and I couldn't find an answer, then I gave up.

gnprice commented 2 months ago

Yeah, I can't claim to be sure of it, but my assessment is that it's probably not a bug. A resubmitted version of the PR sounds great!

If we want to get to a higher degree of certainty that those golden diffs aren't telling us about some way in which the code isn't behaving how it's intended to, then since you've already tried and been unable to reproduce the behavior locally, probably the most effective next step would be for @gspencergoog or another Googler to dig into one of those test cases that doesn't visually have a border and confirm that the widgets do express a border after all. (And the border is just very thin, or the same color as the interior or exterior, or for some other reason doesn't appear visually.) But personally I'd be comfortable skipping that step.

github-actions[bot] commented 2 months ago

This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of flutter doctor -v and a minimal reproduction of the issue.