Repository navigation
[proposal] InkRipple and InkSplash implementations warrant improvements聽#73163
Description
Activity
- changed the title
[-]InkRipple and InkSplash implementations warrant improvements (addresses #20922 and #49957)[/-][+]InkRipple and InkSplash implementations warrant improvements[/+]on Dec 31, 2020 - changed the title
[-]InkRipple and InkSplash implementations warrant improvements[/-][+][proposal] InkRipple and InkSplash implementations warrant improvements[/+]on Dec 31, 2020 Thanks for this excellent write up! I had some notes on this subject. There is some overlap, but copying here to align:
Objective
Explore usages of ink in Flutter and recommend updates to make it easier to understand and extend.
What's the difference between InkSplash and InkRipple?Differences
- InkSplash.splashFactory defines the default splash.
- InkRipple.splashFactory defines a splash that spreads out more aggressively than the default.
- Looking at the implementation of InkRipple and implementation of InkSplash, it appears that the 2 private implementations of _getTargetRadius are different.
- _getClipCallback implemntation is the same in both.
- The InkRipple radius Tween is indeed more aggressive, starting at 30% the target radius, while the InkSplash starts at 0.
- The InkRipple Tween also ends with a circle that has a 5dp larger radius.
Action items
- AI1: refactor and reuse _getClipCallback
- AI2: InkRipple Tween documentation says 60% diameter, but it is defined as 30% the radius, which results in 30% of the diameter too.
- AI3: splashFactory can be renamed, because it doesn't always make a an InkSplash, it makes an InkFeature (superclass of InkSplash, InkRipple, etc), so inkFeatureFactory would be more accurate, but this is breaking. Can add inkFeatureFactory, deprecate inkSplashFactory, and follow deprecation procedure.
- AI4: InkFeatureFactory confirm()` docs may be incorrect.
/// Called when the user input that triggered this feature's appearance was confirmed.
///
/// Typically causes the ink to propagate faster across the material. By default this
/// method does nothing.
This actually only seems to trigger on press up or mouse up. - AI5: Add all references of InkFeatureFactory to class docs (InkSplash, InkRipple, InkHighlight, etc)
- AI6: Make more methods on InkRipple overridable and modular, such as extracting the transformation logic from the painting logic out of paintInkCircle and making both public.
Where are these Inks used?
InkSplash
Summary
The only place InkSplash is used is in the default ThemeData, so what components actually use the default ThemeData.splashFactory? Indirectly, almost all of them. (jump to conclusion)
Implementations
flutter/lib/src/material/ink_splash.dart
The implementation of InkSplash and _InkSplashFactoryUsages
- flutter/lib/src/material/theme_data.dart
- InkSplash.splashFactory is set as the default splashFactory
References in docs only
- flutter/lib/src/material/ink_well.dart
- InkSplash is only referenced in documentation for InteractiveInkFeature and InteractiveInkFeatureFactory
- flutter/lib/src/material/material.dart
- MaterialInkController has a method: void addInkFeature(InkFeature feature)
- Note that InteractiveInkFeature is the super class for InkSplash, InkRipple, and InkHighlight
- Also note that although there are references to InkSplash and InkHighlight, there are no references to InkRipple in this file.
- flutter/lib/src/material/ink_ripple.dart
- Only referenced in docs as a sibling implementation
InkRipple
Summary
InkRipple seems to be only used in the new button universe, or the 3 buttons that override the default splashFactory.
Implementations
flutter/lib/src/material/ink_well.dart
The implementation of InkRipple and _InkRippleFactoryUsages
- flutter/lib/src/material/text_button.dart
-TextButton's defaultStyleOf method sets the ButtonStyle's splashFactory as InkRipple.splashFactory - flutter/lib/src/material/elevated_button.dart
-ElevatedButton's defaultStyleOf method sets the ButtonStyle's splashFactory as InkRipple.splashFactory - flutter/lib/src/material/outlined_button.dart
-OutlinedButton's defaultStyleOf method sets the ButtonStyle's splashFactory as InkRipple.splashFactory
References in docs only
- flutter/lib/src/material/theme_data.dart
- InkRipple.splashFactory is referenced in docs above the default splashFactory
- flutter/lib/src/material/ink_well.dart
- InkRipple is only referenced in documentation for InteractiveInkFeature and InteractiveInkFeatureFactory
- flutter/lib/src/material/ink_splash.dart
- Only referenced in docs as a sibling implementation
鈿狅笍 Exceptions_LocationAwareInkRippleFactory is another subclass of InteractiveInkFeatureFactory.
It is not a great name because it actually uses Theme.of(splashContext).splashFactory.create; for its create method, which means it could actually be creating an InkSplash, InkRipple, or any other subclass of of InkFeatureFactory, but the name implies it creates an InkRipple. This could be easily renamed since it is private.InkHighlight
Summary
It is a little unclear how this exactly relates to either InkSplash or InkRipple. Hover and Focus make sense because they are mouse only, but what about Pressed? This looks like a possible artifact of M1, but differs from other platforms' M2 implementations.
Implementations
- flutter/lib/src/material/ink_highlight.dart
- The implementation of InkHighlight and _InkHighlightFactory
Usages
- flutter/lib/src/material/ink_well.dart
- Used for states: pressed, hover, focus
References in docs only
- flutter/lib/src/material/material.dart
- MaterialInkController has a method: void addInkFeature(InkFeature feature)
- Note that InteractiveInkFeature is the superclass for InkSplash, InkRipple, and InkHighlight
-Also note that although there are references to InkSplash and InkHighlight, there are no references to InkRipple in this file.
- flutter/lib/src/material/ink_splash.dart
- Only referenced in docs as a sibling implementation
- flutter/lib/src/material/ink_ripple.dart
- Only referenced in docs as a sibling implementation
What about InkWell and InkResponse?
InkWell has 48 references and InkResponse has 19 references in material/
- flutter/lib/src/material/ink_well.dart
- Definition of InkWell, InkResponse, and InkFeatureFactory are all here.
- InkWell extends InkResponse!
- InkWell is literally just an InkResponse with containedInkWell = false. Everything else is the same.
- This means it simply clips the ink.
InkWell and InkResponse take in InteractiveInkFeatureFactory? splashFactory. The resulting InteractiveInkFeature either comes from passing this in directly, or defaults to one created by the one from ThemeData (which defaults to InkSplash).
Conclusion
All InteractiveInkFeatures that respond to press in Material use InkSplash except for the 3 new universe buttons (TextButton, ElevatedButton, and OutlinedButton), which all use InkRipple. This is because most components themselves use either InkWell or InkResponse, which both default to the ThemeData's InkSplash, and do not appear to be overridden in any other material/ components. In addition to these InkFeatures, InkHighlight is always used on top of (or behind) for press, and are also used for other highlight states: hover and focus.
How do I apply new Ink across components, such as an M3 "Ripple"?
Start by creating a new InteractiveInkFeature and InteractiveInkFeatureFactory overriding the splashFactory in ThemeData and the 3 ButtonStyles!
Reacted by Will Lockwood and Hilmar WiegandUpdate for those following the issue: work is back in progress here. We're drafting a design doc that will plan out a bunch of improvements to these ink features, including each of these fidelity issues. When ready, I'll post the doc here so y'all can see what's up and have a chance to provide any feedback before we move forward.
Reacted by Yash Chauhan, Lasse Rosenow, El and xioxinReacted by Yash Chauhan and Lasse RosenowIs the design document still in progress?
Other notes that might already be covered but leaving as boxes to check:
- investigate why radius is passed to
createif is possibly derived fromreferenceBoxwhich is already passed- e.g.
_targetRadius = radius ?? _getTargetRadius(referenceBox, containedInkWell, rectCallback, position),probably does not need radius
- e.g.
- make
InteractiveInkFeature createconsistent and re-evaluate required and naming of params, such as textDirection -> customBorderTextDirection
- investigate why radius is passed to
- Change the contrast(opacity, alpha) of the sparkle click animation.
Flutter sparkle
Compose sparkle
Screenrecorder-2023-07-22-22-12-33-176.mp4
Screenrecorder-2023-07-22-22-11-31-106.mp4
- Increase the duration of the sparkle animation to the nearest corresponding native.
Note: Design doc/proposal is in progress; will link it here when ready 馃憤馃徎
Pre-issue notes:
This issue is intended to organize/contextualize the work for several small PRs, related to various possible improvements to the user's and developer's interactions with the
InkRippleandInkSplash. For all of the suggestions written below, unless explicitly specified otherwise, I've already gone through the process of making the improvements on other branches, but have decided to split up the work to make review/merging easier for everyone.So, this is intended to be an issue spanning many small improvements, which I plan to address in individual PRs. Many of the small improvements have been brought up in other issues; the work described below fully encapsulates the improvements described in at least issues #20922 and #49957).
This issue also presupposes the virtue of the point made by Marc Glasberg here: #20922 (comment)
I work in apps that are being migrated from Android to Flutter, and can attest to how annoying it is to have different implementations of the same view in one user experience. If anyone disagrees vehemently with this general point, I'd be happy to chat about it and come to a consensus on how to proceed.
Purpose:
This issue generally addresses the many small ways that the implementation and documentation of ink features (particularly, the
InkSplashandInkRipple) can be improved. The purposes of these two implementations should be more clear for developers, and the implementations themselves should more accurately reflect the way they're implemented on Android, where users may see Flutter material widgets in the same user experience as an Android material view.Proposed/Planned Changes:
I've broken down the improvements into buckets of work that can be addressed in individual, smaller PRs. I have code written already, and I'm planning on putting up PRs for each of these, in the order that they're written here. Click below each for screenshots demonstrating the inconsistencies between Android and Flutter.
SplashFactory Test Coverage: Currently, many tests fail after changing the default splash factory on the material Theme. To make the PR changing the default splash factory easier, test coverage should be improved for widgets using ink features, to be able to pass regardless of what the theme鈥檚 default splash factory is. We can take the opportunity here to consolidate some duplicated code for utils related to testing how ripples are drawn.
Default SplashFactory: As a quick follow-up to the first PR, we should also change the default splash factory to the
InkRipple.splashFactory, which is going to match the material animation for all Android APIs going forward. Currently,InkSplash.splashFactoryis the default, which is an outdated choice, that's getting more outdated by the day.SplashFactory Documentation:
InkRipple/InkSplashdocumentation should be more clear about their intended use cases. Specifically, it should state that theInkRippleis an updated version of the InkSplash, and that the two implementations represent the ripple animations seen on Android versions 9 and above (API 28+), and Android versions 5-8 (API 21-27), respectively.Screenshots:
Ripple Color: The Flutter material ripple color is not exactly correct, which causes a slight inconsistency, most clearly visible in the colors of overlapping ripples, or on ripples on non-white backgrounds.
Screenshots:
Ripple Initial Radius: The initial radius of the
InkRippleis 30% of its container鈥檚 largest dimension, but it should be 60%. Documentation of the Flutter implementation shows that it was intended to be 60%, so this is just a bug in the original implementation.Screenshots:
Ripple Target Radius: The target radius of the
InkRipple/InkSplashnot only lacks fidelity to the Android version (which is simpler/more readable, and more sensible), but is unnecessarily non-performant. Use the simpler logic, used in the Android source.Screenshots:
InkSplash:The difference for the
InkRippleis really hard to spot, but I'll make it clear in my PR. basically, you can't actually tell the difference of the size, except in how the target size affects the timing of the ripple's fade out animation. Especially with the other issues, it's not really worth adding a screenshot here for now.Position/Curves for Ripples/Splashes: The initial position/position of ink features, as well as the curves they follow as they expand, lacks fidelity to the Android implementation. The difference in initial position is more noticeable, but these are likely good things to address at the same time.
Screenshots:
Removing
InkRipple's highlight TheInkRippleshould not have associatedInkHighlightsdrawn for pressed gesture states. Remove it (or just make it fully transparent).Screenshots:
Ink Feature Cancel Tap Logic: The tap logic for when to initiate a splash is incorrect, as pointed out also by Marc Glasberg here: InkWell and InkResponse not close enough to the real thing聽#20922 (comment). It should be initiated on cancel or confirm of a tap, instead of on tapDown.
Screenshots:
TL;DR:
Basically, there are a bunch of ways we can improve the Ripple/Splash animations, to have higher quality and fidelity to the Android versions. I have a bunch of the work planned and written, and I'm planning on opening PRs that just go down the list above. At the end of all this, the InkSplash an InkRipple should look pretty damn close to the way they do on Android, and hopefully, implementation/review will be pretty smooth/painless.