PageView resize from zero-size viewport should not lose state #65015

xu-baolin · 2020-09-01T12:23:57Z

Description

PageView resize from zero-size viewport should not lose state, otherwise will cause TabBar and TabBarView mismatch, see #64718

In _PagePosition.applyViewportDimension, If the viewport has a zero-size, the page can not be got by getPageFromPixels, so we need to cache the page for use when resizing the viewport to non-zero next time.

Related Issues

I have filed a new issue to reproduce this bug.
Fixes #88956

The following issues behaviors can not reproduce because the windows platform change some behaviors.
Fixes #64718
Fixes #55262

Tests

I added the following tests:

See files.

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 read the Tree Hygiene wiki page, which explains my responsibilities.
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

Did any tests fail when you ran them? Please read Handling breaking changes.

No, no existing tests failed, so this is not a breaking change.
Yes, this is a breaking change. If not, delete the remainder of this section.
- I wrote a design doc: https://flutter.dev/go/template Replace this with a link to your design doc's short link
- I got input from the developer relations team, specifically from: Replace with the names of who gave advice
- I wrote a migration guide: https://flutter.dev/go/breaking-changes-template Replace this with a link to a pull request that adds the migration guide to https://flutter.dev/docs/release/breaking-changes

xu-baolin · 2020-09-01T12:34:16Z

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

-      return newPosition.maxScrollExtent + oldDelta;
+    // If resize from zero, recover position from cache, so don't try to
+    // maintain the relative overscroll.
+    if (oldPosition.minScrollExtent != 0.0 || oldPosition.maxScrollExtent != 0.0) {


@Hixie Hi, Please review here, in this case, the _PagePosition.applyViewportDimension will correct the pixels due to resizing from zero, (such as oldPosithon here is (pixels: 800.0, minScrollExtent : 0.0, maxScrollExtent: 0.0)) and it will cause RangeMaintainingScrollPhysics.adjustPositionForNewDimensions mistake maintain the relative overscroll.
In addition, if the #63146 reland, we can remove this check and it works well.

I'll try to land the old PR again to make this moot.

(but with && -> ||)

xu-baolin · 2020-09-01T12:41:33Z

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

+    // Cache the page when resize the viewport to zero.
+    // For use when resize the viewport to non-zero next time.
+    _cachePage = (viewportDimension == 0.0) ? page : null;
+
    if (newPixels != oldPixels) {


Maybe here need to use nearEqual, I am not sure.

xu-baolin · 2020-09-01T12:45:41Z

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

    final double newPixels = getPixelsFromPage(page);

+    // Cache the page when resize the viewport to zero.
+    // For use when resize the viewport to non-zero next time.
+    _cachePage = (viewportDimension == 0.0) ? page : null;


In another extreme case, assuming that the viewport is very small, only 1 pixel, I ignore the processing of this kind of scene.

Hixie · 2020-09-02T18:32:24Z

This seems reasonable, though I get this niggling feeling that it really means our whole design here is wrong, and that we should be deriving the pixel position from the page and not the other way around.

Let's wait until the RangeMaintainingScrollPhysics fix relands before landing this though, just to keep things simple...

Hixie · 2020-09-02T23:48:45Z

#65135 is the new fix for RangeMaintainingScrollPhysics.

xu-baolin · 2020-09-08T00:51:02Z

This seems reasonable, though I get this niggling feeling that it really means our whole design here is wrong, and that we should be deriving the pixel position from the page and not the other way around.

Let's wait until the RangeMaintainingScrollPhysics fix relands before landing this though, just to keep things simple...

So you suggest that we should record the page in real-time, as long as the pixels change, and deprecate getPageFromPixels, right?

Hixie · 2020-09-08T19:03:57Z

I'm imagining a world where the "pixels" getter returns a value derived from the "page" field. But I don't know if the design even makes that possible. You definitely don't need to try to implement that if you don't want to. It might be somewhat involved.

Hixie · 2020-09-08T19:04:24Z

BTW the RangeMaintainingScrollPhysics has landed again, hopefully it sticks this time...

xu-baolin · 2020-09-10T13:33:50Z

I'm imagining a world where the "pixels" getter returns a value derived from the "page" field. But I don't know if the design even makes that possible. You definitely don't need to try to implement that if you don't want to. It might be somewhat involved.

How about open a new topic to discuss how to refactor it？I totally agree that this will be a bit complicated :)
The PR looks very strong this time!

xu-baolin · 2020-09-28T02:54:01Z

@Hixie Hi,
Review requested by manually : )

iskakaushik · 2020-09-30T22:35:34Z

cc: @iskakaushik for looking at pausing the animator on 0 size window update.

xu-baolin · 2020-10-12T12:34:28Z

@dnfield Hi,
Would you like to take a look at this? Thanks :)

dnfield · 2020-10-12T16:40:09Z

@iskakaushik - are you saying that if we paused the animator in this case (which it seems like we should), we'd avoid this bug?

I'm not clear on how the viewport metrics are getting updated on Windows in the minimize case. It seems like we should be resolving that before doing anything else with this bug. IOW, it seems to me that getting a zero sized viewport shouldn't be happening when you minimize the window - but the animator should be paused.

iskakaushik · 2020-10-12T16:47:37Z

@dnfield Yup. I will try to take a look at this sometime this week. IMO app minimization should be treated as application resigning from being active, we should re-use the last drawn frame and show it when the app becomes active again. I need to look at what macOS / Windows provide for us to achieve this behavior. I think the fix will be in the engine and not require us to handle 0 size window metrics events.

dnfield · 2020-10-12T16:59:12Z

When we get the WM_SIZE message we should be checking whether the wparam is SIZE_MINIMIZED https://docs.microsoft.com/en-us/windows/win32/winmsg/wm-size?redirectedfrom=MSDN for Windows

xu-baolin · 2020-11-18T05:38:28Z

Actually, this is not a platform-related patch, setting the size to 0 will also trigger this bug in all the platforms.

Piinks · 2021-08-17T21:33:19Z

@xu-baolin, can you update this PR if you would like to proceed with the change? I'd be happy to take a look if you would still like to land it. Thank you!

xu-baolin · 2021-08-17T23:04:10Z

@xu-baolin, can you update this PR if you would like to proceed with the change? I'd be happy to take a look if you would still like to land it. Thank you!

OK

xu-baolin · 2021-08-26T13:20:43Z

@xu-baolin, can you update this PR if you would like to proceed with the change? I'd be happy to take a look if you would still like to land it. Thank you!

Hi, I have updated this and it is ready for review now. Thanks!

Piinks

I think this is a good change. It is not addressing a platform specific bug as mentioned above, and it is very easy to reproduce this bug just by resizing the window on Mac to be very small. Thank you for another contribution @xu-baolin, and for updating it after so much time. :)

Piinks · 2021-08-30T23:21:51Z

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

+    // Cache the page when resize the viewport to zero.
+    // For use when resizing the viewport to non-zero next time.


Suggested change

// Cache the page when resize the viewport to zero.

// For use when resizing the viewport to non-zero next time.

// If the viewportDimension is zero, cache the page

// in case the viewport is resized to be non-zero.

Piinks · 2021-08-30T23:22:18Z

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

@@ -325,6 +325,10 @@ class _PagePosition extends ScrollPositionWithSingleContext implements PageMetri

  final int initialPage;
  double _pageToUseOnStartup;
+  // When the viewport has a zero-size, the `page` can not
+  // be got by `getPageFromPixels`, so we need to cache the page


Suggested change

// be got by `getPageFromPixels`, so we need to cache the page

// be retrieved by `getPageFromPixels`, so we need to cache the page

xu-baolin · 2021-08-31T11:31:18Z

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

@@ -370,7 +384,8 @@ class _PagePosition extends ScrollPositionWithSingleContext implements PageMetri
  double get _initialPageOffset => math.max(0, viewportDimension * (viewportFraction - 1) / 2);

  double getPageFromPixels(double pixels, double viewportDimension) {
-    final double actual = math.max(0.0, pixels - _initialPageOffset) / math.max(1.0, viewportDimension * viewportFraction);
+    assert(viewportDimension > 0.0);
+    final double actual = math.max(0.0, pixels - _initialPageOffset) / (viewportDimension * viewportFraction);


math.max(1.0, viewportDimension * viewportFraction) -> (viewportDimension * viewportFraction)

This fixes the issue when the viewport is less than 1.0 although it is not common.

xu-baolin · 2021-08-31T11:33:28Z

@Piinks hi, I have added more changes to make sure that all work well when the viewport's size is zero.

Now change the page through the controller or TabBar will not lose state when the viewport size is zero.

Piinks

Thanks for the updates @xu-baolin! LGTM

flutter-dashboard bot added the framework flutter/packages/flutter repository. See also f: labels. label Sep 1, 2020

googlebot added the cla: yes label Sep 1, 2020

xu-baolin commented Sep 1, 2020

View reviewed changes

xu-baolin mentioned this pull request Sep 1, 2020

Fix RangeMaintainingScrollPhysics #63146

Merged

xu-baolin commented Sep 1, 2020

View reviewed changes

Piinks added a: desktop Running on desktop a: state restoration RestorationManager and related APIs f: material design flutter/packages/flutter/material repository. f: scrolling Viewports, list views, slivers, etc. labels Sep 2, 2020

hamdikahloun mentioned this pull request Sep 6, 2020

PageView Builde error resize #65310

Closed

xu-baolin requested a review from Hixie September 16, 2020 09:47

iskakaushik mentioned this pull request Oct 12, 2020

☂️ On Windows minimizing the app window will modify the contents of the frame #65061

Closed

4 tasks

gspencergoog requested a review from iskakaushik November 11, 2020 23:34

xu-baolin removed the a: desktop Running on desktop label Jan 6, 2021

fix a pageview bug

d4eee76

xu-baolin force-pushed the resizepageview branch from d984de4 to d4eee76 Compare August 26, 2021 13:14

Merge branch 'master' of github.com:flutter/flutter into 00825

526e84e

Piinks approved these changes Aug 30, 2021

View reviewed changes

++

6605e47

xu-baolin commented Aug 31, 2021

View reviewed changes

xu-baolin requested a review from Piinks August 31, 2021 11:33

Merge branch 'master' of github.com:flutter/flutter into 00825

c9afb17

Piinks approved these changes Sep 9, 2021

View reviewed changes

Piinks added the waiting for tree to go green label Sep 9, 2021

fluttergithubbot merged commit ba734ed into flutter:master Sep 10, 2021

bleroux mentioned this pull request May 23, 2022

Fix PageView throws Null check error #104405

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PageView resize from zero-size viewport should not lose state #65015

PageView resize from zero-size viewport should not lose state #65015

xu-baolin commented Sep 1, 2020 •

edited

xu-baolin Sep 1, 2020 •

edited

Hixie Sep 2, 2020

Hixie Sep 2, 2020

xu-baolin Sep 1, 2020

xu-baolin Sep 1, 2020

Hixie commented Sep 2, 2020

Hixie commented Sep 2, 2020

xu-baolin commented Sep 8, 2020

Hixie commented Sep 8, 2020

Hixie commented Sep 8, 2020

xu-baolin commented Sep 10, 2020

xu-baolin commented Sep 28, 2020

iskakaushik commented Sep 30, 2020

xu-baolin commented Oct 12, 2020

dnfield commented Oct 12, 2020

iskakaushik commented Oct 12, 2020

dnfield commented Oct 12, 2020

xu-baolin commented Nov 18, 2020

Piinks commented Aug 17, 2021

xu-baolin commented Aug 17, 2021

xu-baolin commented Aug 26, 2021

Piinks left a comment

Piinks Aug 30, 2021

Piinks Aug 30, 2021

xu-baolin Aug 31, 2021

xu-baolin commented Aug 31, 2021 •

edited

Piinks left a comment

		// Cache the page when resize the viewport to zero.
		// For use when resizing the viewport to non-zero next time.

	// be got by `getPageFromPixels`, so we need to cache the page
	// be retrieved by `getPageFromPixels`, so we need to cache the page

PageView resize from zero-size viewport should not lose state #65015

PageView resize from zero-size viewport should not lose state #65015

Conversation

xu-baolin commented Sep 1, 2020 • edited

Description

Related Issues

Tests

Checklist

Breaking Change

xu-baolin Sep 1, 2020 • edited

Choose a reason for hiding this comment

Hixie Sep 2, 2020

Choose a reason for hiding this comment

Hixie Sep 2, 2020

Choose a reason for hiding this comment

xu-baolin Sep 1, 2020

Choose a reason for hiding this comment

xu-baolin Sep 1, 2020

Choose a reason for hiding this comment

Hixie commented Sep 2, 2020

Hixie commented Sep 2, 2020

xu-baolin commented Sep 8, 2020

Hixie commented Sep 8, 2020

Hixie commented Sep 8, 2020

xu-baolin commented Sep 10, 2020

xu-baolin commented Sep 28, 2020

iskakaushik commented Sep 30, 2020

xu-baolin commented Oct 12, 2020

dnfield commented Oct 12, 2020

iskakaushik commented Oct 12, 2020

dnfield commented Oct 12, 2020

xu-baolin commented Nov 18, 2020

Piinks commented Aug 17, 2021

xu-baolin commented Aug 17, 2021

xu-baolin commented Aug 26, 2021

Piinks left a comment

Choose a reason for hiding this comment

Piinks Aug 30, 2021

Choose a reason for hiding this comment

Piinks Aug 30, 2021

Choose a reason for hiding this comment

xu-baolin Aug 31, 2021

Choose a reason for hiding this comment

xu-baolin commented Aug 31, 2021 • edited

Piinks left a comment

Choose a reason for hiding this comment

xu-baolin commented Sep 1, 2020 •

edited

xu-baolin Sep 1, 2020 •

edited

xu-baolin commented Aug 31, 2021 •

edited