Make disposing a ScrollPosition with pixels=null legal by goderbauer · Pull Request #44617 · flutter/flutter

goderbauer · 2019-11-11T23:12:46Z

Description

Certain subclass of ScrollPosition (namely _TabBarScrollPosition and _PagePosition) are initialized with initialPixels = null because the initial scroll offset cannot be determined when the ScrollPosition is first constructed. To determine the scroll offset for these subclasses, the viewport dimensions need to be known. Therefore, determining the initial pixels value is deferred until layout when applyViewportDimension is called.

This creates a problem when such a ScrollPosition is disposed without ever doing a layout run: Prior to this change, ScrollPosition.dispose used to assert(pixels != null), which would fire for the aforementioned ScrollPosition if their associated Viewport has never been layed out.

There's a common scenario for this situation: A (scrollable) TabBar or PageView is part of an OverlayEntry that's covered by another opaque OverlayEntry. The covered OverlayEntry is never layed out because its not visible on screen (it is considered to be offstage). When the covering OverlayEntry is removed, the other OverlayEntry is moved in the RenderTree from its offstage to the onstage position. Since the content of the moved OverlayEntry is associated with a global key the containing Scrollable is deactivated at the old position and re-activated at the new position in the RenderTree. This reactivation at a new position in the tree causes didUpdateDependencies to be called on the State of Scrollable, which calls ScrollableState._updatePosition [1]. This method builds a new ScrollPosition based on ScrollConfiguration and ScrollPhysics inherited from the new position in the RenderTree. The old ScrollPosition is disposed (which used to throw since it still has pixels set to null, see above).

This change makes it legal to dispose a ScrollPosition that has pixels still set to null.

Alternatives considered:

Just initialize the subclasses with a non-null initial pixels value. This seems less then ideal as this would indicate that the ScrollPosition is valid when in fact it is not until first layout.
Defer creation/recreation of ScrollPosition until layout when initial value for pixels is known. This adds a lot of complexity and also breaks the current contract of the Scrollable API, which currently makes the ScrollPosition available before layout.

Side node: For the _PagePosition a work-around for this problem had been implemented in #31581. This change makes the work-around unnecessary and its therefore removed.

[1]

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

Lines 314 to 333 in 35de8d5

    
           void _updatePosition() { 
        
             _configuration = ScrollConfiguration.of(context); 
        
             _physics = _configuration.getScrollPhysics(context); 
        
             if (widget.physics != null) 
        
               _physics = widget.physics.applyTo(_physics); 
        
             final ScrollController controller = widget.controller; 
        
             final ScrollPosition oldPosition = position; 
        
             if (oldPosition != null) { 
        
               controller?.detach(oldPosition); 
        
               // It's important that we not dispose the old position until after the 
        
               // viewport has had a chance to unregister its listeners from the old 
        
               // position. So, schedule a microtask to do it. 
        
               scheduleMicrotask(oldPosition.dispose); 
        
             } 
        
             _position = controller?.createScrollPosition(_physics, this, oldPosition) 
        
               ?? ScrollPositionWithSingleContext(physics: _physics, context: this, oldPosition: oldPosition); 
        
             assert(position != null); 
        
             controller?.attach(position); 
        
           }

Related Issues

Fixes #44269.

Tests

I added the following tests:

Can dispose ScrollPosition when pixels is null
Scrollable in hidden overlay does not crash when unhidden

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 · 2019-11-11T23:34:03Z

Unrelated to the PR, you will probably need to merge with master to pass the failing macos tests. I think there was some changes to the Cirrus macos images.

goderbauer · 2019-11-11T23:37:13Z

Thanks for the heads-up. Merged with master.

shihaohong · 2019-11-12T01:00:23Z

The rationale looks good to me. I think that we would probably have to remove the regression test for the workaround I added. That test relies on the fact that the assert(pixels != null) would throw, but with this update, this would no longer be the case.

The underlying issue that the tab that was warped over would still be built would still be a problem, but there are already issues to track that situation.

goderbauer · 2019-11-12T01:37:27Z

Why do we have to remove the test? It should still pass even with this change?

shihaohong · 2019-11-12T01:48:18Z

It’s more so that the test will never fail now, since it used to fail on that assert without my workaround. However, now that the assert is removed, the test no longer serves that purpose

goderbauer · 2019-11-12T17:29:54Z

The test ensures that we never add that assert back, so it still has a purpose :)

After syncing offline we decided to rename it to better reflect its purpose.

shihaohong

LGTM!

shihaohong · 2019-11-12T18:42:17Z

packages/flutter/test/widgets/scrollable_in_overlay_test.dart

nit:

final OverlayEntry entry2 = OverlayEntry( maintainState: true, opaque: true, builder: (BuildContext context) { return const Text('number2'); }, );

fluttergithubbot added the framework flutter/packages/flutter repository. See also f: labels. label Nov 11, 2019

googlebot added the cla: yes label Nov 11, 2019

goderbauer requested a review from shihaohong November 11, 2019 23:13

goderbauer force-pushed the removeAssert branch from 1eee71d to edf2cb8 Compare November 11, 2019 23:35

shihaohong approved these changes Nov 12, 2019

View reviewed changes

goderbauer force-pushed the removeAssert branch from c669cdb to 2ec01d6 Compare November 12, 2019 18:49

goderbauer added the waiting for tree to go green label Nov 12, 2019

goderbauer added 3 commits November 12, 2019 12:51

Make disposing a ScrollPosition with pixels=null legal

f4f341a

rename test

b4a5bd9

fix nit

30370bf

goderbauer force-pushed the removeAssert branch from 2ec01d6 to 30370bf Compare November 12, 2019 20:51

goderbauer merged commit bcc93bc into flutter:master Nov 13, 2019

goderbauer deleted the removeAssert branch November 13, 2019 19:31

github-actions bot locked as resolved and limited conversation to collaborators Aug 3, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make disposing a ScrollPosition with pixels=null legal#44617

Make disposing a ScrollPosition with pixels=null legal#44617
goderbauer merged 3 commits intoflutter:masterfrom
goderbauer:removeAssert

goderbauer commented Nov 11, 2019

Uh oh!

shihaohong commented Nov 11, 2019

Uh oh!

goderbauer commented Nov 11, 2019

Uh oh!

shihaohong commented Nov 12, 2019 •

edited

Loading

Uh oh!

goderbauer commented Nov 12, 2019

Uh oh!

shihaohong commented Nov 12, 2019 •

edited

Loading

Uh oh!

goderbauer commented Nov 12, 2019

Uh oh!

shihaohong left a comment

Uh oh!

shihaohong Nov 12, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	void _updatePosition() {
	_configuration = ScrollConfiguration.of(context);
	_physics = _configuration.getScrollPhysics(context);
	if (widget.physics != null)
	_physics = widget.physics.applyTo(_physics);
	final ScrollController controller = widget.controller;
	final ScrollPosition oldPosition = position;
	if (oldPosition != null) {
	controller?.detach(oldPosition);
	// It's important that we not dispose the old position until after the
	// viewport has had a chance to unregister its listeners from the old
	// position. So, schedule a microtask to do it.
	scheduleMicrotask(oldPosition.dispose);
	}

	_position = controller?.createScrollPosition(_physics, this, oldPosition)
	?? ScrollPositionWithSingleContext(physics: _physics, context: this, oldPosition: oldPosition);
	assert(position != null);
	controller?.attach(position);
	}

Conversation

goderbauer commented Nov 11, 2019

Description

Related Issues

Tests

Checklist

Breaking Change

Uh oh!

shihaohong commented Nov 11, 2019

Uh oh!

goderbauer commented Nov 11, 2019

Uh oh!

shihaohong commented Nov 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

goderbauer commented Nov 12, 2019

Uh oh!

shihaohong commented Nov 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

goderbauer commented Nov 12, 2019

Uh oh!

shihaohong left a comment

Choose a reason for hiding this comment

Uh oh!

shihaohong Nov 12, 2019

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

shihaohong commented Nov 12, 2019 •

edited

Loading

shihaohong commented Nov 12, 2019 •

edited

Loading