Reland update bottom navigation bar test for m3 #137998

bleroux · 2023-11-07T14:46:17Z

This is a reland of #136624 which was reverted because one new M3 golden test failed. The failure was related to the ink sparkle animation.

Ink sparkle is the M3 default animation, it does not play well with golden because it introduces an element of randomness.
One way to avoid this randomness is to use the InkSparkle.constantTurbulenceSeedSplashFactory.

This PR has two commits:

the first one is the original PR (Update BottomNavigationBar tests for M3 #136624).
the second one updates the failing test using InkSparkle.constantTurbulenceSeedSplashFactory.

flutter-dashboard · 2023-11-07T15:20:14Z

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 #137998 at sha c3856aa

bleroux · 2023-11-07T15:30:15Z

@Piinks This is an attempt to reland #136624, I found InkSparkle.constantTurbulenceSeedSplashFactory and wanted to give it a try.
I have not approved the golden changes in case you wanted to have a look.
There are 21 one of them to approve, that looks right.

Unfortunately, you won't be able to push commits to this PR because it belongs to the Nevercode organisation on Github. For this same reason, Taha switched to a personnal fork some months ago, I will switch too as soon as possible as It would be easier for you.

Piinks

Just QQ about web using the sparkle

Piinks · 2023-11-13T21:15:53Z

packages/flutter/test/material/bottom_navigation_bar_test.dart

+    // Regression test for: https://github.com/flutter/flutter/issues/22226
+    Widget runTest() {
+      int currentIndex = 0;
+      const bool useInkSparkle = !kIsWeb;


Why not on web?

I am pretty sure web supports shaders now, I wonder if we need to update the InkSparkle then...

I don't think InkSparkle shader would work on web + Android mobile (web might support shaders but it is probably on CanvasKit). But we might need to ask someone who knows more.

InkSparkle documentation explicitly mentionned it is not supported with the HTML renderer, see

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

Lines 17 to 18 in 6e9d4cc

/// This effect relies on a shader and therefore is unsupported on the Flutter

/// Web HTML backend.

As of today InkSparkle is deactived on Web. For instance, the useInkSparkle variable I used in this test is direcly inspired by the ThemeData implementation, see

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

Line 418 in 6e9d4cc

final bool useInkSparkle = platform == TargetPlatform.android && !kIsWeb;

And tests for InkSparkle are deactivated on Web, see

flutter/packages/flutter/test/material/ink_sparkle_test.dart

Line 33 in 6e9d4cc

skip: kIsWeb, // [intended] shaders are not yet supported for web.

Yeah I inquired about this yesterday after I noticed the same in the docs. I know that shaders are supported in canvaskit, maybe we need to update the sparkle and the docs to have it on canvaskit then? We could use this flag: https://api.flutter.dev/flutter/foundation/isCanvasKit.html

(We could also file a bug about this and follow up in a separate change 🙂 )

FYI, I filed #138487.

Piinks · 2023-11-13T21:16:18Z

packages/flutter/test/material/bottom_navigation_bar_test.dart

+
+      return MaterialApp(
+        // Because this test relies on golden, fo a constant seed for the ink sparkle animation.
+        theme: ThemeData(splashFactory: useInkSparkle ? InkSparkle.constantTurbulenceSeedSplashFactory : null),


Yes! constantTurbulenceSeedSplashFactory should eliminate the randomness we saw in the previous PR. 👍

Just double checking - this is the only test in this PR with the sparkle?

Yes, this is the only golden test in this PR.

For future M3 test migration PRs, this issue will surface on any golden test that simulates a tap on a widget tree that contains an InkWell. I don't know if there are a lot of them, the tricky part will be to be aware of this (Taha and me will take care on our migration PRs).

Yeah, I wonder, should we create an M3 test utility for this? You could create just a simple wrapper class that creates a MaterialApp with the turbulence seed set, and then re-use it in every M3 test so we'll know it is consistently being applied.

Oh! Actually scratch that. How about this instead - let's update InkSparkle so that in debug mode it always uses the fixed turbulence seed. That would be the best way to prevent this issue from reoccurring. Probably in the InkSparkle constructor, set the seed in an assert statement so in debug mode (including tests) it will always be fixed and consistent!

cc @TahaTesser

Piinks

LGTM

bleroux · 2023-11-21T07:50:53Z

I will push an update once #138757 will be merged.

…ter#137998)

Manual roll requested by [email protected] flutter/flutter@9c9e061...ab721f9 2023-11-21 [email protected] Roll Flutter Engine from 6da31e1bff67 to 746697c27569 (1 revision) (flutter/flutter#138826) 2023-11-21 [email protected] Add mhbdev to AUTHORS (flutter/flutter#138311) 2023-11-21 [email protected] Manual roll Flutter Engine from 70b1c7341255 to 6da31e1bff67 (10 revisions) (flutter/flutter#138817) 2023-11-21 [email protected] Fix Chips with Tooltip throw an assertion when enabling or disabling (flutter/flutter#138799) 2023-11-21 [email protected] Roll Packages from c5443ad to c9933fc (2 revisions) (flutter/flutter#138809) 2023-11-21 [email protected] Roll Flutter Engine from 3348fb0ca302 to 70b1c7341255 (1 revision) (flutter/flutter#138774) 2023-11-21 [email protected] Roll Flutter Engine from f3e9b38a2588 to 3348fb0ca302 (1 revision) (flutter/flutter#138772) 2023-11-21 [email protected] Roll Flutter Engine from 39fb4581cff2 to f3e9b38a2588 (3 revisions) (flutter/flutter#138770) 2023-11-21 [email protected] Add dartdoc warnings (flutter/flutter#138766) 2023-11-21 [email protected] Roll Flutter Engine from 6e8e55ef1a7f to 39fb4581cff2 (2 revisions) (flutter/flutter#138765) 2023-11-20 [email protected] Added Features requested in #137530 (flutter/flutter#137532) 2023-11-20 [email protected] Bump dartdoc to 7.0.2 (flutter/flutter#138760) 2023-11-20 [email protected] Roll Flutter Engine from 7a31543b4630 to 6e8e55ef1a7f (4 revisions) (flutter/flutter#138761) 2023-11-20 [email protected] [Reland] Introduce `AnimationStyle` (flutter/flutter#138721) 2023-11-20 [email protected] Roll Flutter Engine from 337ab58e81f7 to 7a31543b4630 (2 revisions) (flutter/flutter#138759) 2023-11-20 [email protected] Reland update bottom navigation bar test for m3 (flutter/flutter#137998) 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 [email protected],[email protected],[email protected] 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

This PR removes the call to `InkSparkle.constantTurbulenceSeedSplashFactory` which was introduced in #137998. This is now obsolete since #138757 was merged and InkSparkle randomness is no more active in tests.

…ter#137998)

Eerey · 2024-05-02T15:57:47Z

So just to be clear, InkSparkle.constantTurbulenceSeedSplashFactory is now additionally unavailable for a web-build with CanvasKit? Because it worked on flutter 3.13.9, but now on 3.19.6 it shows basically no visual onPress/onTap shader effect on my buttons.

bleroux added 2 commits November 7, 2023 08:53

Update BottomNavigationBar tests for M3

8d5f2d0

Use InkSparkle.constantTurbulenceSeedSplashFactory for one golden test

c3856aa

github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. labels Nov 7, 2023

flutter-dashboard bot added the will affect goldens Changes to golden files label Nov 7, 2023

bleroux requested a review from Piinks November 7, 2023 15:30

TahaTesser mentioned this pull request Nov 7, 2023

Fix ExpansionTile splash effect #135855

Closed

8 tasks

Piinks reviewed Nov 13, 2023

View reviewed changes

Piinks approved these changes Nov 20, 2023

View reviewed changes

Piinks added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 20, 2023

auto-submit bot merged commit 1a9e866 into flutter:master Nov 20, 2023

engine-flutter-autoroll mentioned this pull request Nov 21, 2023

Roll Flutter from 9c9e0617b149 to b6ef78e9551e (11 revisions) flutter/packages#5456

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 21, 2023

1a9e866 Reland update bottom navigation bar test for m3 (flutter/flut…

2701add

…ter#137998)

engine-flutter-autoroll mentioned this pull request Nov 21, 2023

Roll Flutter from 9c9e0617b149 to 2a8eedf08435 (12 revisions) flutter/packages#5457

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 21, 2023

1a9e866 Reland update bottom navigation bar test for m3 (flutter/flut…

bfaae0d

…ter#137998)

engine-flutter-autoroll mentioned this pull request Nov 21, 2023

Roll Flutter from 9c9e0617b149 to e6d43ac6d466 (13 revisions) flutter/packages#5458

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 21, 2023

1a9e866 Reland update bottom navigation bar test for m3 (flutter/flut…

583b4cd

…ter#137998)

engine-flutter-autoroll mentioned this pull request Nov 21, 2023

Roll Flutter from 9c9e0617b149 to 3cd723b496ba (14 revisions) flutter/packages#5459

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 21, 2023

1a9e866 Reland update bottom navigation bar test for m3 (flutter/flut…

dc5c2e7

…ter#137998)

engine-flutter-autoroll mentioned this pull request Nov 21, 2023

Roll Flutter from 9c9e0617b149 to 5ae1baeb9770 (15 revisions) flutter/packages#5460

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 21, 2023

1a9e866 Reland update bottom navigation bar test for m3 (flutter/flut…

4399d81

…ter#137998)

engine-flutter-autoroll mentioned this pull request Nov 21, 2023

Manual roll Flutter from 9c9e0617b149 to ab721f935ffc (16 revisions) flutter/packages#5461

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 21, 2023

1a9e866 Reland update bottom navigation bar test for m3 (flutter/flut…

1e118f2

…ter#137998)

bleroux mentioned this pull request Dec 4, 2023

Remove InkSparkle.constantTurbulenceSeedSplashFactory usage #139481

Merged

bleroux deleted the reland_update_bottom_navigation_bar_test_for_M3 branch December 10, 2023 14:54

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 16, 2024

1a9e866 Reland update bottom navigation bar test for m3 (flutter/flut…

fdfa48a

…ter#137998)

Eerey mentioned this pull request May 3, 2024

InkSparkle.constantTurbulenceSeedSplashFactory causes a runtime error in web #147771

Closed

	/// This effect relies on a shader and therefore is unsupported on the Flutter
	/// Web HTML backend.

Reland update bottom navigation bar test for m3 #137998

Reland update bottom navigation bar test for m3 #137998

Uh oh!

Conversation

bleroux commented Nov 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flutter-dashboard bot commented Nov 7, 2023

Uh oh!

bleroux commented Nov 7, 2023

Uh oh!

Piinks left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Piinks Nov 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Piinks left a comment

Choose a reason for hiding this comment

Uh oh!

bleroux commented Nov 21, 2023

Uh oh!

Eerey commented May 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bleroux commented Nov 7, 2023 •

edited

Loading

Piinks Nov 20, 2023 •

edited

Loading

Eerey commented May 2, 2024 •

edited

Loading