Improve the behavior of Scrollable.ensureVisible when Scrollable nested by xu-baolin · Pull Request #65226 · flutter/flutter

xu-baolin · 2020-09-04T04:47:19Z

Description

If there are multiple scrollable widgets nested, we should let the inner targetRenderObject as visible as possible to improve the user experience. Otherwise, let the outer renderObject as visible as possible maybe cause the targetRenderObject invisible.

Related Issues

Fixes #65100

Tests

I added the following tests:

See files.

xu-baolin · 2020-09-07T12:36:23Z

@Hixie Please review :)

dnfield · 2020-09-08T15:51:08Z

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

+  /// The optional `targetRenderObject` parameter is used to determine which area
+  /// of that object should be as visible as possible. If `targetRenderObject` is null,
+  /// the entire [RenderObject] (as defined by its [RenderObject.paintBounds])
+  /// will be as visible as possible. If `targetRenderObject` is provided it should be
+  /// a descendant of the object.
+  ///


Why wouldn't you just specify the targetRenderObject as the object parameter?

@dnfield Hi,
If we only pass in the inner target RO, the outer viewport cannot be obtained by RenderAbstractViewport.of(object) at line 648 below, and we cannot get the offset to reveal the target for the outer viewport.

Why can't we just traverse up the viewports and get the offset for all of them?

@dnfield
Yes, we traverse up viewport by the superior calling,

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

Lines 320 to 328 in 78fdd52

while (scrollable != null) {

futures.add(scrollable.position.ensureVisible(

context.findRenderObject(),

alignment: alignment,

duration: duration,

curve: curve,

alignmentPolicy: alignmentPolicy,

targetRenderObject: targetRenderObject,

));

Here just a correction to the area to be displayed.

Ohhhh I see now, we're using this parameter to memoize which object we were initially trying to center and let the viewports know that's the one - we don't expect callers to actually pass this in as a duplicate of object. Is that correct?

@dnfield
You are absolutely correct.
Actually, if the caller does this, it has the same effect as setting it to null, which means the target area is itself.

dnfield · 2020-09-14T17:59:47Z

Can you merge this branch up to HEAD and push a new commit? I think the CI should be a flake that was resolved in master.

xu-baolin · 2020-09-15T03:54:53Z

Can you merge this branch up to HEAD and push a new commit? I think the CI should be a flake that was resolved in master.

Done.

dnfield · 2020-09-15T19:14:48Z

You now have some merge conflicts - can you merge again?

xu-baolin · 2020-09-16T01:31:27Z

You now have some merge conflicts - can you merge again?

Done : )

dnfield

LGTM

fluttergithubbot · 2020-09-16T02:27:04Z

This pull request is not suitable for automatic merging in its current state.

The status or check suite Google testing has failed. Please fix the issues identified (or deflake) before re-applying this label.

xu-baolin · 2020-09-16T02:33:33Z

@dnfield I'm sorry I broke the internal test case, please help me investigate at your convenient, thank you very much.
If possible, you can also give me the test case, I will investigate the reason.

dnfield · 2020-09-18T03:19:15Z

My understanding is that this won't have to change to get landed, but it may take a few more days before everything is resolved internally and I don't want to add to confusion.

Piinks · 2020-09-22T19:55:54Z

It looks like this is blocked by the NNDB migration.

xu-baolin · 2020-09-28T03:01:20Z

It looks like this is blocked by the NNDB migration.

So how can I fix it or just wait for googler to fix it?
Thanks.

dnfield · 2020-09-28T03:07:08Z

I'm pretty sure this is ok now. Going to land it. Will revert if necessary.

dnfield · 2020-09-29T17:38:01Z

This is breaking a google3 test. I'll work on getting a reproducible test case for it, but we have to rever tfor now.

…ble nested (#65226)" This reverts commit 73b6398.

…ble nested (#65226)" (#66918) This reverts commit 73b6398.

xu-baolin · 2020-09-30T01:37:40Z

This is breaking a google3 test. I'll work on getting a reproducible test case for it, but we have to rever tfor now.

I am sorry for breaking something.
Please let me known if you reproduce an external test case and I will investigate it.
Thanks :)

… Scrollable nested (flutter#65226)" (flutter#66918)" This reverts commit e8812c4.

* Revert "Revert "Improve the behavior of Scrollable.ensureVisible when Scrollable nested (#65226)" (#66918)" This reverts commit e8812c4. * Fix for page views * comment

xu-baolin added 3 commits September 3, 2020 19:25

improve Scrollable.ensureVisible

49bc50c

improve Scrollable.ensureVisible

01e4ddf

add test case

61fd9f0

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

googlebot added the cla: yes label Sep 4, 2020

xu-baolin added 2 commits September 4, 2020 13:41

add test case

fda8d8b

code style

78fdd52

dnfield reviewed Sep 8, 2020

View reviewed changes

xu-baolin requested a review from dnfield September 9, 2020 03:09

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

2b9084f

Conflict resolution

b6bcd45

dnfield approved these changes Sep 16, 2020

View reviewed changes

dnfield added the waiting for tree to go green label Sep 16, 2020

fluttergithubbot removed the waiting for tree to go green label Sep 16, 2020

Piinks added the f: scrolling Viewports, list views, slivers, etc. label Sep 22, 2020

dnfield merged commit 73b6398 into flutter:master Sep 28, 2020

dnfield added a commit that referenced this pull request Sep 29, 2020

Revert "Improve the behavior of Scrollable.ensureVisible when Scrolla…

042e1fa

…ble nested (#65226)" This reverts commit 73b6398.

dnfield mentioned this pull request Sep 29, 2020

Revert "Improve the behavior of Scrollable.ensureVisible when Scrollable nested" #66918

Merged

dnfield added a commit that referenced this pull request Sep 29, 2020

Revert "Improve the behavior of Scrollable.ensureVisible when Scrolla…

e8812c4

…ble nested (#65226)" (#66918) This reverts commit 73b6398.

dnfield mentioned this pull request Sep 30, 2020

test that ensureVisible does not change PageView pages #67021

Merged

dnfield added a commit to dnfield/flutter that referenced this pull request Oct 9, 2020

Revert "Revert "Improve the behavior of Scrollable.ensureVisible when…

e23fbc0

… Scrollable nested (flutter#65226)" (flutter#66918)" This reverts commit e8812c4.

dnfield mentioned this pull request Oct 9, 2020

Reland ensure visible fix for nested viewports #67773

Merged

	while (scrollable != null) {
	futures.add(scrollable.position.ensureVisible(
	context.findRenderObject(),
	alignment: alignment,
	duration: duration,
	curve: curve,
	alignmentPolicy: alignmentPolicy,
	targetRenderObject: targetRenderObject,
	));

Conversation

xu-baolin commented Sep 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Tests

Uh oh!

xu-baolin commented Sep 7, 2020

Uh oh!

dnfield Sep 8, 2020

Choose a reason for hiding this comment

Uh oh!

xu-baolin Sep 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnfield Sep 12, 2020

Choose a reason for hiding this comment

Uh oh!

xu-baolin Sep 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnfield Sep 14, 2020

Choose a reason for hiding this comment

Uh oh!

xu-baolin Sep 15, 2020

Choose a reason for hiding this comment

Uh oh!

dnfield commented Sep 14, 2020

Uh oh!

xu-baolin commented Sep 15, 2020

Uh oh!

dnfield commented Sep 15, 2020

Uh oh!

xu-baolin commented Sep 16, 2020

Uh oh!

dnfield left a comment

Choose a reason for hiding this comment

Uh oh!

fluttergithubbot commented Sep 16, 2020

Uh oh!

xu-baolin commented Sep 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dnfield commented Sep 18, 2020

Uh oh!

Piinks commented Sep 22, 2020

Uh oh!

xu-baolin commented Sep 28, 2020

Uh oh!

dnfield commented Sep 28, 2020

Uh oh!

dnfield commented Sep 29, 2020

Uh oh!

xu-baolin commented Sep 30, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

xu-baolin commented Sep 4, 2020 •

edited

Loading

xu-baolin Sep 9, 2020 •

edited

Loading

xu-baolin Sep 12, 2020 •

edited

Loading

xu-baolin commented Sep 16, 2020 •

edited

Loading