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
Improve the behavior of Scrollable.ensureVisible when Scrollable nested #65226
Conversation
@Hixie Please review :) |
/// 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. | ||
/// |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why wouldn't you just specify the targetRenderObject
as the object
parameter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why can't we just traverse up the viewports and get the offset for all of them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
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. |
You now have some merge conflicts - can you merge again? |
Done : ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This pull request is not suitable for automatic merging in its current state.
|
@dnfield I'm sorry I broke the internal test case, please help me investigate at your convenient, thank you very much. |
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. |
It looks like this is blocked by the NNDB migration. |
So how can I fix it or just wait for googler to fix it? |
I'm pretty sure this is ok now. Going to land it. Will revert if necessary. |
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. |
… Scrollable nested (flutter#65226)" (flutter#66918)" This reverts commit e8812c4.
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 thetargetRenderObject
invisible.Related Issues
Fixes #65100
Tests
I added the following tests:
See files.