Let scrollOffsetCorrection on small lists not leave the list stuck in overscroll #17599
Conversation
| @@ -252,11 +252,13 @@ abstract class ScrollPosition extends ViewportOffset with ScrollMetrics { | |||
| /// see the discussion there for why that might still be a bad idea.) | |||
| void correctPixels(double value) { | |||
| _pixels = value; | |||
| _didChangeViewportDimensionOrPixels = true; | |||
There was a problem hiding this comment.
The documentation says "In particular, this is typically called as a response to applyViewportDimension or applyContentDimensions", but here you're writing code that assumes it's called before applyContentDimensions, which seems dubious.
There was a problem hiding this comment.
Ya, I'm a bit concerned touching something this central since there's a lot of implicit coupling, though all the tests passed.
I wasn't too sure what's the best way either. The correct behaviour seems to work with long lists because applyNewDimensions is called on line 426 to re-kick the start ballistic activity. Perhaps you have a better solution.
This field seemed like the least invasive as far as I can tell since the current code which uses the field instead of the setter seems to want to defer signaling after this loop here https://github.com/flutter/flutter/blob/master/packages/flutter/lib/src/rendering/viewport.dart#L1053. I'm assuming the documentation saying it's used in response to applyContentDimensions likely meant applyContentDimensions resets this value to false after consuming it.
There was a problem hiding this comment.
PTAL, did what we talked about offline and added some docs and an assert.
| @@ -84,11 +84,6 @@ class ScrollPositionWithSingleContext extends ScrollPosition implements ScrollAc | |||
| return super.setPixels(newPixels); | |||
| } | |||
|
|
|||
| @override | |||
| void correctBy(double correction) { | |||
There was a problem hiding this comment.
I left the correctPixels in the super alone like we talked about but this is an actual bug since it circumvents its super.correctBy and doesn't actually do anything different.
| /// | ||
| /// See also: | ||
| /// | ||
| /// * The method [correctBy], which is a method of [ViewportOffset] be used |
| /// See also: | ||
| /// | ||
| /// * The method [correctBy], which is a method of [ViewportOffset] be used | ||
| /// by viewport render objects to also correct the offset during layout |
| /// See also: | ||
| /// | ||
| /// * [jumpTo], for also changing the scroll position but applying the | ||
| /// change immediately and notifying its listeners. |
There was a problem hiding this comment.
mention that jumpTo is for when you're not in layout.
| /// | ||
| /// See also: | ||
| /// | ||
| /// * [correctBy], a more specialized way of changing the current offset |
There was a problem hiding this comment.
-more specialized (otherwise it makes it sound like jumpTo is also for use during layout)
| void correctPixels(double value) { | ||
| _pixels = value; | ||
| } | ||
|
|
||
| // See also [correctPixels] which is used by the [ScrollPosition] itself to | ||
| // set the offset initially during construction or after | ||
| // [applyViewportDimension] or [applyContentDimensions] is called. |
There was a problem hiding this comment.
Ah, I didn't realize dartdoc'ing overrides actually works. Done
There was a problem hiding this comment.
oh we should also include the docs from the superclass, i didn't realize it was doing that. (These docs will replace the superclass docs.)
| void correctPixels(double value) { | ||
| _pixels = value; | ||
| } | ||
|
|
||
| // See also [correctPixels] which is used by the [ScrollPosition] itself to |
|
|
Fixes #17474
The issue is if offset corrections don't cause the viewport content's scroll extent to change, lists in overscroll can be stuck.