Do not visit offstage subtrees when hero transition

[najeira]

Description

When hero transition, no need to visit offstage subtrees.

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

#29069
#28042

Tests

I added the following tests:

• test/widgets/heroes_test.dart: Can push/pop on outer Navigator if nested Navigators contains same Heroes

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 ///).
  • no comments added
• 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.

[najeira]

I'm going to fix format of test file.

[najeira]

fixed and force pushed

[goderbauer]

Special-casing the Offstage widget is not a good idea as there may be other widgets that will make a Hero "unreachable". We should see if we can fix this problem in a more generic way.

[najeira]

Agree, It's not a good design that Hero knows Offstage. However, the current implementation throws exceptions, I think it is a reasonable first aid.

[Piinks]

I agree with @goderbauer. This is actually a violation of the Flutter style guide: https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes.
Furthermore, addressing the root issue is preferable to a 'first aid' or temporary solution. What is the root issue here that is causing the exception?

[najeira]

Root issue: When there are multiple Navigators such as CupertinoPageScaffold, Hero searches for an inactive Navigators that is not related to the Hero-animating being performed. If the same Heroes exists in different Navigators, the active Navigator's Hero should be selected.

[najeira]

Should each Navigators have a widget like HeroStage that indicates whether or not to participate in a Hero animation and should be switched according to active / inactive? The mechanism is similar to Offstage, but it is a way to explicitly add Hero-related widgets.

[goderbauer]

Having an explicit HeroVisibility widget that allows you to turn off Heroes in a certain subtree seems like a good approach. When the _TabSwitchingViewState marks something as offstage it could then also turn off Heroes in that subtree.

[goderbauer]

I am going to close this PR. Feel free to open a new one with the other approach.