Make InkDecoration not paint if the ink is not visible #122585

tvolkert · 2023-03-14T07:26:45Z

Make InkDecoraiton not paint if the ink is not visible

This introduces a isVisible field in InkDecoration to control whether
the decoration is painted. It also introduces a Visibility.of() method
that can be used to determine the visibility state of widgets in the tree
based on Visibility ancestry state. This then updates Ink to consult
Visibility.of() and set the visible flag on its ink decoration
accordingly.

Finally, this updates IndexedStack to add a Visibility widget around
each of the stack's children, so that non-visible children of the stack
will be able to use Visibility.of() to know their visibility.

Putting it all together, this means that ink no longer paints itself when
it's in a non-selected index of an indexed stack.

#59963

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.

flutter-dashboard · 2023-03-14T07:26:49Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

tvolkert · 2023-03-14T07:28:01Z

@goderbauer or @HansMuller before I work hard on polish, I'm interested in if you think this is a decent way to go about this.

goderbauer · 2023-03-14T17:37:48Z

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

+  @override
+  Widget build(BuildContext context) {
+    List<Widget> wrappedChildren = List<Widget>.generate(children.length, (int i) {
+      return Visibility(


To maintain the documented properties of the IndexedStack ("The stack is always as big as the largest child.") this must set maintainSize to true.

(I hope we have a test that's failing because of this, if not, could you please add one?)

goderbauer · 2023-03-14T17:40:53Z

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

@@ -224,39 +224,46 @@ class Visibility extends StatelessWidget {
  /// objects, to be immediately created if [visible] is true).
  final bool maintainInteractivity;

+  static VisibilityMarker of(BuildContext context) {


Wondering: Should this just return true/false? Do you have any plans to extend VisibilityMarker? Or ideas what we should add in the future?

Good point - returning a boolean is cleaner.

goderbauer · 2023-03-14T17:43:11Z

To fully evaluate this, it would be useful to see what it would take to get all tests to pass again. Are those just failing because of the maintainSize problem? Or is there more to it?

tvolkert · 2023-03-14T20:47:03Z

Thanks @goderbauer! The failing tests were just due to the need to use Visibility.maintain(). I'm gonna clean this up a bit and will let you know when it's ready for a real review.

tvolkert · 2023-03-14T23:53:42Z

Ok, @goderbauer I think this is ready for a real review.

xu-baolin · 2023-03-15T05:39:33Z

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

@@ -279,13 +279,15 @@ class _InkState extends State<Ink> {
    if (_ink == null) {
      _ink = InkDecoration(
        decoration: widget.decoration,
+        isVisible: Visibility.of(context),


Can it be extended to parameters like boundaries to solve these issues at the same time?

tvolkert · 2023-03-17T19:58:04Z

ping @goderbauer 🙂

goderbauer

LGTM

goderbauer · 2023-03-20T17:00:21Z

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

+  bool get isVisible => _isVisible ?? true;
+  bool? _isVisible;


It looks like the fact that _isVisible is nullable is not really important? Maybe simplify to

Suggested change

bool get isVisible => _isVisible ?? true;

bool? _isVisible;

bool get isVisible => _isVisible;

bool _isVisible = true;

auto-submit · 2023-03-22T04:08:49Z

auto label is removed for flutter/flutter, pr: 122585, due to - The status or check suite Linux firebase_abstract_method_smoke_test has failed. Please fix the issues identified (or deflake) before re-applying this label.

This introduces a `isVisible` field in `InkDecoration` to control whether the decoration is painted. It also introduces a `Visibility.of()` method that can be used to determine the visibility state of widgets in the tree based on Visibility ancestry state. This then updates Ink to consult `Visibility.of()` and set the visible flag on its ink decoration accordingly. Finally, this updates `IndexedStack` to add a `Visibility` widget around each of the stack's children, so that non-visible children of the stack will be able to use `Visibility.of()` to know their visibility. Putting it all together, this means that ink no longer paints itself when it's in a non-selected index of an indexed stack. flutter#59963

…tter/flutter#122585)

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

tvolkert requested a review from goderbauer March 14, 2023 07:27

tvolkert mentioned this pull request Mar 14, 2023

Ink appears to bleed through IndexedStack #59963

Closed

goderbauer reviewed Mar 14, 2023

View reviewed changes

tvolkert force-pushed the indexed_stack branch from 68e0e8b to 422bbc7 Compare March 14, 2023 23:21

tvolkert changed the title ~~[WIP] Make InkDecoration not paint if the ink is not visible~~ Make InkDecoration not paint if the ink is not visible Mar 14, 2023

xu-baolin reviewed Mar 15, 2023

View reviewed changes

goderbauer approved these changes Mar 20, 2023

View reviewed changes

goderbauer mentioned this pull request Mar 20, 2023

Remove unused replacement from Visibility.maintain #123039

Merged

tvolkert force-pushed the indexed_stack branch from 19296fe to 36e99c0 Compare March 22, 2023 03:08

tvolkert added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 22, 2023

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Mar 22, 2023

tvolkert force-pushed the indexed_stack branch from 36e99c0 to 2097c00 Compare March 25, 2023 18:42

tvolkert added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 25, 2023

auto-submit bot merged commit 25174d6 into flutter:master Mar 25, 2023
72 checks passed

engine-flutter-autoroll mentioned this pull request Mar 26, 2023

Manual roll Flutter from a8afbfa22e34 to 25174d621e88 (19 revisions) flutter/packages#3547

Closed

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

25174d621 Make InkDecoraiton not paint if the ink is not visible (flu…

c2c8a2f

…tter/flutter#122585)

engine-flutter-autoroll mentioned this pull request Mar 26, 2023

Roll Flutter from a8afbfa22e34 to 1fb2158e3672 (20 revisions) flutter/packages#3550

Closed

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

25174d621 Make InkDecoraiton not paint if the ink is not visible (flu…

48ad14a

…tter/flutter#122585)

engine-flutter-autoroll mentioned this pull request Mar 26, 2023

Roll Flutter from a8afbfa22e34 to 1fb2158e3672 (20 revisions) flutter/packages#3551

Closed

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

25174d621 Make InkDecoraiton not paint if the ink is not visible (flu…

3465168

…tter/flutter#122585)

engine-flutter-autoroll mentioned this pull request Mar 27, 2023

Manual roll Flutter from a8afbfa22e34 to 4a1002e2e255 (21 revisions) flutter/packages#3552

Merged

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

25174d621 Make InkDecoraiton not paint if the ink is not visible (flu…

0e64973

…tter/flutter#122585)

This was referenced Mar 28, 2023

i123643 print java version gradle #123644

Merged

Add Java-Gradle-AGP validation to flutter analyze #123916

Merged

reidbaker mentioned this pull request Apr 5, 2023

Update getGradleVersion to ignore commented out lines #124260

Merged

8 tasks

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

25174d621 Make InkDecoraiton not paint if the ink is not visible (flu…

e636e45

…tter/flutter#122585)

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

25174d621 Make InkDecoraiton not paint if the ink is not visible (flu…

75402ed

…tter/flutter#122585)

TahaTesser mentioned this pull request May 29, 2023

IndexedStack no longer supports Positioned #127553

Open

2 tasks

tvolkert deleted the indexed_stack branch October 23, 2023 02:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make InkDecoration not paint if the ink is not visible #122585

Make InkDecoration not paint if the ink is not visible #122585

tvolkert commented Mar 14, 2023 •

edited

flutter-dashboard bot commented Mar 14, 2023

tvolkert commented Mar 14, 2023

goderbauer Mar 14, 2023

goderbauer Mar 14, 2023

goderbauer Mar 14, 2023

tvolkert Mar 14, 2023

goderbauer commented Mar 14, 2023

tvolkert commented Mar 14, 2023

tvolkert commented Mar 14, 2023

xu-baolin Mar 15, 2023

tvolkert commented Mar 17, 2023

goderbauer left a comment

goderbauer Mar 20, 2023

auto-submit bot commented Mar 22, 2023

Make InkDecoration not paint if the ink is not visible #122585

Make InkDecoration not paint if the ink is not visible #122585

Conversation

tvolkert commented Mar 14, 2023 • edited

Pre-launch Checklist

flutter-dashboard bot commented Mar 14, 2023

tvolkert commented Mar 14, 2023

goderbauer Mar 14, 2023

Choose a reason for hiding this comment

goderbauer Mar 14, 2023

Choose a reason for hiding this comment

goderbauer Mar 14, 2023

Choose a reason for hiding this comment

tvolkert Mar 14, 2023

Choose a reason for hiding this comment

goderbauer commented Mar 14, 2023

tvolkert commented Mar 14, 2023

tvolkert commented Mar 14, 2023

xu-baolin Mar 15, 2023

Choose a reason for hiding this comment

tvolkert commented Mar 17, 2023

goderbauer left a comment

Choose a reason for hiding this comment

goderbauer Mar 20, 2023

Choose a reason for hiding this comment

auto-submit bot commented Mar 22, 2023

tvolkert commented Mar 14, 2023 •

edited