Skip to content

Conversation

@TahaTesser
Copy link
Member

@TahaTesser TahaTesser commented Apr 19, 2024

fixes DropdownMenu throws exception when nested inside nested SingleChildScrollView

Description

Scrollable.ensureVisible in DropdownMenu inside nested scroll views after the OverlayPortal migration throws an error.

The error is thrown by getTransformTo when object.parent != null is not true. This PR fixes the error by getting Scrollable and call ensureVisible.

Code sample

expand to view the code sample
import 'package:flutter/material.dart';

void main() => runApp(const MyApp());

class MyApp extends StatelessWidget {
  const MyApp({super.key});

  @override
  Widget build(BuildContext context) {
    return const MaterialApp(
      debugShowCheckedModeBanner: false,
      home: Material(
        child: SingleChildScrollView(
          child: Card(
            child: SingleChildScrollView(
              child: DropdownMenu<String>(
                label: Text('test'),
                menuHeight: 100,
                dropdownMenuEntries: <DropdownMenuEntry<String>>[
                  DropdownMenuEntry<String>(value: '1', label: 'one'),
                  DropdownMenuEntry<String>(value: '2', label: 'two'),
                  DropdownMenuEntry<String>(value: '3', label: 'three'),
                ],
              ),
            ),
          ),
        ),
      ),
    );
  }
}

Before

Screenshot 2024-04-22 at 18 08 56

After

Screenshot 2024-04-22 at 18 09 10

Pre-launch Checklist

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

@github-actions github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. f: scrolling Viewports, list views, slivers, etc. labels Apr 19, 2024
@TahaTesser TahaTesser force-pushed the dropdown_menu_ensure_visible branch 2 times, most recently from e8bd1ce to 996b013 Compare April 19, 2024 15:00
@TahaTesser TahaTesser changed the title Fix Scrollable.ensureVisible throws an error in DropdownMenu inside nested scroll views WIP] Fix Scrollable.ensureVisible throws an error in DropdownMenu inside nested scroll views Apr 19, 2024
@TahaTesser TahaTesser changed the title WIP] Fix Scrollable.ensureVisible throws an error in DropdownMenu inside nested scroll views [WIP] Fix Scrollable.ensureVisible throws an error in DropdownMenu inside nested scroll views Apr 19, 2024
@TahaTesser TahaTesser force-pushed the dropdown_menu_ensure_visible branch from 996b013 to f806af2 Compare April 22, 2024 07:57
@github-actions github-actions bot removed the f: scrolling Viewports, list views, slivers, etc. label Apr 22, 2024
@TahaTesser TahaTesser changed the title [WIP] Fix Scrollable.ensureVisible throws an error in DropdownMenu inside nested scroll views Fix Scrollable.ensureVisible throws an error in DropdownMenu inside nested scroll views Apr 22, 2024
@TahaTesser TahaTesser marked this pull request as ready for review April 22, 2024 09:38
@TahaTesser TahaTesser requested a review from Piinks April 22, 2024 09:38
@Piinks Piinks added the f: scrolling Viewports, list views, slivers, etc. label Apr 22, 2024
WidgetsBinding.instance.addPostFrameCallback((_) {
final BuildContext? highlightContext = buttonItemKeys[currentHighlight!].currentContext;
if (highlightContext != null) {
Scrollable.ensureVisible(highlightContext);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't we fix Scrollable.ensureVisible? Surely it's a bug that calling that doesn't work.

Copy link
Member Author

@TahaTesser TahaTesser Apr 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DropdownMenu calls Scrollable.ensureVisible which then calls ScrollPosition.ensureVisible. However, after the OverlayPortal migration, it seems the correct scrollable context isn't passed in nested scroll views.

RenderObject? targetRenderObject;
ScrollableState? scrollable = Scrollable.maybeOf(context);
while (scrollable != null) {
final List<Future<void>> newFutures;
(newFutures, scrollable) = scrollable._performEnsureVisible(
context.findRenderObject()!,

final Future<void> ensureVisibleFuture = position.ensureVisible(

My fix finds the scrollable and calls ScrollPosition.ensureVisible directly.

I'm not expert in scrollables so I'll defer to @Piinks, if this makes sense.

Copy link
Member Author

@TahaTesser TahaTesser Apr 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can reproduce the issue in the official OverlayPortal example by nesting scroll views.

Hixie is right it is a bug in Scrollable.ensureVisible. This happens only in OverlayPortal. Scrollable.ensureVisible cannot find the right ScrollableState inside it.

Code sample

expand to view the code sample
import 'package:flutter/material.dart';

/// Flutter code sample for [OverlayPortal].

void main() => runApp(const OverlayPortalExampleApp());

class OverlayPortalExampleApp extends StatelessWidget {
  const OverlayPortalExampleApp({super.key});

  @override
  Widget build(BuildContext context) {
    return MaterialApp(
      home: Scaffold(
        appBar: AppBar(title: const Text('OverlayPortal Example')),
        body: const Center(child: ClickableTooltipWidget()),
      ),
    );
  }
}

class ClickableTooltipWidget extends StatefulWidget {
  const ClickableTooltipWidget({super.key});

  @override
  State<StatefulWidget> createState() => ClickableTooltipWidgetState();
}

class ClickableTooltipWidgetState extends State<ClickableTooltipWidget> {
  final OverlayPortalController _tooltipController = OverlayPortalController();

  @override
  Widget build(BuildContext context) {
    return SingleChildScrollView(
      child: Card(
        child: SingleChildScrollView(
          child: TextButton(
            onPressed: _tooltipController.toggle,
            child: DefaultTextStyle(
              style: DefaultTextStyle.of(context).style.copyWith(fontSize: 50),
              child: OverlayPortal(
                controller: _tooltipController,
                overlayChildBuilder: (BuildContext context) {
                  return  Positioned(
                    right: 50,
                    bottom: 50,
                    child: GestureDetector(
                      onTap: () {
                        print('tooltip tapped');
                        Scrollable.ensureVisible(context);
                      },
                      child: const ColoredBox(
                        color: Colors.amberAccent,
                        child: Text('tooltip'),
                      ),
                    ),
                  );
                },
                child: const Text('Press to show/hide tooltip'),
              ),
            ),
          ),
        ),
      ),
    );
  }
}

Copy link
Member Author

@TahaTesser TahaTesser Apr 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#67773 made similar fix for nested scroll views, it doesn't handle OverlayPortal scenario very well and this is where the error is coming.

I'll close this PR to find out why this fails in an OverlayPortal and try to fix this in ensureVisible instead.

@TahaTesser TahaTesser closed this Apr 23, 2024
@TahaTesser TahaTesser deleted the dropdown_menu_ensure_visible branch April 23, 2024 11:49
@felixSchl
Copy link

Hey, I am running into this problem, too. How can I fix this in user-land or do I have to wait for an upstream fix?

@anthonyR012
Copy link

Hello, I would like to know if there is any news about this issue. It seems that I am facing the same error.

@TahaTesser
Copy link
Member Author

This was fixed in #148897. It would be available in the next major stable release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

f: material design flutter/packages/flutter/material repository. f: scrolling Viewports, list views, slivers, etc. framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

DropdownMenu throws exception when nested inside two SingleChildScrollView

5 participants