Skip to content

Fix drawers are draggable on desktop platforms #100476

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
fluttergithubbot merged 2 commits into from
Jun 7, 2022
Merged

Fix drawers are draggable on desktop platforms #100476

fluttergithubbot merged 2 commits into from
Jun 7, 2022

Conversation

@TahaTesser
Copy link
Member

@TahaTesser TahaTesser commented Mar 21, 2022

fixes #100321

minimal code sample 
import 'package:flutter/material.dart';

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

class MyApp extends StatelessWidget {
  const MyApp({Key? key, this.dark = true}) : super(key: key);

  final bool dark;

  @override
  Widget build(BuildContext context) {
    return MaterialApp(
      debugShowCheckedModeBanner: false,
      title: 'Material App',
      theme: dark ? ThemeData.dark() : ThemeData.light(),
      home: const Home(),
    );
  }
}

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

  @override
  Widget build(BuildContext context) {
    return Scaffold(
      appBar: AppBar(
        title: const Text('Material App Bar'),
      ),
      drawer: const Drawer(),
      endDrawer: const Drawer(),
    );
  }
}

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.
  • All existing and new tests are passing.

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

@flutter-dashboard flutter-dashboard bot added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels Mar 21, 2022
@TahaTesser TahaTesser requested a review from justinmc March 21, 2022 12:45
@skia-gold
Copy link

skia-gold commented Mar 21, 2022

Gold has detected about 1 new digest(s) on patchset 1.
View them at https://flutter-gold.skia.org/cl/github/100476

@flutter-dashboard
Copy link

flutter-dashboard bot commented Mar 21, 2022

Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change).

If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review.

For more guidance, visit Writing a golden file test for package:flutter.

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

Changes reported for pull request #100476 at sha 7deb72979b845d374955962191c16adbd5dc7b29

@flutter-dashboard flutter-dashboard bot added the will affect goldens Changes to golden files label Mar 21, 2022
@TahaTesser TahaTesser removed the will affect goldens Changes to golden files label Mar 22, 2022
justinmc
justinmc reviewed May 16, 2022
View reviewed changes
Copy link
Contributor

@justinmc justinmc left a comment

Choose a reason for hiding this comment

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

What about mobile platforms with a mouse connected or desktop platforms with a touchscreen?

Maybe in this case it's not so easy to handle those cases and we should just go with this for now... This mouse/touch and desktop/mobile distinction is something we haven't done a good job being consistent about in the history of the framework and we're going to have to figure it out at some point.

packages/flutter/lib/src/material/drawer.dart Outdated
Comment on lines 557 to 568
Copy link
Contributor

@justinmc justinmc May 16, 2022

Choose a reason for hiding this comment

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

Nit: Should this be indented like this? I haven't seen it before in the framework but kind of makes sense...

Copy link
Member Author

@TahaTesser TahaTesser Jun 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the feedback, apologies for the delay, looks like this PR fell off my radar (including a couple).

Nit: Should this be indented like this? I haven't seen it before in the framework but kind of makes sense...

Fixed.

Maybe in this case it's not so easy to handle those cases and we should just go with this for now... This mouse/touch and desktop/mobile distinction is something we haven't done a good job being consistent about in the history of the framework and we're going to have to figure it out at some point.

If you file an issue, please tag me, I'd love to help in any way possible. :)

@TahaTesser TahaTesser force-pushed the drawer_desktop_fix branch from 7deb729 to 4f54ea6 Compare June 6, 2022 14:36
@TahaTesser TahaTesser requested a review from justinmc June 6, 2022 14:37
justinmc
justinmc approved these changes Jun 7, 2022
View reviewed changes
Copy link
Contributor

@justinmc justinmc left a comment

Choose a reason for hiding this comment

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

LGTM 👍

I'll plan to gather a few examples of the desktop vs. mobile-with-keyboard-and-mouse distinction and then open an issue.

@fluttergithubbot fluttergithubbot merged commit 1718519 into flutter:master Jun 7, 2022
@talamaska
Copy link
Contributor

talamaska commented Jun 7, 2022 •
edited
Loading

I think this should be reverted. I don't like when the framework is making decisions on my behalf. And this change makes the 2 settings for enabling the drag obsolete on desktop, which is bad. If the dev wants to block the drag, he/she should use the existing props to do it depending on the platform. drawerEnableOpenDragGesture and endDrawerEnableOpenDragGesture allows to disable this. How Are you going to explain this to thousand of developers that will encounter the 2 settings not working on desktop? Editing more docs? Really bad idea. Flutter doesn't have settings that become not-working for different platforms, not that I know of.

@TahaTesser
Copy link
Member Author

TahaTesser commented Jun 7, 2022

How Are you going to explain this to thousand of developers that will encounter the 2 settings not working on the desktop?

Drawers are not draggable on desktop hence drawerEnableOpen(DragGesture) and endDrawerEnableOpen(DragGesture) . drag gestures property by definition won't be used when gestures are not supported in the first place on desktop.

Please try this Material Drawer demo, this does not support drag to open drawers.

In the same way, you can't drag to scroll a list on the desktop using gestures in Flutter
Disable scrolling by drag with a mouse.

@talamaska
Copy link
Contributor

talamaska commented Jun 7, 2022

Are you saying that desktop doesn't have drag and drop gesture?
Then what is this ?
Scroll list are something completely different.
I'm saying that you are making a global change of the framework because of one person and one issue with zero thumbs up reactions.

@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jun 8, 2022
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Jun 8, 2022
@engine-flutter-autoroll
1718519 Fix drawers are draggable on desktop platforms (flutter/flutt…
950d393 
…er#100476)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jun 8, 2022
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 8, 2022
@engine-flutter-autoroll
1718519 Fix drawers are draggable on desktop platforms (flutter/flutt…
760a78e 
…er#100476)
@stevendz stevendz mentioned this pull request Jul 11, 2022
Closed
@TahaTesser TahaTesser deleted the drawer_desktop_fix branch July 12, 2022 11:58
camsim99 pushed a commit to camsim99/flutter that referenced this pull request Aug 10, 2022
@TahaTesser @camsim99
Fix drawers are draggable on desktop platforms (flutter#100476)
4f20c13
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 30, 2022
@engine-flutter-autoroll
1718519 Fix drawers are draggable on desktop platforms (flutter/flutt…
b6f510d 
…er#100476)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Aug 30, 2022
@engine-flutter-autoroll
1718519 Fix drawers are draggable on desktop platforms (flutter/flutt…
1d19d09 
…er#100476)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@justinmc justinmc justinmc approved these changes

Assignees

No one assigned

Labels

f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

It's possbile to drag Drawers with the mouse

5 participants

@TahaTesser @skia-gold @talamaska @justinmc @fluttergithubbot