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

Widget state properties #142151

Merged
merged 38 commits into from Mar 19, 2024

Conversation

MitchellGoodwin
Copy link
Contributor

@MitchellGoodwin MitchellGoodwin commented Jan 24, 2024

Fixes #138270.

Moves the majority of the logic of MaterialStateProperties down to the widgets layer, then has the existing Material classes pull from the widgets versions.

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I signed the CLA.
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is test-exempt.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@github-actions github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. f: scrolling Viewports, list views, slivers, etc. labels Jan 24, 2024
@MitchellGoodwin MitchellGoodwin marked this pull request as ready for review January 24, 2024 21:27
@MitchellGoodwin MitchellGoodwin marked this pull request as draft January 24, 2024 23:15
@MitchellGoodwin MitchellGoodwin marked this pull request as ready for review January 25, 2024 22:48

/// Convenience class for creating a [MaterialStateProperty] that
/// resolves to the given value for all states.
class MaterialStatePropertyAll<T> implements MaterialStateProperty<T> {
class MaterialStatePropertyAll<T> extends WidgetStatePropertyAll<T>{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wondering why this is the only one that isn't a typedef?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's because of the toString message saying specifically "MaterialStatePropertyAll". I was unsure if it was important if that backwards compatibility was preserved, so I left it as it's own thing.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do any customer_tests or anything in google3 fail if the toString message is changed? If nothing fails, it wouldn't be considered a breaking change under our policy and we should simplify this.

/// See https://material.io/design/interaction/states.html#usage.
error,
}
typedef MaterialState = WidgetState;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably deprecate the Material-only ones since it is going to be confusing to have the same concept under two different names.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should also add a flutter fix to make it easier for people to migrate to the widget version.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that'd be cleaner. The only issue is if Material deviates from what could be considered generic, non-specific we would need to add back in MaterialStates after deprecating it.

Which I don't feel like is too likely? So maybe it would be better to pull the trigger and deprecate the one's that can't be typedefs.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am leaning towards deprecating the Material* ones to avoid the confusion of having two names for the same concept. If Material evolves, I think we'd want to evolve the underlying Widget* ones to be able to handle those cases as well. But lets also get the input from our material and deprecation experts (@HansMuller @Piinks) on this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the goal was to allow material to have its own specific states, while the widget base concept was kept more generic. So, MaterialState would inherit all the basic states from WidgetState, and then have some of its own material-specific ones.

However, I am not sure how this is possible with this current proposal. How would we add a material-specific state to MaterialState with this change?

Maybe we could consider scrolledUnder as material-specific and remove it from the widgets states to put it here in the material states as a proof of concept.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure how possible it is to take the values from one enum to create an extended version while making it easy to have them be somewhat interchangeable, without turning both into a class that emulates enums. It looks like the Dart team is against the idea: dart-lang/language#2733.

I think in the long run, even if there are Material specific values, it would be helpful to still put them in the Widgets layer. There would likely be use cases for all of them in Cupertino by itself, but it would also make adaptive work smoother. The Material specific logic can be in functions like MaterialStateOutlineInputBorder that would have apply Material styling on top of the shared WidgetState enum.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From a custom design system pov and taking future proofing into account would be nice if the WidgetState would be extensible. But what is already in the enum is probably more than enough for the majority of the design systems out there.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think in the long run, even if there are Material specific values, it would be helpful to still put them in the Widgets layer

This already happened here and I think it's this is a concerning outcome. We now have material specific concepts in a package that was previously agnostic to widget sets and design systems.

/// See also:
///
/// * [WidgetPropertyResolver], the non-Material form of `MaterialPropertyResolver`
/// that is inherited by `MaterialPropertyResolver.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

inherited? I think this should probably say what you said on the other one that they can just be used interchangable?

/// See also:
///
/// * [WidgetStateMixin], the generic version of `MaterialStatesController`
/// that can be used with non-Material widgets.
@optionalTypeArgs
mixin MaterialStateMixin<T extends StatefulWidget> on State<T> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be done with a typedef?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as elsewhere, I was worried about the debugFillProperties returning differently.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as my comment above.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Converting this mixin into a typedef seems like a pretty big breaking change. It's methods (setMaterialState, addMaterialState, `removeMaterialState, etc.) throw errors if this is just converted into a typedef. I'm marking this as deprecated and seeing what can be helped along with a flutter fix.

/// See https://material.io/design/interaction/states.html#usage.
error,
}
typedef MaterialState = WidgetState;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the goal was to allow material to have its own specific states, while the widget base concept was kept more generic. So, MaterialState would inherit all the basic states from WidgetState, and then have some of its own material-specific ones.

However, I am not sure how this is possible with this current proposal. How would we add a material-specific state to MaterialState with this change?

Maybe we could consider scrolledUnder as material-specific and remove it from the widgets states to put it here in the material states as a proof of concept.

@@ -102,7 +102,7 @@ void main() {
expect(description[8], 'showSelectedLabels: true');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the tests in the material directory, should they use MaterialState instead of WidgetState since the material one should be a subclass of the widget one?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now, MaterialStateMouseCursor is a typedef, which because the widget version has an overridden get debugDescription, it is returning "WidgetStateMouseCursor" for both. Which means it should probably be a subclass instead.

@HansMuller
Copy link
Contributor

If we make the MaterialStates a distinct type from WidgetStates developers will not be able use them interchangeably. That might be counterproductive and if we don't have a important use case to motivate the distinction it might be easier to just have one definition of the set of valid component states.

@MitchellGoodwin
Copy link
Contributor Author

Converting to a draft. Will make a branch off of this one to test flutter fixes.

@github-actions github-actions bot added a: text input Entering text in a text field or keyboard related problems c: tech-debt Technical debt, code quality, testing, etc. labels Feb 6, 2024
@MitchellGoodwin MitchellGoodwin mentioned this pull request Feb 6, 2024
8 tasks
@github-actions github-actions bot added d: api docs Issues with https://api.flutter.dev/ d: examples Sample code and demos f: focus Focus traversal, gaining or losing focus and removed d: api docs Issues with https://api.flutter.dev/ d: examples Sample code and demos f: focus Focus traversal, gaining or losing focus labels Feb 9, 2024
@MitchellGoodwin MitchellGoodwin added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 19, 2024
@auto-submit auto-submit bot merged commit 6190c5e into flutter:master Mar 19, 2024
67 checks passed
@knopp
Copy link
Member

knopp commented Mar 20, 2024

This now ties widgets package to a specific design system. Some design system have tri-state checkboxes for example and the WidgetState enum is not generic enough to represent. It even links to material documentation. This feels like a mistake.

@rydmike
Copy link
Contributor

rydmike commented Mar 20, 2024

@Kypsis, I just found this discussion when I saw the merge. Thank you! I could not agree more on everything you said and concluded in your two posts.


About flex_color_scheme that you mention too, it was basically born because I did not want to deal with the complexities and verbosity of theming Material widgets every time. Well, theming them to the extent that Material widgets can be themed, which like you say is very inconsistent and often limited. Loved your examples of that! 😄

  • FlexColorScheme started as just a simpler factory to make reasonable and nice looking ThemeData and ColorScheme, and from it use custom color mappings to component themes. Quick and easy customization of border radius globally and per component is also a big part of it. These simple things, that many devs seem to struggle with, can get many designs "close enough", sure not big custom design requirements like yours, since it is still 100% limited by what ThemeData can do. Still close enough for "most" devs design needs and use cases. I guess that contributed to its popularity. Then later FCS also got a visual config tool Themes Playground, so I and its other users don't have to remember FCS own APIs. Which over time has become a bit, well, let's say it has its own none breaking legacies too 😄

  • FCS also later got features for adaptive themes, and custom seeded color scheme configs. So if you like, you can keep default M3 styles, radius and color tinted surface results, typically on Android, but get another more agnostic style on other platforms, often by removing tints, using a different radius, etc. Sure those pesky elevation tints (that you can remove with FCS on all or desired platforms) are going away now anyway, in favor fixed surface colors with various levels primary chroma mixed into them. End result will still be that users will want an adaptive response where those color mappings with their primary color chroma baked in, are not so aggressive (M3 opinionated) on other platforms.

I have also as started thinking about a next gen tool for styling and theming Flutter apps. The Figma integrations you mention is something I'm also considering. I have looked at Mix too, it does a lot of things right.

Still I digress (as usual), I just wanted to say I know very well where you are coming from, and that I also think that a more holistic approach to component styling and building is desperately needed in Flutter.


Going back to other feedback about WidgetState, e.g. just now by @knopp, I also suspect this is not generic enough and it feels a bit like a quick band-aid fix, sure it will help now, but it may come back to haunt us with ghost pains later. I think @Hixie captured many of those concerns as well, in all the earlier discussions above.

Another thing that was mentioned in comments above was that using MaterialState is seen as very challenging and complicated, thus WidgetState is too. It is is even seen as a hurdle by many devs. Not sure what could be done about it, but I do agree that it is "a bit" complicated and does not look very pretty when used. The order in which you resolve the states also impact the end result, which may add to the usage confusion.

Another interesting thing is that assigning a custom MaterialState in any ThemeData component theme property breaks the equality check of ThemeData. Depending on how you use ThemeData, this can cause extra rebuilds of MaterialApp. This is demonstrated here: #89127 (comment)

@knopp
Copy link
Member

knopp commented Mar 20, 2024

I've read the conversation above I am a bit confused. @Hixie has raised some good points but then, after a meeting (?) the PR got approved without any of those points being addressed.

For reference, this is state of a button in our design system:

enum SelectionState {
  on,
  off,
  mixed,
}

class ButtonState {
  ButtonState({
    this.selected = SelectionState.off,
    this.enabled = false,
    this.focused = false,
    this.hovered = false,
    this.pressed = false,
    this.tracked = false,
  });

  /// Determines selection state of the control.
  final SelectionState selected;

  /// Control is enabled and can be interacted with.
  final bool enabled;

  /// Control has keyboard focus.
  final bool focused;

  /// Control is being hovered over by a pointer.
  final bool hovered;

  /// Control is being pressed. When pointer is released without leaving the
  /// control, the control will receive a tap event.
  final bool pressed;

  /// Control is receiving pointer events but the pointer may or may not be
  /// hovering over the control.
  final bool tracked;

  ...
}

It's pretty simple as far as states go, but even then it cannot be represented by WidgetState because of indeterminate selection state. If a standard desktop checkbox can't be represented by WidgetState does it really belong to the base widgets package?

On top of that WidgetState has scrolledUnder, which was clearly added there with material in mind.

Btw, having simple class with bunch of properties is much nicer to use than a set of enums. But that's not the main point.

I'm not against having widget state represented in widgets package, but it shouldn't be a carbon copy of MaterialStateProperties.

@rydmike
Copy link
Contributor

rydmike commented Mar 20, 2024

I've read the conversation above I am a bit confused. @Hixie has raised some good points but then, after a meeting (?) the PR got approved without any of those points being addressed.

There was/is also the Widget State Properties document, but it seems like it was abandoned without much further discussion or elaboration.

@ariba1039

This comment was marked as spam.

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 20, 2024
auto-submit bot pushed a commit to flutter/packages that referenced this pull request Mar 20, 2024
flutter/flutter@d31a85b...b96c13d

2024-03-20 engine-flutter-autoroll@skia.org Roll Flutter Engine from f42fc937028c to 883adfe2ef61 (1 revision) (flutter/flutter#145459)
2024-03-20 engine-flutter-autoroll@skia.org Roll Flutter Engine from 32ae508316a6 to f42fc937028c (1 revision) (flutter/flutter#145456)
2024-03-20 engine-flutter-autoroll@skia.org Roll Flutter Engine from 1e89d04c6b52 to 32ae508316a6 (2 revisions) (flutter/flutter#145449)
2024-03-20 engine-flutter-autoroll@skia.org Roll Flutter Engine from e96b2b0047f7 to 1e89d04c6b52 (1 revision) (flutter/flutter#145446)
2024-03-20 engine-flutter-autoroll@skia.org Roll Flutter Engine from 24b84203f6cc to e96b2b0047f7 (1 revision) (flutter/flutter#145444)
2024-03-20 engine-flutter-autoroll@skia.org Roll Flutter Engine from 3f67bc531eef to 24b84203f6cc (1 revision) (flutter/flutter#145441)
2024-03-20 engine-flutter-autoroll@skia.org Roll Flutter Engine from 00bfdb580b1b to 3f67bc531eef (1 revision) (flutter/flutter#145438)
2024-03-20 engine-flutter-autoroll@skia.org Roll Flutter Engine from dce639a4c453 to 00bfdb580b1b (1 revision) (flutter/flutter#145435)
2024-03-19 engine-flutter-autoroll@skia.org Roll Flutter Engine from bd3a9241c2c5 to dce639a4c453 (3 revisions) (flutter/flutter#145429)
2024-03-19 engine-flutter-autoroll@skia.org Roll Flutter Engine from bacb027aa824 to bd3a9241c2c5 (1 revision) (flutter/flutter#145427)
2024-03-19 chingjun@google.com Fix remote DDS in proxied devices. (flutter/flutter#145346)
2024-03-19 engine-flutter-autoroll@skia.org Roll Flutter Engine from 7095e6a4a643 to bacb027aa824 (2 revisions) (flutter/flutter#145424)
2024-03-19 engine-flutter-autoroll@skia.org Roll Flutter Engine from 59138d5f8e88 to 7095e6a4a643 (1 revision) (flutter/flutter#145409)
2024-03-19 jhy03261997@gmail.com Add a `minTileHeight` to ListTile widget so its height can be customized to less than the default height. (flutter/flutter#145244)
2024-03-19 58190796+MitchellGoodwin@users.noreply.github.com Widget state properties (flutter/flutter#142151)
2024-03-19 164015983+goodmost@users.noreply.github.com chore: fix some comments (flutter/flutter#145397)
2024-03-19 engine-flutter-autoroll@skia.org Roll Flutter Engine from 7ea6b003ffeb to 59138d5f8e88 (1 revision) (flutter/flutter#145404)
2024-03-19 engine-flutter-autoroll@skia.org Roll Packages from a757073 to a2f4ce0 (3 revisions) (flutter/flutter#145399)
2024-03-19 engine-flutter-autoroll@skia.org Roll Flutter Engine from d8fbcfbd799c to 7ea6b003ffeb (2 revisions) (flutter/flutter#145398)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages
Please CC camillesimon@google.com,rmistry@google.com,stuartmorgan@google.com on the revert to ensure that a human
is aware of the problem.

To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
@benthillerkus
Copy link

I personally like MaterialState (though only in combination with flutter_hooks and ISet from fast_immutable_collections).

Maybe instead of tying the Set to a predefined Enum of possible values, instead something like

class WidgetState {
  final ISet<WidgetStateAspect> state;
}

class WidgetStateAspect {}

class Hovered extends WidgetStateAspect {}
...

could be possible?

That way it'd be easy to extend with design system specific details -- and some metadata or substate could actually be put inside of these instances.

@Kypsis
Copy link

Kypsis commented Mar 20, 2024

@rydmike Massive respect for what you have accomplished with FCS and your tireless work in explaining how to use it on Twitter etc to solve the pain points of working around Flutters Material theming. But there is a corner of my mind that screams at the sheer absurdity that this is even necessary.

@knopp In the absence of meaningful big picture pragmatic solutions from Flutter team on the overall topic, one will accept the small wins regardless of the right or wrong of the implementation details. At least the needle is moving.

Even if we as Flutter developers have gotten accustomed to the "Stockholm syndrome" it does not objectively mean that things are in a good place. I would argue that almost a decade later and Flutter still does not quite grasp the greater UI ecosystem it is part of and how to consume what other parts (mainly design) provide. If you look at the very earliest Flutter Github issues where the discussions mainly flowed between Flutter team members, the general ignorance of the UI problem space and not understanding the greater context paints a hilariously bad picture. Almost like a story of non-UI developers best guesses and thought experiments without evidence of deeper insights based on industry "in the trenches" experience and battle scars. And on the meta level this has persisted to the year 2024 (yes things are getting "better" but only by taking baby steps). Somehow Flutters competitors don't seem to suffer from this particular malady.

So in conclusion "A bird in hand is worth two in the bush".

@goderbauer
Copy link
Member

Thanks for all the feedback about WidgetStateProperties. I would like to point out that they are just a building block that the widget library offers. If they don't work for your design language, you don't have to expose them in your API and you can (continue to) use whatever suits your needs. They have been battle-tested in the material library for years and have solved problems that we are now also facing in the Cupertino library, hence this generalization to use the same mechanism for the same problem.

Specifically about the tristate checkbox: The material Checkbox has offered tristate support for years and the material/widget states AFAIK haven't been an obstacle for that.

@MitchellGoodwin
Copy link
Contributor Author

There are two main goals to moving state properties down to the widgets layer:

  1. Supporting adaptive work. Cupertino has had no concept of MaterialState properties, and no way to get them. Moving state properties down to the widgets layer lets Material pass it's logic to Cupertino directly without having to make best guesses in what a MaterialState.all property should be.

  2. Making state property logic available to package authors that want it, but do not want to use Material. This is from feedback given to the team. If developers want to use tools we already have to build on top of flutter, we don't see a reason to stop them.

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. The negative feedback about MaterialState/WidgetState is something that we do take seriously and we do want to find a more graceful solution to it. This is not the end of improvements to WidgetState.

On MaterialState/WidgetState being Material focused: before starting this I tried to find other design systems ideas of a widgets interactive state to see if there's a reason not to make it agnostic to Material. None of the properties are, at their core, Material specific. I think if they were presented fresh under a different name outside of Flutter, nobody would go "Hey aren't these just Material design?" The weirdest one being scrolledUnder, which definitely was added with Material in mind, but I can think of one obvious place where it could be used in Cupertino, which is with the sliver navbar widget.

@knopp
Copy link
Member

knopp commented Mar 20, 2024

Specifically about the tristate checkbox: The material Checkbox has offered tristate support for years and the material/widget states AFAIK haven't been an obstacle for that.

Right. But that's only because the checkbox MaterialState doesn't distinguish between selected and mixed. Both are "selected". As far as I can tell you can't have a different color for mixed vs selected state for example. (I'll be happy to get corrected if I'm wrong about this).

If they don't work for your design language, you don't have to expose them in your API and you can (continue to) use whatever suits your needs.

I don't think "just don't use" is a fair argument here. With this change the Widgets package now takes over material specific concepts. This has not been the case before and goes in a direction that to me is quite concerning.

@knopp
Copy link
Member

knopp commented Mar 20, 2024

Supporting adaptive work. Cupertino has had no concept of MaterialState properties, and no way to get them. Moving state properties down to the widgets layer lets Material pass it's logic to Cupertino directly without having to make best guesses in what a MaterialState.all property should be.

I guess this is my issue here: In my opinion just because something is useful for both material and Cupertino it does not necessarily mean it should go into widgets layer. Flutter ecosystem goes beyond material and Cupertino. (sidenote - I'd love to see these becoming separate packages, and I know I'm not the only one, but that's for another conversation).

sfshaza2 pushed a commit to flutter/website that referenced this pull request Apr 4, 2024
Adds a migration guide guide for MaterialState logic being marked
deprecated by
[flutter/pull/142151](flutter/flutter#142151)

## Presubmit checklist

- [x] This PR doesn’t contain automatically generated corrections
(Grammarly or similar).
- [x] This PR follows the [Google Developer Documentation Style
Guidelines](https://developers.google.com/style) — for example, it
doesn’t use _i.e._ or _e.g._, and it avoids _I_ and _we_ (first person).
- [x] This PR uses [semantic line
breaks](https://github.com/dart-lang/site-shared/blob/main/doc/writing-for-dart-and-flutter-websites.md#semantic-line-breaks)
of 80 characters or fewer.
atsansone pushed a commit to atsansone/website that referenced this pull request Apr 5, 2024
Adds a migration guide guide for MaterialState logic being marked
deprecated by
[flutter/pull/142151](flutter/flutter#142151)

## Presubmit checklist

- [x] This PR doesn’t contain automatically generated corrections
(Grammarly or similar).
- [x] This PR follows the [Google Developer Documentation Style
Guidelines](https://developers.google.com/style) — for example, it
doesn’t use _i.e._ or _e.g._, and it avoids _I_ and _we_ (first person).
- [x] This PR uses [semantic line
breaks](https://github.com/dart-lang/site-shared/blob/main/doc/writing-for-dart-and-flutter-websites.md#semantic-line-breaks)
of 80 characters or fewer.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a: text input Entering text in a text field or keyboard related problems autosubmit Merge PR when tree becomes green via auto submit App c: tech-debt Technical debt, code quality, testing, etc. d: api docs Issues with https://api.flutter.dev/ d: examples Sample code and demos f: material design flutter/packages/flutter/material repository. f: scrolling Viewports, list views, slivers, etc. framework flutter/packages/flutter repository. See also f: labels. will affect goldens Changes to golden files
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create widgets level support for States, StateProperties
10 participants