Reland ensure visible fix for nested viewports#67773
Merged
dnfield merged 3 commits intoflutter:masterfrom Oct 10, 2020
Merged
Conversation
… Scrollable nested (flutter#65226)" (flutter#66918)" This reverts commit e8812c4.
flutter-dashboard
bot
added
the
framework
gspencergoog
approved these changes
Oct 9, 2020
| /// 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 |
Contributor
There was a problem hiding this comment.
Suggested change
| /// will be as visible as possible. If `targetRenderObject` is provided it should be | |
| /// will be as visible as possible. If `targetRenderObject` is provided, it should be |
Also should be? Or must? It would be nice to enforce that.
gspencergoog
reviewed
Oct 9, 2020
|
|
||
| Rect? targetRect; | ||
| if (targetRenderObject != null && targetRenderObject != object) { | ||
| targetRect = MatrixUtils.transformRect( |
Contributor
There was a problem hiding this comment.
Is it possible to add an assert to make sure that targetRenderObject is a descendant?
Contributor
Author
There was a problem hiding this comment.
getTransformTo asserts it.
Contributor
|
This pull request is not suitable for automatic merging in its current state.
|
Contributor
Author
|
Internal failure appears to be a flake. Looking into it. |
Contributor
Author
|
Yup, flake. |
Member
|
Does |
Contributor
Author
|
Hmm. Probably. It might be ok because tabview uses pageview under the hood. Will have to check. |
This was referenced Apr 19, 2024
Closed
9 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This relands #65226, which was reverted in #66918 because it caused regressions in google3 for tests involving page views that used ensureVisible.
A test was added in #67021 which covers it.
The changes between this and the original change are in b840a8a, which does the following:
targetRenderObject == renderObjectensureVisiblein_PagePositionto discard thetargetRendreObject. As explained in the comment, this prevents further attempts to "center" the target render object, which could result in changing which page is used.@xu-baolin
Fixes #65100