Refactor FAB (Remove `RawMaterialButton`) #99753

TahaTesser · 2022-03-08T11:31:36Z

Description

This PR replaced RawMaterialButton with ElevatedButton (as it is obsolete and soon to be deprecated)

There are golden changes due to highlight color not being translated to ElevatedButton (ButtonStyleButton sets highlight color to be transparent).

Highlight color golden changes preview (when FAB Is pressed)

M2 FAB (before/after)	M3 FAB (before/after)

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.

skia-gold · 2022-03-08T11:38:05Z

Gold has detected about 4 new digest(s) on patchset 1.
View them at https://flutter-gold.skia.org/cl/github/99753

skia-gold · 2022-03-08T13:33:05Z

Gold has detected about 6 new digest(s) on patchset 2.
View them at https://flutter-gold.skia.org/cl/github/99753

flutter-dashboard · 2022-03-08T13:54:03Z

Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change).

If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review.

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.

Changes reported for pull request #99753 at sha 016dc101e7edb794639ac3818896b2951f6e9866

Piinks

Wow! Thank you for doing this. It looks like the golden file images show the color is slightly different, but I don't know if that is expected in the new design.

I am deferring to @HansMuller on the general design since he wrote the new Button/ButtonStyle universe. If this looks ok in keeping with the new paradigm I can give it a more thorough review.

I am wondering first if we are going to maintain all of the numerated properties on FAB, or deprecate and redirect them to be fashioned in a ButtonStyle. I think either approach has pros and cons, and this current one seems clean and minimally disruptive.
FAB already has its own FABTheme, so all of that churn would extend there as well. That being said I like this approach

HansMuller

I agree with Kate that just changing FAB's implementation, without adopting the ButtonStyle API here and in the FAB theme, is the prudent change to make. I think that applies to ToggleButtons as well, per Kate's feedback.

Supporting a ButtonStyle style parameter for FAB and ToggleButtons is a separate decision. If we choose to do that; it would be separate PR as well as a plan for migration, etc.

HansMuller · 2022-03-10T23:26:45Z

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

Maybe leave mixing in Diagnosticable for a separate PR that does so for ButtonStyleButton and then adds whatever debugFillProperties overrides are needed for the other ButtonStyleButtons.

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

skia-gold · 2022-03-11T11:18:05Z

Gold has detected about 6 new digest(s) on patchset 3.
View them at https://flutter-gold.skia.org/cl/github/99753

Piinks

Hey @TahaTesser what is the state of this PR? It looks like there are some failing tests.
I am concerned the subtle color change will cause a lot of internal tests to fail.

TahaTesser · 2022-03-22T22:17:10Z

Hey @TahaTesser what is the state of this PR? It looks like there are some failing tests.

Hey @Piinks
We need to land #99975 in order to fix the tests.
These tests are were passing when I added Diagnosticable changes in this PR, which I reverted to be filed in a separate PR based on #99753 (comment).

I am concerned the subtle color change will cause a lot of internal tests to fail.

These golden changes are due to ButtonStyleButton setting Colors.transparent to highlight color when the button is pressed.
(Highlight color was provided in RawMaterialButton)

#99493 also has the same changes related highlight color.
cc @HansMuller could shed some light if this is okay or not.

Piinks · 2022-03-22T22:27:21Z

Awesome! Thanks for the update, I was just checking that I was not neglecting this if it needed another review. :)

flutter-dashboard · 2022-04-11T22:31:37Z

This pull request executed golden file tests, but it has not been updated in a while (20+ days). Test results from Gold expire after as many days, so this pull request will need to be updated with a fresh commit in order to get results from Gold.

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.

flutter-dashboard · 2022-05-09T17:50:34Z

Golden file changes are available for triage from new commit, Click here to view.

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.

Changes reported for pull request #99753 at sha b5bd45514c754c0dc5ad5f6caa2272f8987cb9e0

TahaTesser · 2022-05-09T18:13:15Z

@Piinks
#99976 is no longer blocking this. Actually the problem was MaterialStatePropertyoverlay in the ElevatedButton, this is now fixed and I also updated examples/api tests as they used to expect RawMaterialButton (added these examples tests a couple of weeks ago) now expect ElevatedButton and tests are passing.

This is ready to review

Piinks

This looks great! Just some style nits. I am checking with customer tests.

packages/flutter/test/material/floating_action_button_theme_test.dart

examples/api/test/material/floating_action_button/floating_action_button.0_test.dart

packages/flutter/test/material/floating_action_button_theme_test.dart

Piinks · 2022-05-13T21:49:44Z

There are indeed some customers to migrate, working on that now. I'll update to land once complete. :)

Piinks · 2022-05-18T23:15:29Z

Update: still working with customers. There are some changes here that cause the color of the button to fail contrast guidelines when pressed. That may be related to the highlight color change you mentioned above, still tracking it down.

Piinks · 2022-06-07T22:54:28Z

Not forgotten. :) Just back from time off, following up on customer migrations. I'll report back tomorrow.

Piinks · 2022-06-08T21:41:38Z

@TahaTesser can you update this branch with the latest from master? I am having trouble using our tools to sync this with the internal copy for testing. Thanks!

Piinks · 2022-06-09T19:46:51Z

Thanks for updating! This is still going to take a minute to fix all of the affected customers, we're making progress though!

TahaTesser · 2022-08-03T12:27:16Z

@Piinks
Fixed conflicts and rebased this too.

Piinks · 2022-08-09T22:42:35Z

Thanks for the update! This is now at the front of my customer migration list.

Piinks · 2022-08-16T23:22:02Z

Welcome back from vacation again. :) This is under active migration with customers.

TahaTesser · 2022-08-18T13:20:10Z

@Piinks
Thank you so much. Looking forward to merging this :)

Piinks · 2022-08-18T21:54:50Z

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

Suggested change

shape: MaterialStateProperty.all<OutlinedBorder?>(shape as OutlinedBorder),

shape: MaterialStateProperty.all<OutlinedBorder?>(shape as OutlinedBorder),

This is causing issues for customers that have

FloatingActionButton( shape: Border(), ... )

Is there a way that this can still be supported? Or is it a breaking change?

It can still be supported by wrapping ElevatedButton with DecoratedBox BUT this can be done by the customer too, floatingActionButton: takes any widget so FAB can be wrapped in a DecoratedBox.

The reason when Border() provide to the shape parameter currently works is because material.dart checks if "shape" is a ShapeBorder otherwise based on material type, directly draw the border with custom paint hence there is no issue with this in the current design.

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

Lines 908 to 916 in 75ee440

class _ShapeBorderPainter extends CustomPainter {

_ShapeBorderPainter(this.border, this.textDirection);

final ShapeBorder border;

final TextDirection? textDirection;

@override

void paint(Canvas canvas, Size size) {

border.paint(canvas, Offset.zero & size, textDirection: textDirection);

}

but there is no existing test where the border is provided to shape in the test class and looks a little bit off to me when the border is provided to shape parameter.

If you still want this, I can wrap ElevatedButton with DecoratedBox along with a couple of tests to make sure this behavior works as expected.

If you still want this, I can wrap ElevatedButton with DecoratedBox along with a couple of tests to make sure this behavior works as expected.

There are a surprising number of cases with Border, if we can still support it, it would be definitely be easier. Do you want to try it and see what it would look like?

I just added the code.
decoration: ShapeDecoration(shape: shape), is responsible for rendering the border and we're defaulting to a rectangular shape to match current behavior when a border is provided.

shape: MaterialStateProperty.all<OutlinedBorder?>(shape is! OutlinedBorder ? const RoundedRectangleBorder() : shape as OutlinedBorder?, ),

Piinks · 2022-09-14T20:41:38Z

Still catching up from having covid, thanks for your patience. For this, there is one more customer test file that I need to migrate, and then this should be ready.

TahaTesser · 2022-10-05T13:19:37Z

@Piinks
Friendly ping.

TahaTesser · 2023-01-09T11:29:04Z

Closing this PR for now. Will come to this with a clean PR in the future.

flutter-dashboard bot added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels Mar 8, 2022

TahaTesser requested review from HansMuller and Piinks March 8, 2022 11:32

flutter-dashboard bot added the will affect goldens Changes to golden files label Mar 8, 2022

Piinks reviewed Mar 10, 2022

View reviewed changes

HansMuller reviewed Mar 11, 2022

View reviewed changes

TahaTesser mentioned this pull request Mar 11, 2022

Provide Diagnosticable mixin to ButtonStyleButton buttons #99975

Closed

Piinks reviewed Mar 22, 2022

View reviewed changes

TahaTesser mentioned this pull request Mar 23, 2022

Add Diagnosticable mixin to ButtonStyleButton buttons #99976

Closed

8 tasks

TahaTesser force-pushed the refactor_fab branch 2 times, most recently from 14e0af1 to b5bd455 Compare May 9, 2022 17:15

TahaTesser requested a review from Piinks May 9, 2022 18:04

Piinks reviewed May 12, 2022

View reviewed changes

TahaTesser force-pushed the refactor_fab branch 2 times, most recently from a6ff21f to abf17f9 Compare June 9, 2022 12:15

Piinks mentioned this pull request Jun 9, 2022

Deprecate toggleableActiveColor #97972

Merged

8 tasks

TahaTesser force-pushed the refactor_fab branch from abf17f9 to b17e29a Compare August 3, 2022 11:52

TahaTesser requested a review from Piinks August 3, 2022 12:28

Piinks reviewed Aug 18, 2022

View reviewed changes

Refactor FAB (Remove RawMaterialButton)

c0a3764

TahaTesser force-pushed the refactor_fab branch from b17e29a to c0a3764 Compare August 22, 2022 14:25

Add decorated box

37001c7

TahaTesser requested a review from Piinks August 22, 2022 14:56

TahaTesser closed this Jan 9, 2023

TahaTesser deleted the refactor_fab branch January 9, 2023 11:29

gspencergoog mentioned this pull request Jan 25, 2023

Remove the dependency on RawMaterialButton from the FAB #119176

Closed

	shape: MaterialStateProperty.all<OutlinedBorder?>(shape as OutlinedBorder),
	shape: MaterialStateProperty.all<OutlinedBorder?>(shape as OutlinedBorder),

	class _ShapeBorderPainter extends CustomPainter {
	_ShapeBorderPainter(this.border, this.textDirection);
	final ShapeBorder border;
	final TextDirection? textDirection;

	@override
	void paint(Canvas canvas, Size size) {
	border.paint(canvas, Offset.zero & size, textDirection: textDirection);
	}

Refactor FAB (Remove RawMaterialButton) #99753

Refactor FAB (Remove RawMaterialButton) #99753

Uh oh!

Conversation

TahaTesser commented Mar 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Highlight color golden changes preview (when FAB Is pressed)

Pre-launch Checklist

Uh oh!

skia-gold commented Mar 8, 2022

Uh oh!

skia-gold commented Mar 8, 2022

Uh oh!

flutter-dashboard bot commented Mar 8, 2022

Uh oh!

Piinks left a comment

Choose a reason for hiding this comment

Uh oh!

HansMuller left a comment

Choose a reason for hiding this comment

Uh oh!

HansMuller Mar 10, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

skia-gold commented Mar 11, 2022

Uh oh!

Piinks left a comment

Choose a reason for hiding this comment

Uh oh!

TahaTesser commented Mar 22, 2022

Uh oh!

Piinks commented Mar 22, 2022

Uh oh!

flutter-dashboard bot commented Apr 11, 2022

Uh oh!

flutter-dashboard bot commented May 9, 2022

Uh oh!

TahaTesser commented May 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Piinks left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Piinks commented May 13, 2022

Uh oh!

Piinks commented May 18, 2022

Uh oh!

Piinks commented Jun 7, 2022

Uh oh!

Piinks commented Jun 8, 2022

Uh oh!

Piinks commented Jun 9, 2022

Uh oh!

TahaTesser commented Aug 3, 2022

Uh oh!

Piinks commented Aug 9, 2022

Uh oh!

Piinks commented Aug 16, 2022

Uh oh!

TahaTesser commented Aug 18, 2022

Uh oh!

Piinks Aug 18, 2022

Choose a reason for hiding this comment

Uh oh!

TahaTesser Aug 19, 2022

Choose a reason for hiding this comment

Uh oh!

Piinks Aug 19, 2022

Choose a reason for hiding this comment

Uh oh!

TahaTesser Aug 22, 2022

Choose a reason for hiding this comment

Uh oh!

Refactor FAB (Remove `RawMaterialButton`) #99753

Refactor FAB (Remove `RawMaterialButton`) #99753

TahaTesser commented Mar 8, 2022 •

edited

Loading

TahaTesser commented May 9, 2022 •

edited

Loading