Pinging back to the top of my inbox to check on the Google testing results.

Following up on Google testing, it looks like this breaks a couple of things in subtle ways that might be hard for developers to diagnose and fix. There is at least one custom scroll position class in the google testing set that looks to have caused some test failures. Separately there are some screen shot tests that show that the scroll position is not the same with this change.
Do you have thoughts on options to make this a non-breaking change?

@yiiim Hi! Are you able to address the comments above? (From triage)

Following up on Google testing, it looks like this breaks a couple of things in subtle ways that might be hard for developers to diagnose and fix. There is at least one custom scroll position class in the google testing set that looks to have caused some test failures. Separately there are some screen shot tests that show that the scroll position is not the same with this change. Do you have thoughts on options to make this a non-breaking change?

Thank you for your feedback. After carefully reviewing the implementation, I couldn't find an alternative approach to achieve the desired functionality without introducing a breaking change. If you have any suggestions or ideas, I would be happy to explore them further.

I have added maxLayoutCycles. Could you rerun the Google tests and try again?

Rebasing to rerun 👍

As I understand it, the while loop for applyContentDimensions is intended to correct the offset when minScrollExtent or maxScrollExtent changes. This is not related to whether the child is a Sliver. The way applyContentDimensions works requires it to be called repeatedly.

I may have missed something, so please correct me if i am wrong.

the while loop for applyContentDimensions is intended to correct the offset when minScrollExtent or maxScrollExtent changes

do you have an repro that will fail unless the applyContentDimensions has to be called several times even through the parameter in performLayout won't change? it feels to me there may be something else that need to be fixed instead of using a loop here.

If this is not done, _lastMetrics will be incorrect after scrolling.

Looking at the code, it seems we should update _lastMetrics (or part of it) when the scroll positiion change, not just in applyContentDimensions, right now the the code does a unnecessary round trip, ScrollPosition scrolled > notify SingleChildScrollView > mark layout dirty > layout> applyContentDimensions, just to update the lastMetrics (the layout shouldn't change in scrolling)

instead when ScrollPosition scrolled, it could just update whatever relevant metrics in _lastMetrics

Again, is there a repro that can demonstrate the issue

do you have an repro that will fail unless the applyContentDimensions has to be called several times even through the parameter in performLayout won't change? it feels to me there may be something else that need to be fixed instead of using a loop here.

You can reproduce this issue using the code from this comment.

To observe the difference, set _maxLayoutCycles in current branch to 1 and then to a value greater than 1.

Looking at the code, it seems we should update _lastMetrics (or part of it) when the scroll positiion change, not just in applyContentDimensions, right now the the code does a unnecessary round trip, ScrollPosition scrolled > notify SingleChildScrollView > mark layout dirty > layout> applyContentDimensions, just to update the lastMetrics (the layout shouldn't change in scrolling)

You are absolutely right. I will find a way to update _lastMetrics without triggering a new layout.

This is the code i ran

import 'package:flutter/material.dart';


void main() {
  runApp(const MyApp());
}

class MyApp extends StatelessWidget {
  const MyApp({Key? key}) : super(key: key);

  // This widget is the root of your application.
  @override
  Widget build(BuildContext context) {
    return MaterialApp(
      title: 'Flutter Demo',
      theme: ThemeData(
        // This is the theme of your application.
        //
        // Try running your application with "flutter run". You'll see the
        // application has a blue toolbar. Then, without quitting the app, try
        // changing the primarySwatch below to Colors.green and then invoke
        // "hot reload" (press "r" in the console where you ran "flutter run",
        // or simply save your changes to "hot reload" in a Flutter IDE).
        // Notice that the counter didn't reset back to zero; the application
        // is not restarted.
        primarySwatch: Colors.blue,
        // This makes the visual density adapt to the platform that you run
        // the app on. For desktop platforms, the controls will be smaller and
        // closer together (more dense) than on mobile platforms.
        visualDensity: VisualDensity.adaptivePlatformDensity,
      ),
      home: const HomePage(),
    );
  }
}

class HomePage extends StatefulWidget {
  const HomePage({Key? key}) : super(key: key);

  @override
  State<HomePage> createState() => _HomePageState();
}

class _HomePageState extends State<HomePage> {
  final ScrollController _scrollController = ScrollController();

  @override
  void initState() {
    _scrollController.addListener(_scrollListener);
    super.initState();
  }

  @override
  void dispose() {
    _scrollController.dispose();
    super.dispose();
  }

  void _scrollListener() {
    // offset bigger than maxScrollExtent?
    print(
        'offset: ${_scrollController.offset} / maxScrollExtent: ${_scrollController.position.maxScrollExtent}');
  }

  @override
  Widget build(BuildContext context) {
    return Scaffold(
        body: Theme(
            data: ThemeData(
                scrollbarTheme: ScrollbarThemeData(
                    thumbVisibility: MaterialStateProperty.all(true),
                    thumbColor: MaterialStateProperty.all(Colors.red))),
            child: Scrollbar(
                controller: _scrollController,
                child: SingleChildScrollView(
                    controller: _scrollController,
                    scrollDirection: Axis.horizontal,
                    child:
                    const SizedBox(width: 2000, child: Placeholder())))));
  }
}

and I just comment out the offset correction part

I can't reproduce the error

oh nvm I see the issue now, you have to make it small enough to be a scrollable

Yeah I think repeatedly calling applycontentDimesion makes sense now after I read more into the code. the return value is actually used to decide who should drive the scroll offset correction, return false means the performalayout should drive by repeatedly calls applycontentDimesion, so that part of the code LGTM now.

Looking for a solution to #179133, will be back later.