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.

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

IconButton's splash radius is too large on an AppBar #64702

Closed
sidrao2006 opened this issue Aug 27, 2020 · 33 comments · Fixed by #109349, #109432 or #110722
Closed

IconButton's splash radius is too large on an AppBar #64702

sidrao2006 opened this issue Aug 27, 2020 · 33 comments · Fixed by #109349, #109432 or #110722
Assignees
Labels
a: quality A truly polished experience f: material design flutter/packages/flutter/material repository. found in release: 1.20 Found to occur in 1.20 framework flutter/packages/flutter repository. See also f: labels. has reproducible steps The issue has been confirmed reproducible and is ready to work on r: fixed Issue is closed as already fixed in a newer version

Comments

@sidrao2006
Copy link
Contributor

The IconButton does follow the default material specs and has a splash radius of about 24px. This is definitely not suitable for auto generated icons like the back button and the scaffold drawer button.

Though the material design does specify a set splash radius, an overflow is definitely not intended. The icon button's splash always overflows in AppBars.

Possible solutions -

  1. Allow setting the default splash radius for all auto generated icon buttons in AppBars.

  2. Set the default splash radius so that it doesnt overflow atleast - in the defaul AppBar

@sidrao2006
Copy link
Contributor Author

Possibly related to inactive issue - #31194

@darshankawar
Copy link
Member

Hi @sidrao2006,
Can you provide a screenshot of the splash effect that overflows the app bar ?
Also, provide flutter doctor -v.
Thanks.

@darshankawar darshankawar added in triage Presently being triaged by the triage team waiting for customer response The Flutter team cannot make further progress on this issue until the original reporter responds labels Aug 27, 2020
@sidrao2006
Copy link
Contributor Author

sidrao2006 commented Aug 27, 2020

It was easier to use a dartpad for this as there were very few changes needed.
Link to dartpad

Changes made

actions: [
IconButton(
icon: Icon(Icons.ac_unit),
onPressed: () => print('Click'),
),
],

@no-response no-response bot removed the waiting for customer response The Flutter team cannot make further progress on this issue until the original reporter responds label Aug 27, 2020
@sidrao2006
Copy link
Contributor Author

CC @pedromassango, @TahaTesser

@dnfield
Copy link
Contributor

dnfield commented Aug 27, 2020

Why not just set the splashSize property to your desired value?

@HansMuller HansMuller added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels Aug 27, 2020
@sidrao2006
Copy link
Contributor Author

Why not just set the splashSize property to your desired value?

@dnfield it is not possible to the splashRadius in auto generated IconButtons like the back button and the open drawer button.

@dnfield
Copy link
Contributor

dnfield commented Aug 27, 2020

Ahh, ok. It sounds like appBar may need to have some extra property to pass through to iconButtons it creates.

I did check on some other Android apps and it looks ike the splash does not typically overflow the bar.

That said, app bars can be different sizes, and some people may want the overflow, so whatever is done here shouldn't be a change to defaults on IconButton.

/cc @willlarche @rami-a who might know the right person on the Material team to look at this.

@dnfield dnfield changed the title IconButton's splash radius is too large IconButton's splash radius is too large on an AppBar Aug 27, 2020
@dnfield dnfield added the a: quality A truly polished experience label Aug 27, 2020
@dnfield
Copy link
Contributor

dnfield commented Aug 27, 2020

Also, based on a quick look at this code: https://github.com/flutter/flutter/tree/master/packages/flutter/lib/src/material/ink_highlight.dart#L122

Seems a little fishy to me - I wonder if there's some way we should be making sure that the highlight is drawn as a percentage of the constraints rather than as a fixed amount.

@rami-a
Copy link
Member

rami-a commented Aug 27, 2020

Thanks for bringing this up @sidrao2006. I think that there's definitely an improvement we can make here to allow for the out-of-the-box IconButtons inside app bars (Back button, menu button) to use a smaller splash radius. I've reached out to our designers to get some additional guidance on this and I'll report back once I have some.

@sidrao2006
Copy link
Contributor Author

sidrao2006 commented Aug 27, 2020

I think I was able to get somewhere, taking @dnfield 's suggestion in mind. Now, I just need to run some tests

@Hixie
Copy link
Contributor

Hixie commented Aug 27, 2020

The IconButton does follow the default material specs and has a splash radius of about 24px.

I thought we did follow the specs... they may have changed, though. Do you have a link to the spec we should follow?

@Hixie
Copy link
Contributor

Hixie commented Aug 27, 2020

As far as how to control the splash size, that seems like something that we should put in the theme. cc @HansMuller

@sidrao2006
Copy link
Contributor Author

sidrao2006 commented Aug 28, 2020

@Hixie , thanks for replying, as I have mentioned, flutter indeed does follow the material spec.

Amazing, I look into the themes and open a pr. But I am still confused whether to apply that to all the IconButtons (which is a more predictable behaviour) or apply it only to the IconButtons generated by autoImplyLeading

@darshankawar darshankawar added found in release: 1.20 Found to occur in 1.20 has reproducible steps The issue has been confirmed reproducible and is ready to work on passed first triage and removed in triage Presently being triaged by the triage team labels Aug 28, 2020
@darshankawar
Copy link
Member

This is how it looks on latest stable (1.20.2) when we tap on iconButton in appbar:

64702

flutter doctor -v
Flutter 1.20.2 • channel stable • https://github.com/flutter/flutter.git
Framework • revision bbfbf1770c (15 hours ago) • 2020-08-13 08:33:09 -0700
Engine • revision 9d5b21729f
Tools • Dart 2.9.1

Running flutter doctor...
Doctor summary (to see all details, run flutter doctor -v):
[✓] Flutter (Channel stable, 1.20.2, on Mac OS X 10.15.2 19C57, locale en-US)
[!] Android toolchain - develop for Android devices (Android SDK version 29.0.3)
    ✗ Android license status unknown.
      Try re-installing or updating your Android SDK Manager.
      See https://developer.android.com/studio/#downloads or visit https://flutter.dev/docs/get-started/install/macos#android-setup for detailed instructions.
[✓] Xcode - develop for iOS and macOS (Xcode 11.6)
[✓] Android Studio (version 3.6)
[✓] Connected device (1 available)

! Doctor found issues in 1 category.

@sidrao2006
Copy link
Contributor Author

@HansMuller, @dnfield , @Hixie can you please review my solution plan

  1. Create a property named iconButtonSplashRadius in ThemeData and then set the default value to overflow by following the material spec.

  2. Add this property to AppBarTheme as well so that it can be customized only for the app bar too.

  3. Set IconButton.splashRadius to Theme.of(context).iconButtonSplashRadius in icon_button.dart.

On approval, I will add these to my PR's TODO.

@willlarche
Copy link
Member

@rami-a is working with designers to confirm the spec. The specs change and what's published isn't always the latest guidance. We're proud that on Flutter we often get the latest guidance before it's even published so our clients can have the benefit of new features and improvements.

Also, sometimes the spec does change without alerting us that we need to make a code change. This happens much less often these days but happened a lot in earlier years.

Either way, we need to confirm what the designers want us to do here. And @rami-a is working on that. So, sit tight and we'll get back to you when this information request comes up in the queue!

@TahaTesser
Copy link
Member

TahaTesser commented Dec 1, 2021

#93478 landed

Nevercode automation moved this from PR submitted to Done (PR merged) Dec 1, 2021
@TahaTesser TahaTesser added r: fixed Issue is closed as already fixed in a newer version and removed r: fixed Issue is closed as already fixed in a newer version labels Dec 1, 2021
@TahaTesser TahaTesser reopened this Dec 3, 2021
Nevercode automation moved this from Done (PR merged) to In progress Dec 3, 2021
@TahaTesser
Copy link
Member

#93478 is reverted, I am working on reimplementation

@Enzodtz
Copy link

Enzodtz commented Feb 15, 2022

What is the status of this? Thanks

@GayatriDunakhe
Copy link

The icon splashRadius look this so
image

I try to do this -
splashRadius: Material.defaultSplashRadius / 2
then it dosen't show any click moment on that app bar

image

How I deal with this?

@QuncCccccc
Copy link
Contributor

Hi! The IconButton component is recently updated to the Material Design 3 spec, so the splash radius issue will no longer exist. Here's the issue link which contains the related PRs: #103528

Please let me know if there are any questions:) Thanks!

@FeodorFitsner
Copy link

Is it by design that IconButton in AppBar is not having a round splash now?

image

@QuncCccccc
Copy link
Contributor

The PR is reverted, so I re-opened this issue. Will add a reland soon.

@darshankawar darshankawar removed the r: fixed Issue is closed as already fixed in a newer version label Aug 26, 2022
@darshankawar darshankawar added the r: fixed Issue is closed as already fixed in a newer version label Sep 2, 2022
@github-actions
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
a: quality A truly polished experience f: material design flutter/packages/flutter/material repository. found in release: 1.20 Found to occur in 1.20 framework flutter/packages/flutter repository. See also f: labels. has reproducible steps The issue has been confirmed reproducible and is ready to work on r: fixed Issue is closed as already fixed in a newer version
Projects
None yet