`SplashFactory` in the widgets library #149963

nate-thegrate · 2024-06-08T21:12:26Z

This pull request transforms ink effects to be completely design system-agnostic.

Original names	"Splash" names
`InkFeature`	—
`InteractiveInkFeature`	`Splash`
`InteractiveInkFeatureFactory`	`SplashFactory`
—	`SplashBox`
`MaterialInkController`	`SplashController`
`debugCheckHasMaterial`	`debugCheckSplash`

Merging this PR will resolve #147392 and make progress toward #48017.

Handling breakages:

justinmc

@nate-thegrate Have you seen the breaking changes policy? We'll have to follow it if we include these changes that break the Google tests and/or the customer tests. Not saying we shouldn't make these changes though, it could totally be worth it. https://github.com/flutter/flutter/blob/master/docs/contributing/Tree-hygiene.md#handling-breaking-changes

The test failure is caused by Material being changed from a stateful to a stateless widget.

It looks like it would be very easy to migrate the packages repo to fix these breakages, something like this?

-       final StatefulElement srcMaterialElement = tester.firstElement(
+       final StatelessElement srcMaterialElement = tester.firstElement(
        find.ancestor(
          of: find.text('Closed'),
          matching: find.byType(Material),
        ),
      );

https://github.com/flutter/packages/blob/937038cd033d2c468f871a338ec893343a09ca04/packages/animations/test/open_container_test.dart#L40-L45

The other breaking change I see is the removal of debugCheckHasMaterial, which I commented on.

Also @nate-thegrate have you considered using deprecation to mitigate these breaking changes? For example, if we do decide to remove/replace debugCheckHasMaterial, the typical way would be to deprecate it and remove it later.

We could also do something similar for the Stateful=>Stateless breaking change, but it might not be worth it for that one.

Edit: By the way I forgot to say, thank you for the PR! I appreciate you taking a look at the hard problems like this, this is the kind of introspection that Flutter needs to stay great.

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

yakagami · 2024-06-19T14:10:38Z

Is the discussion about using WidgetState in the Widgets library relevant here?

From #142151:

As far as I'm aware, there is no plan to use WidgetState in a major way across the base Widgets. If there is a concern about increasing complexity by using WidgetState in them, we can either just be careful about where we use them, or we can try and find a way to make them not required to use.

There is currently no plan to use WidgetState anywhere in the base widgets, especially ones that already exist. Ideally, we'd come up with a better way to design the API for WidgetState before that happens.

These are in response to this point:

The MaterialStateProperty API is rather awkward to use from a developer's perspective. I think it's fine for Material (and probably Cupertino) but I wouldn't want us to start using this at the widgets level (e.g. in TextField), because that would force other UI libraries built on Flutter to use it whenever they wanted to interact with the widgets layer, and different UI libraries have different ideas of what that should look like (e.g. have different groups of state properties, different mechanisms for animating them, etc). This would complicate any package that's trying to provide an alternative API to the material API

nate-thegrate · 2024-06-19T22:53:05Z

@yakagami thanks for bringing this up!

The InkGestureDetector class has a couple of spots for WidgetStateProperty objects, but they're optional:

The mouseCursor can be set using either a MouseCursor or a WidgetStateMouseCursor object.
The ink colors can be set either with a WidgetStateProperty<Color> overlayColor, or just by passing Colors to focusColor, hoverColor, and splashColor.

I agree with the concern you mentioned—as it is currently, the WidgetStateProperty API feels a bit too clunky. It has a learning curve, and even when you're used to it, it's not super pleasant to look at:

InkGestureDetector(
  overlayColor: WidgetStateProperty.resolveWith((states) {
    if (states.contains(WidgetState.focused) || states.contains(WidgetState.hovered)) {
      return Colors.blue;
    }
    if (states.contains(WidgetState.pressed)) {
      return Colors.black;
    }
    return null;
  }),
)

~~I have an open pull request that introduces an easier syntax:~~ Edit: it's merged!

InkGestureDetector(
  overlayColor: WidgetStateProperty<Color?>.fromMap({
    WidgetState.focused | WidgetState.hovered: Colors.blue,
    WidgetState.pressed: Colors.black,
  }),
)

nate-thegrate · 2024-08-08T21:36:00Z

Quick status update…

I pushed 1 more commit here (more details in a comment above).
The design doc now links to a demo PR that shows a straightforward way to improve the efficiency of Scaffold and other widgets.
I made another draft PR that shows how we might create a design-agnostic InkResponse.
I recently noticed another breakage and opened yet another PR to fix it.
(idk why an aforementioned test failure seems to have disappeared, but I'm not complaining!)

Let me know what you think! :)

Preparing for: - flutter/flutter#149963

nate-thegrate · 2024-09-27T23:41:32Z

Earlier this week, I had hopes of splitting this big PR into a couple of smaller PRs, by holding off on migrating to the widgets library.

Based on master...nate-thegrate:flutter:SplashFactory-no-migration, unfortunately it'd be splitting 1 big PR into 2 big PRs :(
(still over 900 lines added)

Piinks · 2024-10-01T19:06:00Z

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

+  'This feature was deprecated after v3.24.0-0.2.pre.',
+)
+/// An interface for creating ink effects on a [Material].
+typedef MaterialInkController = SplashController;


Rather than deprecating and typedef-ing this, can it implement the interface and add the addInkFeature? That way it would not break anyone, and we would not need to add that method as a deprecation in the new class.

I personally am not worried about breaking anyone here, since data-driven fixes are easy and since interacting with a MaterialInkController directly is highly unusual.

(As far as implementing the interface, see comment below)

packages/flutter/lib/src/widgets/splash.dart

Piinks · 2024-10-01T19:08:08Z

packages/flutter/lib/src/widgets/splash.dart

+  @Deprecated(
+    'Use addSplash instead. '
+    '"Splash effects" no longer rely on a MaterialInkController. '
+    'This feature was deprecated after v3.24.0-0.2.pre.',
+  )
+  void addInkFeature(Splash feature);


Since this is a new class, why add something just for it to be deprecated? Let's see if we can make MaterialInkController implement this class instead of a typedef, so this method does not need to be here. It will be less breaking too I think.

Unfortunately, MaterialInkController was the interface, not the implementation.
Re-implementing the interface within the Material library would involve a lot of duplicated logic, but I do agree that a newly-added deprecated method is very off-putting.

The style guide mentions using extension methods for cases like this one:

Avoid using extension.

Prefer adding methods directly to relevant classes. If that is not possible, create a method that clearly identifies what object(s) it works with and is part of.

(A rare exception can be made for extensions that provide temporary workarounds when deprecating features. In those cases, however, the extensions and all their members must be deprecated in the PR that adds them, and they must be removed in accordance with our deprecation policy.)

The downside of "extension methods aren't easy to discover" isn't much of a downside when the method is deprecated and we'd rather not have people discovering it anyway :)

Piinks · 2024-10-01T19:09:31Z

packages/flutter/lib/src/widgets/debug.dart

+        ErrorHint(
+          'A SplashController can be provided by an ancestor SplashBox; alternatively, '
+          'there are several viable options from the Material libary, including '
+          'Material, Card, Dialog, Drawer, and Scaffold.',
+        ),


If someone is not using the material library, I am not sure if this hint is very helpful.

Fair point, though my guess would be that the majority of the time, the person reading this message is someone using the material library.

Then why are we moving this to the widgets library? :)

Once this migration takes place, I imagine there will be Cupertino widgets that build a SplashBox; once that happens, I think it'd be good to add them to this message.

Would we further alter the error message to account for every widget and library possible? The point I was trying to make was that error messages should be as specific as they can be. Listing widgets that may or may not have to do with the error is not helpful to the user.

+1 on this being a little too specific. You can say something like, "Many existing widgets in Flutter already contain a SplashBox. Alternatively, you can add one yourself."

We maybe should have never had a list of widgets in the original error hint for the material debug check. If something like that changes in the future, I don't trust myself to remember to check the error hints to be correct.

Piinks · 2024-10-01T19:10:30Z

packages/flutter/lib/src/widgets/splash.dart

+    required Offset position,
+    required Color color,
+    required TextDirection textDirection,
+    bool containedInkWell = false,


This also looks like leakage from Material

…well shoot, it looks like deprecating & replacing an abstract method's parameter isn't possible. 😓

I believe the solution is to use a union type here. An abstract parent type would probably be ideal for facilitating the migration—it's not quite as fun as a sealed class, but not being restricted to a single library means we won't have to bring a bunch of material stuff into the widgets layer!

This would also let us get rid of the useless constructor, so we'll have a clean class with 1 method :)

abstract interface class SplashFactory { @factory Splash create( ... ); }

Thanks for catching this—it very well could have gone unnoticed.

Piinks · 2024-10-01T19:11:59Z

packages/flutter/lib/src/widgets/splash.dart

+      child: _InkFeatures(
+        key: _inkFeatureRenderer,


We should rename these, ink is very specifically material.

It's unfortunate that you weren't able to make it to the dash forum where we talked about this process, but I've unresolved an above thread that has the current plan:

Get a LGTM

Rename _RenderInkFeatures and migrate a whole bunch of tests

Merge

It is a private class being added anew here in the widget layer. We should be able to call it what we intend to call it here. I found no references to _InkFeatures in any tests, which tests are affected by that?

Apologies, it was _RenderInkFeatures that was causing the issues. Will edit previous comment.

Piinks · 2024-10-01T19:12:16Z

packages/flutter/lib/src/widgets/splash.dart

+class _RenderInkFeatures extends RenderProxyBox implements SplashController {
+  _RenderInkFeatures({this.color, required this.vsync}) : super(null);
+
+  /// Enables [InkFeature] animations.


Same here re: ink references

Piinks · 2024-10-01T19:12:23Z

packages/flutter/lib/src/widgets/splash.dart

+    'Splash effects are no longer exclusive to Material design. '
+    'This feature was deprecated after v3.24.0-0.2.pre.',
+  )
+  List<Splash> get debugInkFeatures => splashes;


This one should also be addressed once tests are updated.

packages/flutter/lib/src/widgets/splash.dart

nate-thegrate · 2024-10-04T18:10:20Z

@Piinks and @MitchellGoodwin thank you both very much for the feedback; it feels great to be making progress on this PR after it had been on hold for a while.

I'm getting ready to embark on a weekend camping trip; once I'm back I will make updates based on advice given above!

flutter-dashboard · 2024-10-04T18:16:38Z

This pull request has been changed to a draft. The currently pending flutter-gold status will not be able to resolve until a new commit is pushed or the change is marked ready for review again.

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.

nate-thegrate · 2024-10-07T17:18:51Z

Based on the advice given recently, I think maybe the best option is to close this PR and try a different approach:

Material.of(context) in the test framework #156341

Rather than planning to dump a bunch of test changes here, we could migrate the tests first in their own PR, and then use this pull request as a template for a much cleaner transition :)

github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. labels Jun 8, 2024

nate-thegrate marked this pull request as ready for review June 8, 2024 21:53

This comment was marked as resolved.

Sign in to view

Piinks requested review from justinmc, dkwingsmt and MitchellGoodwin June 12, 2024 17:37

This comment was marked as resolved.

Sign in to view

nate-thegrate marked this pull request as draft June 17, 2024 05:30

nate-thegrate force-pushed the blank-material branch from d2ca6ea to 0cb4025 Compare June 18, 2024 05:20

nate-thegrate marked this pull request as ready for review June 18, 2024 05:21

nate-thegrate changed the title ~~Add BlankMaterial widget~~ Ink effects in widgets library Jun 18, 2024

nate-thegrate force-pushed the blank-material branch from 9b6bede to 2795cf3 Compare June 18, 2024 06:49

This comment was marked as resolved.

Sign in to view

justinmc reviewed Jun 18, 2024

View reviewed changes

packages/flutter/lib/src/material/debug.dart Show resolved Hide resolved

nate-thegrate mentioned this pull request Jun 19, 2024

Fix breakages introduced by SplashFactory flutter/packages#6952

Merged

nate-thegrate force-pushed the blank-material branch 2 times, most recently from 35c5574 to af3f426 Compare June 20, 2024 03:00

This comment was marked as outdated.

Sign in to view

This comment was marked as resolved.

Sign in to view

nate-thegrate force-pushed the blank-material branch from c16743f to 342cdb6 Compare July 30, 2024 16:11

github-actions bot added the c: tech-debt Technical debt, code quality, testing, etc. label Jul 30, 2024

nate-thegrate force-pushed the blank-material branch from 342cdb6 to 80481bd Compare August 8, 2024 19:37

nate-thegrate mentioned this pull request Aug 8, 2024

Fix SplashBox breakages flutter/devtools#8176

Closed

nate-thegrate mentioned this pull request Aug 8, 2024

TouchResponse nate-thegrate/flutter#9

Draft

nate-thegrate requested review from dkwingsmt, Piinks and MitchellGoodwin August 8, 2024 22:03

auto-submit bot pushed a commit to flutter/packages that referenced this pull request Aug 13, 2024

Fix breakages introduced by SplashFactory (#6952)

66e98d8

Preparing for: - flutter/flutter#149963

This comment was marked as outdated.

Sign in to view

This was referenced Sep 4, 2024

Equivalent of InkWell in Cupertino style #48017

Open

"Fix failing checks" wiki page for new contributors #154629

Open

Piinks reviewed Oct 1, 2024

View reviewed changes

nate-thegrate force-pushed the blank-material branch 2 times, most recently from 1a9e3d5 to dde079e Compare October 3, 2024 02:27

nate-thegrate marked this pull request as draft October 4, 2024 18:11

nate-thegrate force-pushed the blank-material branch from 2548df3 to 78f166c Compare October 7, 2024 15:59

nate-thegrate added 4 commits October 7, 2024 11:01

SplashFactory in widgets library

7370f49

revert some doc changes

c535751

Apply feedback from Kate's review

b96161d

documentation tweaks

f4612e7

nate-thegrate force-pushed the blank-material branch from 78f166c to f4612e7 Compare October 7, 2024 17:01

nate-thegrate closed this Oct 7, 2024

justinmc mentioned this pull request Oct 7, 2024

Material.of(context) in the test framework #156341

Open

nate-thegrate mentioned this pull request Oct 14, 2024

Notable Mishaps #156720

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`SplashFactory` in the widgets library #149963

`SplashFactory` in the widgets library #149963

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as outdated.

This comment was marked as resolved.

This comment was marked as outdated.

SplashFactory in the widgets library #149963

SplashFactory in the widgets library #149963

Conversation

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

Choose a reason for hiding this comment

This comment was marked as outdated.

This comment was marked as resolved.

This comment was marked as outdated.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Avoid using extension.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

`SplashFactory` in the widgets library #149963

`SplashFactory` in the widgets library #149963