Fix RangeMaintainingScrollPhysics #63146
Conversation
flutter-dashboard
bot
added
framework
There was a problem hiding this comment.
I like it! This is what I was trying to do but it is a much simpler way of tracking it.
|
As it stands this breaks a bunch of tests. :-/ Investigating... |
Hixie
force-pushed
the
scroll
branch
2 times, most recently
from
055dc79 to
ca024e5
Compare
|
Well it passes the tests now, at least. |
|
@Hixie I've stared testing with this patch, and so far so good. Thanks! |
We definitely shouldn't just add epsilon, but maybe we could use |
There was a problem hiding this comment.
Maybe we should update the docs on RangeMaintainingScrollPhysics indicate that there are more cases where the range is not maintained.
It now uses the scroll metrics as they stood at the end of the last frame. It previously used a weird combination of the old extents and the newish position, which led to some weird effects when the position had been changed in expectation of a viewport or content dimension change.
|
Added more documentation. Will land on green unless I hear otherwise. Thanks for the reviews! |
|
Since this is a 1.20 cherrypick candidate, I landed on green. |
|
Fix for #60288 |
| _pendingDimensions = false; | ||
| } | ||
| assert(!_didChangeViewportDimensionOrReceiveCorrection, 'Use correctForNewDimensions() (and return true) to change the scroll offset during applyContentDimensions().'); | ||
| _lastMetrics = copyWith(); |
There was a problem hiding this comment.
Nice fixed!
We can not use the oldPosition above, for example, the _PagePosition.applyViewportDimension will correct the pixels due to resizing from zero, and it will cause RangeMaintainingScrollPhysics.adjustPositionForNewDimensions mistake. I am currently fix this bug (#65015 ).
There was a problem hiding this comment.
I'll leave this as-is in my reland and will let you follow up in #65015 (i'll review that shortly, sorry i missed that until now)
| } | ||
| } | ||
| if ((oldPosition.pixels < oldPosition.minScrollExtent) && | ||
| (oldPosition.pixels > oldPosition.maxScrollExtent)) { |
There was a problem hiding this comment.
This should be
if ((oldPosition.pixels < oldPosition.minScrollExtent) ||
(oldPosition.pixels > oldPosition.maxScrollExtent))
right?
There was a problem hiding this comment.
oh wow, how did that not get caught by the tests, yikes
There was a problem hiding this comment.
fixing it doesn't cause any test failures even! what on earth.
| /// | ||
| /// The second is to enforce the boundary when the position is in range. | ||
| /// | ||
| /// If the current velocity is zero, neither adjustment is made. The |
There was a problem hiding this comment.
If the current velocity is non-zero
There was a problem hiding this comment.
oops. will fix in the reland.
It now uses the scroll metrics as they stood at the end of the last frame. It previously used a weird combination of the old extents and the newish position, which led to some weird effects when the position had been changed in expectation of a viewport or content dimension change.
It now uses the scroll metrics as they stood at the end of the last frame. It previously used a weird combination of the old extents and the newish position, which led to some weird effects when the position had been changed in expectation of a viewport or content dimension change.
6 participants
It now uses the scroll metrics as they stood at the end of the last frame.
It previously used a weird combination of the old extents and the newish position, which led to some weird effects when the position had been changed in expectation of a viewport or content dimension change.