Add HeroMode widget #48223

najeira · 2020-01-06T02:44:32Z

Description

When there are multiple Navigators like CupertinoPageScaffold, I want to disable heroes in the page that is not displayed.

HeroMode widget provides a means to control whether heroes is enabled or disabled.

Error

current Flutter, An error occurs in the following step:

Nested Navigators, such as CupertinoPageScaffold
Inner CupertinoTabViews has the same name Heroes
Show both inner CupertinoTabViews to init
Push route that has the same name Hero to root Navigator

import 'package:flutter/cupertino.dart';
import 'package:flutter/material.dart';

const String kHeroTag = "hero tag";

void main() => runApp(CupertinoApp(
      title: 'Hero with CupertinoTabScaffold',
      home: HomePage(),
    ));

class HomePage extends StatelessWidget {
  @override
  Widget build(BuildContext context) {
    return CupertinoTabScaffold(
      tabBar: CupertinoTabBar(
        items: <BottomNavigationBarItem>[
          BottomNavigationBarItem(icon: Icon(Icons.home)),
          BottomNavigationBarItem(icon: Icon(Icons.favorite)),
        ],
      ),
      tabBuilder: (BuildContext context, int index) {
        return CupertinoTabView(
          builder: (BuildContext context) => FirstPage(),
        );
      },
    );
  }
}

class FirstPage extends StatelessWidget {
  @override
  Widget build(BuildContext context) {
    return CupertinoPageScaffold(
      navigationBar: CupertinoNavigationBar(),
      child: Center(
        child: Column(
          mainAxisAlignment: MainAxisAlignment.center,
          children: <Widget>[
            Hero(
              tag: kHeroTag,
              child: Container(
                width: 100,
                height: 100,
                color: Colors.red,
              ),
            ),
            RaisedButton(
              onPressed: () {
                Navigator.of(context, rootNavigator: false).push(
                  CupertinoPageRoute(
                    builder: (BuildContext context) => NextPage(),
                  ),
                );
              },
              child: Text("Next in tab"),
            ),
            RaisedButton(
              onPressed: () {
                Navigator.of(context, rootNavigator: true).push(
                  CupertinoPageRoute(
                    builder: (BuildContext context) => NextPage(),
                  ),
                );
              },
              child: Text("Next out tab"),
            ),
          ],
        ),
      ),
    );
  }
}

class NextPage extends StatelessWidget {
  @override
  Widget build(BuildContext context) {
    return CupertinoPageScaffold(
      navigationBar: CupertinoNavigationBar(),
      child: Center(
        child: Hero(
          tag: kHeroTag,
          child: Container(
            width: 100,
            height: 100,
            color: Colors.red,
          ),
        ),
      ),
    );
  }
}

Related Issues

#44890
#28042

Tests

I added the following tests:

test/widgets/heroes_test.dart
- Can push/pop on outer Navigator if nested Navigators contains same Heroes
- Heroes in enabled HeroMode do transition
- Heroes in disabled HeroMode do not transition

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I signed the CLA.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I updated/added relevant documentation (doc comments with ///).
All existing and new tests are passing.
The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require Flutter developers to manually update their apps to accommodate your change?

Yes, this is a breaking change (Please read Handling breaking changes). Replace this with a link to the e-mail where you asked for input on this proposed change.
No, this is not a breaking change.

This is the addition of a new widget, will not break existing apps.

justinmc

Sorry this took so long to get a review! Here are a few minor changes that I saw. I checked out the branch and confirmed that the tests properly test the added functionality.

We should get an approval of the overall strategy from @goderbauer since you guys were talking about this approach in the previous PR.

justinmc · 2020-02-19T21:33:13Z

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

Also the child argument must not be null.

justinmc · 2020-02-19T21:35:06Z

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

What do you think about replacing "heroes" with [Hero]es? Here and below.

justinmc · 2020-02-19T21:36:13Z

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

Nit: Maybe shorten this to: (widget is HeroMode && !widget.enabled).

justinmc · 2020-02-19T21:43:02Z

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

I'm trying to think of a more accurate way to describe this since I don't know what "perform" means at first glance. Is this really just enabling/disabling animations? Maybe this is better:

/// Whether or not [Hero]es are enabled in this subtree. /// /// If true, then the [Hero]es in this subtree will animate as usual. /// /// If false, then the [Hero]es in this subtree will not animate. /// /// Defaults to true and must not be null.

justinmc · 2020-02-19T21:51:38Z

packages/flutter/test/widgets/heroes_test.dart

Put a period at the end of this comment (and a few others below).

justinmc · 2020-02-28T00:08:51Z

@najeira Are you still available to help finish up this PR?

najeira · 2020-02-28T01:50:09Z

Yes! Thanks for the review. Sorry for noticing late.

I will update.

justinmc · 2020-02-28T16:24:56Z

Cool, no problem! Thanks for taking a look at my review.

najeira · 2020-03-03T06:06:09Z

Some CI tests have failed.

|   error • The getter 'subtitle1' isn't defined for the class 'TextTheme' • lib/src/views/seamail.dart:196:104 • undefined_getter
|   error • The argument type 'dynamic' can't be assigned to the parameter type 'TextStyle' • lib/src/views/seamail.dart:199:35 • argument_type_not_assignable
|   error • The getter 'bodyText1' isn't defined for the class 'TextTheme' • lib/src/views/seamail.dart:199:58 • undefined_getter
|   error • The argument type 'dynamic' can't be assigned to the parameter type 'TextStyle' • lib/src/views/seamail.dart:467:23 • argument_type_not_assignable

...

Should I rebase?

justinmc · 2020-03-05T23:41:09Z

@najeira Yes, it looks like that's not caused by your changes. Rebase with master (or merge master) and hopefully it will go away. And let me know if you're ready for me to review again. Thanks!

goderbauer

Approach seems fine.

goderbauer · 2020-03-26T16:44:40Z

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

This could inherit from ProxyWidget instead?

ProxyWidget is abstract, the code addition will increase. SingleChildRenderObjectWidget is good for this, right?

Is it better to inherit ProxyWidget and return SingleChildRenderObjectElement?

No, SingleChildRenderObjectWidget is expected to have a renderObject, The HeroMode itself does not layout or paint in the render layer. It is only adding information to the widget tree. The ProxyWidget returns ComponentElement might make most sense in this case.

Note: there is a ProxyElement, but it has specific logic built in to notify child when its configuration changes. We do not need this functionality, so i think ProxyElement is not suitable in this use case

This seems not be addressed yet?

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

chunhtai · 2020-03-26T17:40:27Z

I cannot reproduce the original error, and #28042 seems to be fixed.
What is this pr aim to fix?

najeira · 2020-03-27T02:31:52Z

Explanation of reproduction procedure was not enough.

Try:

launch app
show heart tab
push "Next out tab"

Error:

════════ Exception caught by scheduler library ═════════════════════════════════════════════════════
The following assertion was thrown during a scheduler callback:
There are multiple heroes that share the same tag within a subtree.

Within each subtree for which heroes are to be animated (i.e. a PageRoute subtree), each Hero must have a unique non-null tag.
In this case, multiple heroes had the following tag: hero tag

Here is the subtree for one of the offending heroes: Hero
  tag: hero tag
  dependencies: [_ModalScopeStatus]
  state: _HeroState#b2991
When the exception was thrown, this was the stack: 
#0      Hero._allHeroesFor.inviteHero.<anonymous closure> (package:flutter/src/widgets/heroes.dart:266:11)
#1      Hero._allHeroesFor.inviteHero (package:flutter/src/widgets/heroes.dart:277:8)
#2      Hero._allHeroesFor.visitor (package:flutter/src/widgets/heroes.dart:305:23)
#3      MultiChildRenderObjectElement.visitChildren (package:flutter/src/widgets/framework.dart:5929:16)
#4      Hero._allHeroesFor.visitor (package:flutter/src/widgets/heroes.dart:309:15)
...
════════════════════════════════════════════════════════════════════════════════════════════════════

goderbauer · 2020-03-27T17:28:47Z

@chunhtai can you reproduce this with the additional steps?

justinmc · 2020-03-27T18:05:28Z

I got a crash when I followed those steps on the iOS simulator, and I confirmed that this PR fixes it.

chunhtai · 2020-03-27T19:12:05Z

I see, yes i can reproduce it now. Can you open a new issue about this? It is fixing a different problem from #28042

packages/flutter/lib/src/cupertino/tab_scaffold.dart

chunhtai · 2020-03-27T21:07:58Z

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

nit: this have to be a full sentence.
Maybe combine both sentences.
If this property is false, the [Hero]es in this subtree will not animate on route changes; Otherwise, they will animate as usual

this seems not addressed yet?

chunhtai · 2020-03-27T21:35:11Z

packages/flutter/test/widgets/heroes_test.dart

I might miss something, but is this really related to nested navigators? I thought the problem is the CupertinoTabScaffold builds a stack where some of the children are offstage and the heroes in those children conflicting with each other?

This is not addressed yet?

Heroes are animated by screen transition. This is related to Navigator. The problem is caused by multiple nested Navigators.

packages/flutter/test/widgets/heroes_test.dart

najeira · 2020-03-28T01:01:59Z

Thanks for the review!

I'm not good at English. I'm not sure how to fix any documentation issues.

I will check and respond to issues regarding the implementation.

justinmc · 2020-04-10T21:30:00Z

@najeira How's it going with these last few comments? Let me know if anything isn't clear about the documentation and I can help out by writing the English.

najeira · 2020-04-27T05:45:52Z

updated comments

justinmc

LGTM but @chunhtai and/or @goderbauer should take a final look before we merge. There are one or two comments that I'm not sure have been addressed.

chunhtai

Some of the comments have not been addressed yet

chunhtai · 2020-04-27T17:01:29Z

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

This seems not be addressed yet?

chunhtai · 2020-04-27T17:01:44Z

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

chunhtai · 2020-04-27T17:01:53Z

packages/flutter/test/widgets/heroes_test.dart

chunhtai · 2020-10-02T16:22:20Z

Could anybody show me an example using ProxyWidget and ProxyElement?

The ProxyElement is for inheritedElement that it implement extra logic

najeira · 2020-10-03T03:40:06Z

Could anybody show me an example using ProxyWidget and ComponentElement?

najeira · 2020-10-03T03:42:12Z

Is Widget build(BuildContext context) => child; a heavy process to avoid?

najeira · 2020-10-03T03:51:46Z

In my understanding, implementing HeroModeElement skips the call HeroMode.build.

extends StatelessWidget:

StatelessElement.build
HeroMode.build
returned child widget is used

extends ProxyWidget and impelement HeroModeElement:

HeroModeElement.build
returned child widget is used

Is this all? Please let me know if there are any other processes that affect performance.

chunhtai · 2020-10-05T16:19:14Z

In my understanding, implementing HeroModeElement skips the call HeroMode.build.

extends StatelessWidget:

StatelessElement.build

HeroMode.build

returned child widget is used

extends ProxyWidget and impelement HeroModeElement:

HeroModeElement.build

returned child widget is used

Is this all? Please let me know if there are any other processes that affect performance.

yes that is all as far as I know

Is Widget build(BuildContext context) => child; a heavy process to avoid?

It is not a heavy process, but we can improve it by implementing the HeroModeElement which is not too much work.

najeira · 2020-10-06T02:31:50Z

Widget build(BuildContext context) => child; is very very small task, it is hard to find even in performance measurement. It does not allocate any widgets, just returns variable. I think that implementing the HeroModeElement is too much work to avoid its micro task. I'd rather keep the code simple than avoid performance issues that are too small to find.

If we should optimize it, it is better to do it in another PR together with the following classes:

NotificationListener
KeyedSubtree
PageStorage
PreferredSize

These widgets inherit StatelessWidget and do Widget build(BuildContext context) => child;, not inherit ProxyWidget. They are the same as this case.

Additionally: what do you think of this comment?

StatelessElement uses Element._dirty, HeroModeElement can not use the variable because it class is in the outside. And it is not good to implement HeroModeElement in widgets/framework.dart.

I want you to comment on which is preferable.

CC: @justinmc @goderbauer

chunhtai

LGTM, with some nit pick

chunhtai · 2020-10-06T16:34:27Z

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

this seems not addressed yet?

chunhtai · 2020-10-06T16:34:46Z

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

this can be an arrow function

chunhtai · 2020-10-06T16:35:02Z

packages/flutter/test/widgets/heroes_test.dart

This is not addressed yet?

chunhtai · 2020-10-06T16:36:53Z

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

Suggested change

/// When [enabled] is true (the default), Hero widgets may be involved in a

/// When [enabled] is true (the default), [Hero] widgets may be involved in

chunhtai · 2020-10-06T16:37:38Z

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

Suggested change

/// Hero animation, like normal.

/// hero animations, as usual.

chunhtai · 2020-10-06T16:38:10Z

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

Suggested change

/// When [enabled] is false, all Hero widgets in this subtree will not be

/// When [enabled] is false, all [Hero] widgets in this subtree will not be

chunhtai · 2020-10-06T16:38:41Z

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

Suggested change

/// considered for Hero animations.

/// involved in hero animations.

najeira · 2020-10-12T03:26:42Z

diff: najeira/flutter@70815c7...a76023b

chunhtai · 2020-10-12T16:31:12Z

@najeira it seems like this branch is outdated, can you try to rebase off latest master and see if you can fix the cirrus?

najeira · 2020-10-28T01:44:39Z

I rebased this branch.

chunhtai · 2020-10-30T19:09:56Z

@najeira
I am not sure what is going on with the presubmit, but we have some infra change in the last couple of days. Can you rebase off the latest master again and see if it makes cirrus happy?

najeira · 2020-11-01T11:31:31Z

I am very grateful for the long discussions and for helping me with my poor English. Thank you all.

justinmc · 2020-11-02T16:18:21Z

Just saw that this was merged, thank you @najeira!

fluttergithubbot added f: cupertino flutter/packages/flutter/cupertino repository framework flutter/packages/flutter repository. See also f: labels. labels Jan 6, 2020

googlebot added the cla: yes label Jan 6, 2020

justinmc reviewed Feb 19, 2020

View reviewed changes

justinmc added the waiting for customer response The Flutter team cannot make further progress on this issue until the original reporter responds label Feb 28, 2020

justinmc removed the waiting for customer response The Flutter team cannot make further progress on this issue until the original reporter responds label Feb 28, 2020

najeira force-pushed the hero-mode branch 2 times, most recently from 9d3b345 to 5180231 Compare March 9, 2020 02:01

Piinks added a: animation Animation APIs f: routes Navigator, Router, and related APIs. labels Mar 18, 2020

goderbauer reviewed Mar 26, 2020

View reviewed changes

chunhtai reviewed Mar 27, 2020

View reviewed changes

packages/flutter/lib/src/cupertino/tab_scaffold.dart Outdated Show resolved Hide resolved

chunhtai reviewed Mar 27, 2020

View reviewed changes

justinmc added the waiting for customer response The Flutter team cannot make further progress on this issue until the original reporter responds label Apr 17, 2020

najeira force-pushed the hero-mode branch from 5180231 to 4e05caf Compare April 27, 2020 05:20

justinmc approved these changes Apr 27, 2020

View reviewed changes

chunhtai reviewed Apr 27, 2020

View reviewed changes

chunhtai approved these changes Oct 6, 2020

View reviewed changes

najeira force-pushed the hero-mode branch 2 times, most recently from 1a722e4 to 3e317ef Compare October 21, 2020 03:50

chunhtai added waiting for tree to go green and removed waiting for customer response The Flutter team cannot make further progress on this issue until the original reporter responds labels Oct 28, 2020

najeira added 11 commits October 31, 2020 11:21

Add HeroMode widget

043b258

Update comments for HeroMode

a729bb6

Simplify if statement

2814b6e

Add type annotation

f3b6cb4

Use const constructor

568ff4a

Update a comment for HeroMode

2e690fc

update comment

5025d01

Simplified by arrow function

9486175

Fix comment

2c2cbf4

fix for null-safety

bf80e81

Fix for null-safety

885272f

najeira force-pushed the hero-mode branch from 3e317ef to 885272f Compare October 31, 2020 02:52

fluttergithubbot merged commit 72267a6 into flutter:master Oct 31, 2020

	/// When [enabled] is true (the default), Hero widgets may be involved in a
	/// When [enabled] is true (the default), [Hero] widgets may be involved in

	/// Hero animation, like normal.
	/// hero animations, as usual.

	/// When [enabled] is false, all Hero widgets in this subtree will not be
	/// When [enabled] is false, all [Hero] widgets in this subtree will not be

	/// considered for Hero animations.
	/// involved in hero animations.

Add HeroMode widget #48223

Add HeroMode widget #48223

Conversation

najeira commented Jan 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Error

Related Issues

Tests

Checklist

Breaking Change

Uh oh!

justinmc 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!

justinmc commented Feb 28, 2020

Uh oh!

najeira commented Feb 28, 2020

Uh oh!

justinmc commented Feb 28, 2020

Uh oh!

najeira commented Mar 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

justinmc commented Mar 5, 2020

Uh oh!

goderbauer 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!

Uh oh!

chunhtai commented Mar 26, 2020

Uh oh!

najeira commented Mar 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

goderbauer commented Mar 27, 2020

Uh oh!

justinmc commented Mar 27, 2020

Uh oh!

chunhtai commented Mar 27, 2020

Uh oh!

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!

Uh oh!

najeira commented Mar 28, 2020

Uh oh!

justinmc commented Apr 10, 2020

najeira commented Jan 6, 2020 •

edited

Loading

najeira commented Mar 3, 2020 •

edited

Loading

najeira commented Mar 27, 2020 •

edited

Loading

najeira commented Oct 6, 2020 •

edited

Loading