It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Why add a method to scroll position? That will definitely fail some tests somewhere because we have customers that extend and implement scroll position.

Developers with subclasses of ScrollPosition that override jumpTo, expecting it to be called when the scrollbar thumb is dragged, will break. I haven't found an example of that yet, but I'd be surprised even there isn't one somewhere.

FTR this PR doesn't break any existing tests, including internal tests (see Google internal link cl/534230932).

I added the ScrollPosition.dragTo() method to avoid unconditionally firing start/end scroll notifications when the scrollbar thumb is dragged to update the scroll position. The code used to call ScrollPosition.jumpTo(), which unconditionally produces a start/update/end notification each time the thumb moved.

The resulting notifications - a start when the thumb-drag starts, updates as the thumb is dragged, and an end when the thumb drag ends - match what happens when the scroll position is driven by a touch-drag gesture.

Drag is implemented internally from scroll position in conjunction with Scrollable, so what does it mean to dragTo in the context of the actual drag implementation?

I'm not quite sure what you're getting at. ScrollPosition.dragTo is the same as ScrollPosition.jumpTo without the activity transitions and extra notifications. Dragging the scrollbar's thumb used to call jumpTo, now it calls dragTo. We could call it updateTo instead of dragTo. I suppose we could even redefine jumpTo():

  @override
  void jumpTo(double value) {
    goIdle();
    if (pixels != value) {
      didStartScroll();
      updateTo(value);
      didEndScroll();
    }
    goBallistic(0.0);
  }

Also, it does not look like it makes sense to add that method on scroll position if in fact the scrollbar is going to manually dispatch notifications. I think sending out a start and end notification is a bad idea, because the scrollbar itself is listening to those notifications. So the scrollbar will react to the notifications it is sending, as well as natural notifications from the scroll position.

There may be a better way to send the start and end scroll notifications then having the scrollbar do so directly. On the other hand, only the scrollbar (thumb) knows when the drag gesture ends.

Why not just modify the isScrollingNotifier? I thought that was what was needed here and was the basis of the recommended fix. It is not actually scrolling, so I think keeping this minimal for the specific use case is important.

If scrollbar just modifies isScrollingNotifier's value, the notification listeners see a series of start/update/end notifications, albeit with isScrolling always true. That doesn't match what happens during a touch-drag gesture.

Spoke with @HansMuller and he said this is back on his radar, so leaving open.

Spoke with @HansMuller and he said this is still on his radar, so leaving open.

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact "@test-exemption-reviewer" in the #hackers channel in Chat (don't just cc them here, they won't see it! Use Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Closing this PR in favor of #146654.