[Material] Use InkSparkle for splashFactory in ThemeData when useMaterial3 is true for Android non-web runtimes #99882

clocksmith · 2022-03-10T00:04:41Z

Description

Use InkSparkle for splashFactory (material 3 ripple) in ThemeData when useMaterial3 is set to true, the platform is Android, and the compilation target is not Web.
One step closer to closing Introduce new Android 12 style ink ripple #82850, which can be closed once New buttons should use InkSparkle for splashFactory when useMaterial3 is true #99884 is closed.
closes ThemeData should use InkSparkle for splashFactory when useMaterial3 is true #99886

Tests

Added platform tests for using the correct splashFactory when useMaterial3 is false
Added platform tests for using the correct splashFactory when useMaterial3 is true
Existing ThemeData global test continues to pass.

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.

clocksmith · 2022-03-10T01:43:56Z

not sure why one of the tests is failing, the case where it should be InkSparkle...

══╡ EXCEPTION CAUGHT BY FLUTTER TEST FRAMEWORK ╞════════════════════════════════════════════════════
The following TestFailure was thrown running a test:
Expected: <Instance of '_InkSparkleFactory'>
  Actual: <Instance of '_InkSplashFactory'>

When the exception was thrown, this was the stack:
#4      main.<anonymous closure> (file:///Users/xxxxxx/git/flutter/packages/flutter/test/material/theme_data_test.dart:279:9)
#5      main.<anonymous closure> (file:///Users/xxxxxx/git/flutter/packages/flutter/test/material/theme_data_test.dart:271:97)
#6      testWidgets.<anonymous closure>.<anonymous closure> (package:flutter_test/src/widget_tester.dart:170:29)
<asynchronous suspension>
<asynchronous suspension>
(elided one frame from package:stack_trace)

This was caught by the test expectation on the following line:
  file:///Users/xxxxxx/git/flutter/packages/flutter/test/material/theme_data_test.dart line 279
The test description was:
  splashFactory is InkSparkle only for Android non-web when useMaterial3 is true (variant:
  TargetPlatform.android)
════════════════════════════════════════════════════════════════════════════════════════════════════
00:03 +21 -1: splashFactory is InkSparkle only for Android non-web when useMaterial3 is true (variant: TargetPlatform.android) [E]                                                                                                                       
  Test failed. See exception logs above.
  The test description was: splashFactory is InkSparkle only for Android non-web when useMaterial3 is true (variant: TargetPlatform.android)
  
00:03 +53 -1: Some tests failed.

darrenaustin

Thx for doing this. Looks good, just a couple of suggestions below.

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

packages/flutter/test/material/theme_data_test.dart

darrenaustin · 2022-03-10T07:33:12Z

not sure why one of the tests is failing, the case where it should be InkSparkle...

══╡ EXCEPTION CAUGHT BY FLUTTER TEST FRAMEWORK ╞════════════════════════════════════════════════════
The following TestFailure was thrown running a test:
Expected: <Instance of '_InkSparkleFactory'>
  Actual: <Instance of '_InkSplashFactory'>

As I mentioned in the review, this is due to useMaterial3 only changing the default splash factory if it is set in the constructor, and not in copyWith.

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

LasseRosenow · 2022-03-10T10:18:38Z

I'm curious: is the sparkle effect not part of material design, or why is it only applied on Android?

clocksmith · 2022-03-10T14:06:04Z

Understood about the useMaterial3 flag and how it only responds to constructor, and not copy with. I modified the paragraphs that explain this, and added a list structure to add new properties that are affected by flag

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

Co-authored-by: Pierre-Louis <[email protected]>

clocksmith · 2022-03-10T22:46:50Z

I'm curious: is the sparkle effect not part of material design, or why is it only applied on Android?

The current goal is to make a ripple in Flutter that is consistent with Android 12. I will try to find out a better answer for your question though.

fluttergithubbot · 2022-03-11T06:20:17Z

This pull request is not suitable for automatic merging in its current state.

The status or check suite Linux framework_tests_libraries has failed. Please fix the issues identified (or deflake) before re-applying this label.
The status or check suite Mac framework_tests_libraries has failed. Please fix the issues identified (or deflake) before re-applying this label.
The status or check suite Windows framework_tests_libraries has failed. Please fix the issues identified (or deflake) before re-applying this label.
The status or check suite framework_tests-libraries-linux has failed. Please fix the issues identified (or deflake) before re-applying this label.

clocksmith · 2022-03-12T19:14:45Z

I think some tests are failing due to tickers not being stopped when the widget containing a FragmentProgram is disposed. Other tests pass probably because the shader compilation finishes so quickly, but some tests do not give it time.

Is there currently a way to stop/cancel the async shader compilation once started? @zanderso @chinmaygarde @flar is there something that can be done with the FragmentProgram in onDispose here, or should the affected tests be fixed with something like pumpAndSettle or waiting till the compilation is done?

Test exception:

02:06 +3129 ~1: /b/s/w/ir/x/w/flutter/packages/flutter/test/material/navigation_rail_test.dart: onDestinationSelected is not called if null                                                            
Pending timers:
Timer (duration: 0:00:00.000000, periodic: false), created:
#0      new FakeTimer._ (package:fake_async/fake_async.dart:284:41)
#1      FakeAsync._createTimer (package:fake_async/fake_async.dart:248:27)
#2      FakeAsync.run.<anonymous closure> (package:fake_async/fake_async.dart:181:19)
#7      FragmentProgram.compile (dart:ui/painting.dart:3778:12)
#8      FragmentShaderManager.compile (package:flutter/src/material/ink_sparkle.dart:620:41)
#9      FragmentShaderManager.inkSparkle (package:flutter/src/material/ink_sparkle.dart:614:19)
#10     _InkSparkleFactory.compileShaderIfNeccessary (package:flutter/src/material/ink_sparkle.dart:427:29)
#11     new InkSparkle (package:flutter/src/material/ink_sparkle.dart:125:24)
#12     _InkSparkleFactory.create (package:flutter/src/material/ink_sparkle.dart:452:12)
#13     _InkResponseState._createInkFeature (package:flutter/src/material/ink_well.dart:912:72)
#14     _InkResponseState._startSplash (package:flutter/src/material/ink_well.dart:990:42)
#15     _InkResponseState._handleTapDown (package:flutter/src/material/ink_well.dart:971:5)
#16     TapGestureRecognizer.handleTapDown.<anonymous closure> (package:flutter/src/gestures/tap.dart:586:61)
#17     GestureRecognizer.invokeCallback (package:flutter/src/gestures/recognizer.dart:198:24)
#18     TapGestureRecognizer.handleTapDown (package:flutter/src/gestures/tap.dart:586:11)
#19     BaseTapGestureRecognizer._checkDown (package:flutter/src/gestures/tap.dart:289:5)
#20     BaseTapGestureRecognizer.acceptGesture (package:flutter/src/gestures/tap.dart:267:7)
#21     GestureArenaManager._resolveByDefault (package:flutter/src/gestures/arena.dart:251:25)
#22     GestureArenaManager._tryToResolveArena.<anonymous closure> (package:flutter/src/gestures/arena.dart:232:31)
#29     FakeAsync.flushMicrotasks (package:fake_async/fake_async.dart:193:32)
#30     AutomatedTestWidgetsFlutterBinding.runTest.<anonymous closure> (package:flutter_test/src/binding.dart:1256:17)
#31     AutomatedTestWidgetsFlutterBinding.runTest.<anonymous closure> (package:flutter_test/src/binding.dart:1243:35)
(elided 25 frames from dart:async and package:stack_trace)

══╡ EXCEPTION CAUGHT BY FLUTTER TEST FRAMEWORK ╞════════════════════════════════════════════════════
The following assertion was thrown running a test:
A Timer is still pending even after the widget tree was disposed.
'package:flutter_test/src/binding.dart':
Failed assertion: line 1291 pos 12: '!timersPending'

When the exception was thrown, this was the stack:
#2      AutomatedTestWidgetsFlutterBinding._verifyInvariants (package:flutter_test/src/binding.dart:1291:12)
#3      TestWidgetsFlutterBinding._runTestBody (package:flutter_test/src/binding.dart:848:7)
<asynchronous suspension>
(elided 2 frames from class _AssertionError)

The test description was:
  onDestinationSelected is not called if null
════════════════════════════════════════════════════════════════════════════════════════════════════

chinmaygarde · 2022-03-14T19:24:56Z

Is there currently a way to stop/cancel the async shader compilation once started? ... is there something that can be done with the FragmentProgram in onDispose here...

While shader compilation may be async, it isn't concurrent. And there is no way to stop a compilation once it has started on the UI thread. If shader compilation takes too long, the task to service the onDispose call will be queued behind the completion of shader compilation (as will any other tasks on the UI thread).

…tap but not pump

… useMaterial3 is true for Android non-web runtimes (flutter/flutter#99882)

restrict InkSparkle to Android non-web and material 3

ef42fab

clocksmith added f: material design flutter/packages/flutter/material repository. work in progress; do not review labels Mar 10, 2022

flutter-dashboard bot added the framework flutter/packages/flutter repository. See also f: labels. label Mar 10, 2022

clocksmith added 2 commits March 9, 2022 19:22

update docs

cabee03

tests

63e82de

clocksmith changed the title ~~restrict InkSparkle to Android non-web and material 3~~ [Material] Use InkSparkle for Android non-web when useMaterial3 is set to true in ThemeData Mar 10, 2022

clocksmith added 2 commits March 9, 2022 19:45

tests

2e5614a

merge

ee0f5cd

clocksmith requested review from darrenaustin and guidezpl March 10, 2022 00:48

clocksmith added 4 commits March 9, 2022 19:52

tests

5533ba0

tests

584869a

fix switch statement to maintain tap target size setting

495909a

update

1adc63b

clocksmith removed the work in progress; do not review label Mar 10, 2022

formatting

ba58905

darrenaustin suggested changes Mar 10, 2022

View reviewed changes

guidezpl reviewed Mar 10, 2022

View reviewed changes

packages/flutter/lib/src/material/theme_data.dart Outdated Show resolved Hide resolved

clocksmith added 2 commits March 10, 2022 08:54

fix docs

24aec89

fix docs

42569b2

clocksmith added 2 commits March 10, 2022 09:17

add comment for skip test

a584f8e

analyzer

cf7dbfc

guidezpl reviewed Mar 10, 2022

View reviewed changes

packages/flutter/lib/src/material/theme_data.dart Outdated Show resolved Hide resolved

guidezpl approved these changes Mar 10, 2022

View reviewed changes

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

d81a7a9

Co-authored-by: Pierre-Louis <[email protected]>

Merge branch 'master' into m3-ripple

c5d20eb

Use InkSparkle when m3 theme is enabled on Android non-web

c46753a

clocksmith added the waiting for tree to go green label Mar 11, 2022

flutter deleted a comment from flutter-dashboard bot Mar 11, 2022

fluttergithubbot removed the waiting for tree to go green label Mar 11, 2022

clocksmith added 3 commits March 11, 2022 08:32

Merge branch 'master' into m3-ripple

4d4ef3d

Merge branch 'master' into m3-ripple

2025c09

Fix test that does not let the static shader compilation finish

1220491

clocksmith added 3 commits March 14, 2022 19:20

Merge branch 'master' into m3-ripple

ffc6d9e

pumpAndSettle for pending shader compilation on edge case tests that …

c23ec5a

…tap but not pump

Merge branch 'master' into m3-ripple

62d2a32

clocksmith changed the title ~~[Material] Use InkSparkle for Android non-web when useMaterial3 is set to true in ThemeData~~ [Material] Use InkSparkle for splashFactory in ThemeData when useMaterial3 is true for Android non-web runtimes Mar 15, 2022

clocksmith added 3 commits March 15, 2022 16:12

Merge branch 'master' into m3-ripple

ebaa84c

Merge branch 'master' into m3-ripple

8ac627b

Merge branch 'master' into m3-ripple

98f2ab4

clocksmith merged commit 7c73053 into flutter:master Mar 18, 2022

engine-flutter-autoroll mentioned this pull request Mar 18, 2022

Roll Flutter from 1c2c9421121f to 136ff6dad673 (42 revisions) flutter/plugins#5070

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Mar 18, 2022

7c73053 [Material] Use InkSparkle for splashFactory in ThemeData when…

53b0f08

… useMaterial3 is true for Android non-web runtimes (flutter/flutter#99882)

engine-flutter-autoroll mentioned this pull request Mar 18, 2022

Roll Flutter from e99a66a47ef3 to 136ff6dad673 (18 revisions) flutter/packages#1253

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 18, 2022

7c73053 [Material] Use InkSparkle for splashFactory in ThemeData when…

b75207d

… useMaterial3 is true for Android non-web runtimes (flutter/flutter#99882)

engine-flutter-autoroll mentioned this pull request Mar 18, 2022

Roll Flutter from 1c2c9421121f to 795fe375bc41 (45 revisions) flutter/plugins#5071

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Mar 18, 2022

7c73053 [Material] Use InkSparkle for splashFactory in ThemeData when…

741176a

… useMaterial3 is true for Android non-web runtimes (flutter/flutter#99882)

engine-flutter-autoroll mentioned this pull request Mar 18, 2022

Roll Flutter from 1c2c9421121f to a217bbb6defe (46 revisions) flutter/plugins#5072

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Mar 18, 2022

7c73053 [Material] Use InkSparkle for splashFactory in ThemeData when…

4344b91

… useMaterial3 is true for Android non-web runtimes (flutter/flutter#99882)

bleroux mentioned this pull request Nov 15, 2023

Activate InkSparkle on Canvaskit (Android only) #138487

Open

2 tasks

[Material] Use InkSparkle for splashFactory in ThemeData when useMaterial3 is true for Android non-web runtimes #99882

[Material] Use InkSparkle for splashFactory in ThemeData when useMaterial3 is true for Android non-web runtimes #99882

Uh oh!

Conversation

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

Uh oh!

Description

Tests

Pre-launch Checklist

Uh oh!

clocksmith commented Mar 10, 2022

Uh oh!

darrenaustin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

LasseRosenow commented Mar 10, 2022

Uh oh!

clocksmith commented Mar 10, 2022

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

fluttergithubbot commented Mar 11, 2022

Uh oh!

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

Uh oh!

Uh oh!

chinmaygarde commented Mar 14, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

clocksmith commented Mar 10, 2022 •

edited

Loading

darrenaustin commented Mar 10, 2022 •

edited

Loading

clocksmith commented Mar 10, 2022 •

edited

Loading

clocksmith commented Mar 12, 2022 •

edited

Loading