fix: update _floatingActionButtonVisibility only if floatingActionButon is not null #148829

hello-coder-xu · 2024-05-22T09:12:00Z

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I signed the CLA.
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making, or this PR is test-exempt.
I followed the breaking change policy and added Data Driven Fixes where supported.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

…ton is not null

…pdate * master: (92 commits) Add tests for actions.0.dart API example. (flutter#148678) Introduce `WidgetStateBorderSide.lerp` (flutter#148122) add `default-flavor` field to flutter pubspec, which will be used as the flavor in `flutter build/run` if `--flavor` is not provided (flutter#147968) [wiki migration] Pages under docs/postmortems/ (flutter#148798) Roll Flutter Engine from e5a73e520e89 to c89defa55801 (2 revisions) (flutter#148812) Make hover tests functional and cleanup mouse pointers in Material toggleables (flutter#148808) Fix two dimensional viewport unexpected null exception when no child is laid out (flutter#148256) Roll Flutter Engine from bc1345b6b50a to e5a73e520e89 (3 revisions) (flutter#148807) Add test for undo_history_controller.0.dart (flutter#148205) Roll Flutter Engine from a8872c8915a2 to bc1345b6b50a (6 revisions) (flutter#148802) Fix test that leaks images. (flutter#148494) Fix warnings in `dependency_version_checker.gradle.kts` (flutter#148699) [wiki migration] Android team pages (flutter#148585) Fix leaky test. (flutter#148788) Add DropdownButton.menuWidth (flutter#148125) Add test for focus example 2 (flutter#147624) Add a migrator to remove `FlutterMultiDexApplication.java` (flutter#148515) [wiki migration] Infra team pages (flutter#148718) Roll Flutter Engine from 8a352f01e503 to a8872c8915a2 (1 revision) (flutter#148776) Fix the output of the CDN test. (flutter#148730) ...

JaffaKetchup · 2024-05-22T20:12:54Z

This comment (#146736 (comment)) reports an issue of the exact same type at the exact same call stack - but doesn't relate to whether there is a FAB.

Although this might fix the issue with the FAB, I doubt it will fix the root issue, which seems to be in this line:

flutter/packages/flutter/lib/src/widgets/draggable_scrollable_sheet.dart

Line 583 in 26e5352

_currentSize.value = clampedSize;

The change to the ValueNotifier causes a listener on it to fire, which fires another listener, which calls here. This appears to be the setState that causes the issue.

flutter/packages/flutter/lib/src/widgets/transitions.dart

Lines 128 to 135 in 26e5352

    
           void _handleChange() { 
        
             if (!mounted) { 
        
               return; 
        
             } 
        
             setState(() { 
        
               // The listenable's state is our build state, and it changed already. 
        
             }); 
        
           }

Interestingly, the mounted check was added in 1d7c680, which doesn't appear to be present on the current beta v3.22.0-0.3.pre. I doubt this check will make a difference, as it's not a set state after disposal issue, but I could be wrong. Can the original issue be reproduced on 'master'?

hello-coder-xu · 2024-05-23T03:15:24Z

This comment (#146736 (comment)) reports an issue of the exact same type at the exact same call stack - but doesn't relate to whether there is a FAB.

Although this might fix the issue with the FAB, I doubt it will fix the root issue, which seems to be in this line:

flutter/packages/flutter/lib/src/widgets/draggable_scrollable_sheet.dart

Line 583 in 26e5352

_currentSize.value = clampedSize;

The change to the ValueNotifier causes a listener on it to fire, which fires another listener, which calls here. This appears to be the setState that causes the issue.

flutter/packages/flutter/lib/src/widgets/transitions.dart

Lines 128 to 135 in 26e5352

void _handleChange() {

if (!mounted) {

return;

}

setState(() {

// The listenable's state is our build state, and it changed already.

});

}

Interestingly, the mounted check was added in 1d7c680, which doesn't appear to be present on the current beta v3.22.0-0.3.pre. I doubt this check will make a difference, as it's not a set state after disposal issue, but I could be wrong. Can the original issue be reproduced on 'master'?

I use the latest master, the problem still exists

Doctor summary (to see all details, run flutter doctor -v):
[✓] Flutter (Channel master, 3.22.0-44.0.pre.10, on macOS 12.5.1 21G83 darwin-arm64, locale zh-Hans-CN)
[✓] Android toolchain - develop for Android devices (Android SDK version 34.0.0)
[!] Xcode - develop for iOS and macOS (Xcode 14.2)
    ! Flutter recommends a minimum Xcode version of 15.
      Download the latest version or update via the Mac App Store.
[✓] Chrome - develop for the web
[✓] Android Studio (version 2023.3)
[✓] VS Code (version 1.89.1)
[✓] Connected device (3 available)
[✓] Network resources

! Doctor found issues in 1 category.

hello-coder-xu · 2024-05-23T03:27:08Z

I merged and formatted the code

JaffaKetchup

As I stated above, I'm not sure this PR fixes the underlying issue, and if it is merged, I will likely have to open another issue that's pretty much exactly the same.

justinmc · 2024-05-24T21:16:56Z

packages/flutter/lib/src/material/scaffold.dart

+      if (scaffold.widget.floatingActionButton != null) {
+        scaffold._floatingActionButtonVisibilityValue = 1.0;
+      }


This might just be hacking around the problem and not addressing the root cause, but I'll defer to @Piinks.

Piinks

I agree with @JaffaKetchup and @justinmc. This does not appear to fix the root issue since it is not dependent on the presence of the FAB.

@hello-coder-xu if you would like to continue working on this, one idea may be to create a test case where this issue reproduces without a FAB. That way if the test passes, we'll know that the primary issue here is fixed.

JaffaKetchup · 2024-05-30T14:53:54Z

I'm happy to create an MRE of my case that doesn't use a FAB, but it might take a little while to boil down from what I've got right now. Would that be helpful?
I'm not good enough with widget tests to write a proper test though!

JaffaKetchup · 2024-05-30T17:07:31Z

Here's an MRE. It doesn't do anything complex at all, doesn't even use anything from the DraggableScrollableController, just listens to it using an AnimatedBuilder.

The error occurs when the bottom sheet is expanded, then shrunk to/past its initial height.

void main() {
  final outerBottomSheetController = DraggableScrollableController();

  runApp(
    MaterialApp(
      home: Scaffold(
        body: AnimatedBuilder(
          animation: outerBottomSheetController,
          builder: (context, _) => const SizedBox(),
        ),
        bottomSheet: DraggableScrollableSheet(
          controller: outerBottomSheetController,
          builder: (context, innerScrollController) => ColoredBox(
            color: Colors.black,
            child: ListView.builder(
              controller: innerScrollController,
              itemBuilder: (context, index) => Text(index.toString()),
            ),
          ),
        ),
      ),
    ),
  );
}

hello-coder-xu · 2024-05-31T14:50:47Z

Here's an MRE. It doesn't do anything complex at all, doesn't even use anything from the DraggableScrollableController, just listens to it using an AnimatedBuilder.这是一个 MRE。它根本不做任何复杂的事情，甚至不使用， DraggableScrollableController 只是使用 AnimatedBuilder .

The error occurs when the bottom sheet is expanded, then shrunk to/past its initial height.当底板展开，然后缩小到/超过其初始高度时，就会发生错误。
void main() {
  final outerBottomSheetController = DraggableScrollableController();

  runApp(
    MaterialApp(
      home: Scaffold(
        body: AnimatedBuilder(
          animation: outerBottomSheetController,
          builder: (context, _) => const SizedBox(),
        ),
        bottomSheet: DraggableScrollableSheet(
          controller: outerBottomSheetController,
          builder: (context, innerScrollController) => ColoredBox(
            color: Colors.black,
            child: ListView.builder(
              controller: innerScrollController,
              itemBuilder: (context, index) => Text(index.toString()),
            ),
          ),
        ),
      ),
    ),
  );
}

Yes, I reproduced the problem, and I found another solution, I need some time to verify it

hello-coder-xu · 2024-06-06T07:23:41Z

I updated it, please check it out

Piinks · 2024-06-06T20:22:34Z

packages/flutter/test/widgets/draggable_scrollable_sheet_test.dart

@@ -1746,4 +1746,49 @@ void main() {
      areCreateAndDispose,
    );
  });
+


Can you add back the test you had for the floating action button? That way we know we fixed both scenarios where this bug presented.

Piinks · 2024-06-06T20:23:33Z

packages/flutter/lib/src/widgets/draggable_scrollable_sheet.dart

@@ -835,7 +835,14 @@ class _DraggableScrollableSheetScrollController extends ScrollController {
        curve: Curves.linear,
      );
    }
-    extent.updateSize(extent.initialSize, position.context.notificationContext!);
+    final StatefulElement element = position.context.notificationContext! as StatefulElement;


This is very different from the earlier fix, can you explain a bit in the PR description what you found as the root issue, and why this fixes it?

i added description

Hmm, ok, thank you for adding the comment.
This does not look like the right fix to me.

We should not need to be checking whether or not the element is dirty, and further shouldn't be using a post frame callback to initiate the update. It feels like this is working around the root cause of the issue, do you know why we end up here with a dirty element? Tracing this back further will probably reveal the real issue.

…pdate * master: (181 commits) Fix the scrolling layout deviation of `CupertinoActionSheet` (flutter#149439) Roll Flutter Engine from 60a7bb2353b6 to a6aa5d826649 (2 revisions) (flutter#149627) Roll Flutter Engine from ea72558be758 to 60a7bb2353b6 (2 revisions) (flutter#149623) Place `flutter_gpu` in the package cache. (flutter#149299) Switch to triage-* labels for platform package triage (flutter#149614) Roll pub packages (flutter#149617) Fixes multi line textfield hint text gets ellipsized (flutter#148423) Support failures-only and silent reporters in `flutter test` (flutter#148739) [CupertinoActionSheet] Fix overflow of the overscroll section when the user scrolls far (flutter#149542) Fix InputDecorator.prefixIcon color when disabled (flutter#149595) Added filter callback on dropdown menu (flutter#143939) update generated localized message files in the stocks test app (flutter#148741) Add a simplified SimpleCascadingMenuApp example (flutter#149147) Reland "Prevent LayoutBuilder from rebuilding more than once (flutter#147856)" (flutter#149303) Move some benchmarks from MotoG4 to Mokey (flutter#149567) Roll Packages from d8e8e8cee712 to 11e192a86db3 (2 revisions) (flutter#149596) Cleanup triage reports from docs/ (flutter#149545) Roll Flutter Engine from d81edf635a9f to ea72558be758 (1 revision) (flutter#149590) Roll Flutter Engine from b0f4d7459708 to d81edf635a9f (1 revision) (flutter#149468) Roll Flutter Engine from 40b868efcc46 to b0f4d7459708 (1 revision) (flutter#149467) ...

Piinks · 2024-06-13T16:14:21Z

packages/flutter/lib/src/widgets/draggable_scrollable_sheet.dart

@@ -835,7 +835,14 @@ class _DraggableScrollableSheetScrollController extends ScrollController {
        curve: Curves.linear,
      );
    }
-    extent.updateSize(extent.initialSize, position.context.notificationContext!);
+    final StatefulElement element = position.context.notificationContext! as StatefulElement;


Hmm, ok, thank you for adding the comment.
This does not look like the right fix to me.

We should not need to be checking whether or not the element is dirty, and further shouldn't be using a post frame callback to initiate the update. It feels like this is working around the root cause of the issue, do you know why we end up here with a dirty element? Tracing this back further will probably reveal the real issue.

hello-coder-xu · 2024-06-14T01:33:11Z

@Piinks
You're right, I should look for why it's marked dirty, and I'm trying to do that, but I haven't found it yet...
I will continue

fix: update _floatingActionButtonVisibility only if floatingActionBut…

76ae988

…ton is not null

github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. labels May 22, 2024

Piinks self-requested a review May 22, 2024 18:30

Piinks force-pushed the fix/_floatingActionButtonVisibilityValue-update branch from e09a2a2 to 05ce0ce Compare May 22, 2024 19:43

JaffaKetchup reviewed May 23, 2024

View reviewed changes

justinmc reviewed May 24, 2024

View reviewed changes

Piinks requested changes May 30, 2024

View reviewed changes

JaffaKetchup mentioned this pull request May 30, 2024

"setState() or markNeedsBuild() called during build" error combining DraggableScrollableSheet with FloatingActionButton #146736

Open

github-actions bot added f: scrolling Viewports, list views, slivers, etc. and removed f: material design flutter/packages/flutter/material repository. labels Jun 4, 2024

hello-coder-xu force-pushed the fix/_floatingActionButtonVisibilityValue-update branch from 4e0aaa3 to 3846fea Compare June 6, 2024 06:01

github-actions bot added f: material design flutter/packages/flutter/material repository. f: scrolling Viewports, list views, slivers, etc. and removed f: scrolling Viewports, list views, slivers, etc. f: material design flutter/packages/flutter/material repository. labels Jun 6, 2024

Piinks reviewed Jun 6, 2024

View reviewed changes

github-actions bot added the f: material design flutter/packages/flutter/material repository. label Jun 9, 2024

hello-coder-xu force-pushed the fix/_floatingActionButtonVisibilityValue-update branch from c4d98fb to 26adfe6 Compare June 9, 2024 17:13

github-actions bot removed the f: scrolling Viewports, list views, slivers, etc. label Jun 9, 2024

udpated

e137945

github-actions bot added the f: scrolling Viewports, list views, slivers, etc. label Jun 9, 2024

updated

17eadaf

dkwingsmt self-requested a review June 12, 2024 17:36

Piinks requested changes Jun 13, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: update _floatingActionButtonVisibility only if floatingActionButon is not null #148829

fix: update _floatingActionButtonVisibility only if floatingActionButon is not null #148829

hello-coder-xu commented May 22, 2024

JaffaKetchup commented May 22, 2024 •

edited

hello-coder-xu commented May 23, 2024

hello-coder-xu commented May 23, 2024

JaffaKetchup left a comment

justinmc May 24, 2024

Piinks left a comment

JaffaKetchup commented May 30, 2024 •

edited

JaffaKetchup commented May 30, 2024

hello-coder-xu commented May 31, 2024

hello-coder-xu commented Jun 6, 2024

Piinks Jun 6, 2024

hello-coder-xu Jun 11, 2024

Piinks Jun 6, 2024

hello-coder-xu Jun 11, 2024

Piinks Jun 13, 2024

Piinks Jun 13, 2024

hello-coder-xu commented Jun 14, 2024

fix: update _floatingActionButtonVisibility only if floatingActionButon is not null #148829

Are you sure you want to change the base?

fix: update _floatingActionButtonVisibility only if floatingActionButon is not null #148829

Conversation

hello-coder-xu commented May 22, 2024

Pre-launch Checklist

JaffaKetchup commented May 22, 2024 • edited

hello-coder-xu commented May 23, 2024

hello-coder-xu commented May 23, 2024

JaffaKetchup left a comment

Choose a reason for hiding this comment

justinmc May 24, 2024

Choose a reason for hiding this comment

Piinks left a comment

Choose a reason for hiding this comment

JaffaKetchup commented May 30, 2024 • edited

JaffaKetchup commented May 30, 2024

hello-coder-xu commented May 31, 2024

hello-coder-xu commented Jun 6, 2024

Piinks Jun 6, 2024

Choose a reason for hiding this comment

hello-coder-xu Jun 11, 2024

Choose a reason for hiding this comment

Piinks Jun 6, 2024

Choose a reason for hiding this comment

hello-coder-xu Jun 11, 2024

Choose a reason for hiding this comment

Piinks Jun 13, 2024

Choose a reason for hiding this comment

Piinks Jun 13, 2024

Choose a reason for hiding this comment

hello-coder-xu commented Jun 14, 2024

JaffaKetchup commented May 22, 2024 •

edited

JaffaKetchup commented May 30, 2024 •

edited