Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Exception on Nested TabBarView disposal #31581

Merged
merged 14 commits into from
May 4, 2019

Conversation

shihaohong
Copy link
Contributor

@shihaohong shihaohong commented Apr 24, 2019

Description

When a TabBarView is nested in a page of another TabBarView, there will be particular scenarios that will throw an exception.

To reproduce this issue, two conditions must be true:

  1. Tap on a tab that is adjacent to a tab containing a nested TabBarView
  2. The nested tab has to be in between the initial tab and the tab that was tapped on
    (ie. current index = 0, tap on tab at index 3 when tab at index 2 has nested TabBarView)

This happens when the framework tries to build _PagePosition, then dispose of it before applyViewportDimension has the chance to be called to set pixels to a non-null value based on the size of the viewport. This fix adds a flag to determine if applyViewportDimension is invoked before dispose. If it hasn't been, then dispose will set pixels to an arbitrary value before attempting to dispose of it.

First exception: This is due to the introduction of #29188, which happens because the initial swap that is necessary for warping causes the nested page that is swapped but not shown to be disposed of before applyViewportDimension to be called.

Second exception: This one is caused by the animation of the page during warp. The last frame does not build before the second setState is called to swap the children back to their original positions, causing the nested page to be built and disposed of before applyViewportDimension can be called.

TODO:

Related Issues

Fixes #18756

Tests

I added the following tests:

  • A regression test to ensure that the exception does throw when a tab adjacent to nested tab is selected. It tests for the first exception case mentioned above. A separate issue will be created to track writing a test for the second exception case.

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.

@shihaohong shihaohong closed this Apr 24, 2019
@shihaohong shihaohong changed the title [ (WIP) Fix Exception on Nested TabBar/TabBarView disposal Apr 24, 2019
@shihaohong shihaohong reopened this Apr 25, 2019
@shihaohong shihaohong added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. c: crash Stack traces logged to the console labels Apr 25, 2019
@chunhtai
Copy link
Contributor

chunhtai commented May 3, 2019

#31978 merged

@shihaohong shihaohong changed the title (WIP) Fix Exception on Nested TabBar/TabBarView disposal Fix Exception on Nested TabBar/TabBarView disposal May 3, 2019
@shihaohong shihaohong changed the title Fix Exception on Nested TabBar/TabBarView disposal Fix Exception on Nested TabBarView disposal May 3, 2019
@shihaohong shihaohong requested a review from HansMuller May 3, 2019 18:48
Copy link
Contributor

@HansMuller HansMuller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is definitely a workaround. Hopefully we'll be able to replace it with fixes for #32054, #32056.


@override
void dispose() {
// Sets `pixels` to a non-null value before `ScrollPosition.dispose` is
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add a TODO and link to the issue and note once the issue is fix we can remove this workaround

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

@shihaohong shihaohong merged commit 2f75005 into flutter:master May 4, 2019
@shihaohong shihaohong deleted the nested-tabbars branch May 4, 2019 16:14
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 6, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
c: crash Stack traces logged to the console f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Exception while scrolling through TabBar navigation
4 participants