-
Notifications
You must be signed in to change notification settings - Fork 29.1k
Make the barrier panel on the Material Drawer non-dismissible #164810
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.
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
Make the barrier panel on the Material Drawer non-dismissible #164810
Conversation
…roperty named drawerDismissible
// If set to false, prevents the barrier behind the [Drawer] from being dismissed. | ||
// Defaults to true. The value of this flag also gets inherited by the parent Scaffold, | ||
// or can be overridden if a [DrawerController] is extended and this value set accordingly. | ||
final bool drawerDismissible; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder why we call it drawerDismissible
here instead of simply dismissible
, since this is already known as a DrawerController. (I know some other properties are also prefixed with "drawer", which also appears weird to me.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd totally be open to call it dismissible - I was just trying to keep the existing convention which I also thought it was a bit redundant but didn't want to have to change the rest if there was a purpose for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it. Can you rename it to dismissible
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will do!
/// If false, and a [drawer] is specified, then the barrier behind the drawer should not | ||
/// respond to a tap event and thus remain open. The default behavior is to close upon the | ||
/// user tapping on the barrier. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same doc style guide problem as above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, this should be split into 2 paragraphs so that the first paragraph is a single sentence, per the styleguide.
@@ -27,4 +27,109 @@ void main() { | |||
await tester.pumpAndSettle(); | |||
expect(findHeadingLevelOnes, findsOne); | |||
}); | |||
|
|||
testWidgets('drawer is dismissible', (WidgetTester tester) async { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless it's intentional, test cases should reside in packages/flutter/test/material/drawer_test.dart
, not a11y_assessments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, not intentional. I actually saw some of the other drawer tests there so I wanted to put them along with the rest, but these can easily be moved wherever is more appropriate.
Co-authored-by: Tong Mu <dkwingsmt@users.noreply.github.com>
…missible"; moved tests topackages/flutter/test/material/drawer_test.dart
Can you move the test to the unit test file? (
|
…ally_assessments; left the ones there as they were before
One analyzer issue missing
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
…omanejaquez/flutter into drawer_barrier_non_dismissible
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few small things here, and it looks like the analyzer is failing. Thank you for the PR!
Is there an issue for this by the way?
/// If false, and a [drawer] is specified, then the barrier behind the drawer should not | ||
/// respond to a tap event and thus remain open. The default behavior is to close upon the | ||
/// user tapping on the barrier. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, this should be split into 2 paragraphs so that the first paragraph is a single sentence, per the styleguide.
), | ||
); | ||
|
||
// check the flag is set at the Scaffold level |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Capital letter at the start and period at the end, here and elsewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Took care of the comments above. However for the comment on the scaffold.dart, for the first sentence I had to split it into 2 because it would exceed the max length on a comment line.
Fixed the punctuation and capitalization on the other comments - thank you for catching that.
@@ -851,3 +1003,5 @@ void main() { | |||
}); | |||
}); | |||
} | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove these newlines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gotcha - addressed the newline issue on the drawer_test.dart, but for some reason the Linux analyzer keeps barking on the newline not being added.
Co-authored-by: Tong Mu <dkwingsmt@users.noreply.github.com>
Co-authored-by: Tong Mu <dkwingsmt@users.noreply.github.com>
|
The analyzer errors seem to be only format errors, which can be resolved by running |
will do 👍 |
This is probably good to go but I will give it a closer re-review on Monday. Sorry for the delay. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM but I see a few things that you seem to have intended to change but didn't. See my comments below. Maybe you forgot to push a commit.
@@ -339,6 +339,7 @@ class DrawerController extends StatefulWidget { | |||
this.scrimColor, | |||
this.edgeDragWidth, | |||
this.enableOpenDragGesture = true, | |||
this.drawerBarrierDismissible = true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the name drawerBarrierDismissible and I don't think it's too long 👍
final ScaffoldState state = tester.firstState(find.byType(Scaffold)); | ||
state.openEndDrawer(); | ||
|
||
await tester.pump(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This pump is still here.
state.openEndDrawer(); | ||
|
||
await tester.pump(); | ||
await tester.pump(const Duration(seconds: 1)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This hasn't been replaced by pumpAndSettle (also below).
|
||
expect(find.byType(Drawer), findsExactly(1)); | ||
|
||
// Tap on the modal barrier. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This hasn't been removed.
Co-authored-by: Justin McCandless <justinjmccandless@gmail.com>
Thanks for the comments. I was missing one additional commit where those remaining changes were - also I left the previous tests there as-is as I didn't want to disrupt anything else other than the ones I added (since I saw a bunch of other pumps and such, and whoever added those must've added them for a reason). |
flutter/flutter@54de32d...336a7ec 2025-05-13 sokolovskyi.konstantin@gmail.com Add assert for index parameter in IndexedStack. (flutter/flutter#167757) 2025-05-13 jessiewong401@gmail.com Fixed Android Lint Errors (flutter/flutter#168613) 2025-05-13 bruno.leroux@gmail.com Tab bar theme and dialog theme documentation cleanup (flutter/flutter#168724) 2025-05-13 engine-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from 6vjKe0bfYDVaECqBL... to 6J81agNhuK4q255Jj... (flutter/flutter#168712) 2025-05-12 romanejaquez@gmail.com Make the barrier panel on the Material Drawer non-dismissible (flutter/flutter#164810) 2025-05-12 engine-flutter-autoroll@skia.org Roll ICU from c9fb4b3a6fb5 to b929596baebf (5 revisions) (flutter/flutter#168691) 2025-05-12 bruno.leroux@gmail.com Fix DropdownButtonFormField icon not vertically centered (flutter/flutter#163205) 2025-05-12 jmccandless@google.com Android home/end keyboard shortcut support (flutter/flutter#168184) 2025-05-12 matanlurey@users.noreply.github.com Update `TESTOWNERS` for Android PV tests (flutter/flutter#168694) 2025-05-12 engine-flutter-autoroll@skia.org Roll Dart SDK from 0bea6379f654 to 645d04a7a964 (14 revisions) (flutter/flutter#168679) 2025-05-12 chinmaygarde@google.com [Impeller] libImpeller: Allow setting text decorations. (flutter/flutter#168408) 2025-05-12 34871572+gmackall@users.noreply.github.com Error when trying to use old HC mode when HCPP is enabled (flutter/flutter#168027) 2025-05-12 victorsanniay@gmail.com Nav bar back label is not clipped mid-transition (flutter/flutter#168194) 2025-05-12 58529443+srujzs@users.noreply.github.com Add test for stack trace mapping and test expression eval tests using DDC library bundle format (flutter/flutter#168017) 2025-05-12 victorsanniay@gmail.com Remove CupertinoSliverNavigationBar background box when large title is extended (flutter/flutter#168407) 2025-05-12 1063596+reidbaker@users.noreply.github.com Label platform view modes using the unified naming scheme (flutter/flutter#168670) 2025-05-12 engine-flutter-autoroll@skia.org Roll Packages from 7814fab to 6a28ad9 (2 revisions) (flutter/flutter#168669) 2025-05-12 matanlurey@users.noreply.github.com Update `Engine-artifacts.md` to reflect the new `engine.version` verifier (flutter/flutter#168413) 2025-05-12 engine-flutter-autoroll@skia.org Roll Skia from 9f9e1f37917e to c97451da059f (1 revision) (flutter/flutter#168671) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC stuartmorgan@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
…r#9246) flutter/flutter@54de32d...336a7ec 2025-05-13 sokolovskyi.konstantin@gmail.com Add assert for index parameter in IndexedStack. (flutter/flutter#167757) 2025-05-13 jessiewong401@gmail.com Fixed Android Lint Errors (flutter/flutter#168613) 2025-05-13 bruno.leroux@gmail.com Tab bar theme and dialog theme documentation cleanup (flutter/flutter#168724) 2025-05-13 engine-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from 6vjKe0bfYDVaECqBL... to 6J81agNhuK4q255Jj... (flutter/flutter#168712) 2025-05-12 romanejaquez@gmail.com Make the barrier panel on the Material Drawer non-dismissible (flutter/flutter#164810) 2025-05-12 engine-flutter-autoroll@skia.org Roll ICU from c9fb4b3a6fb5 to b929596baebf (5 revisions) (flutter/flutter#168691) 2025-05-12 bruno.leroux@gmail.com Fix DropdownButtonFormField icon not vertically centered (flutter/flutter#163205) 2025-05-12 jmccandless@google.com Android home/end keyboard shortcut support (flutter/flutter#168184) 2025-05-12 matanlurey@users.noreply.github.com Update `TESTOWNERS` for Android PV tests (flutter/flutter#168694) 2025-05-12 engine-flutter-autoroll@skia.org Roll Dart SDK from 0bea6379f654 to 645d04a7a964 (14 revisions) (flutter/flutter#168679) 2025-05-12 chinmaygarde@google.com [Impeller] libImpeller: Allow setting text decorations. (flutter/flutter#168408) 2025-05-12 34871572+gmackall@users.noreply.github.com Error when trying to use old HC mode when HCPP is enabled (flutter/flutter#168027) 2025-05-12 victorsanniay@gmail.com Nav bar back label is not clipped mid-transition (flutter/flutter#168194) 2025-05-12 58529443+srujzs@users.noreply.github.com Add test for stack trace mapping and test expression eval tests using DDC library bundle format (flutter/flutter#168017) 2025-05-12 victorsanniay@gmail.com Remove CupertinoSliverNavigationBar background box when large title is extended (flutter/flutter#168407) 2025-05-12 1063596+reidbaker@users.noreply.github.com Label platform view modes using the unified naming scheme (flutter/flutter#168670) 2025-05-12 engine-flutter-autoroll@skia.org Roll Packages from 7814fab to 6a28ad9 (2 revisions) (flutter/flutter#168669) 2025-05-12 matanlurey@users.noreply.github.com Update `Engine-artifacts.md` to reflect the new `engine.version` verifier (flutter/flutter#168413) 2025-05-12 engine-flutter-autoroll@skia.org Roll Skia from 9f9e1f37917e to c97451da059f (1 revision) (flutter/flutter#168671) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC stuartmorgan@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
…r#9246) flutter/flutter@54de32d...336a7ec 2025-05-13 sokolovskyi.konstantin@gmail.com Add assert for index parameter in IndexedStack. (flutter/flutter#167757) 2025-05-13 jessiewong401@gmail.com Fixed Android Lint Errors (flutter/flutter#168613) 2025-05-13 bruno.leroux@gmail.com Tab bar theme and dialog theme documentation cleanup (flutter/flutter#168724) 2025-05-13 engine-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from 6vjKe0bfYDVaECqBL... to 6J81agNhuK4q255Jj... (flutter/flutter#168712) 2025-05-12 romanejaquez@gmail.com Make the barrier panel on the Material Drawer non-dismissible (flutter/flutter#164810) 2025-05-12 engine-flutter-autoroll@skia.org Roll ICU from c9fb4b3a6fb5 to b929596baebf (5 revisions) (flutter/flutter#168691) 2025-05-12 bruno.leroux@gmail.com Fix DropdownButtonFormField icon not vertically centered (flutter/flutter#163205) 2025-05-12 jmccandless@google.com Android home/end keyboard shortcut support (flutter/flutter#168184) 2025-05-12 matanlurey@users.noreply.github.com Update `TESTOWNERS` for Android PV tests (flutter/flutter#168694) 2025-05-12 engine-flutter-autoroll@skia.org Roll Dart SDK from 0bea6379f654 to 645d04a7a964 (14 revisions) (flutter/flutter#168679) 2025-05-12 chinmaygarde@google.com [Impeller] libImpeller: Allow setting text decorations. (flutter/flutter#168408) 2025-05-12 34871572+gmackall@users.noreply.github.com Error when trying to use old HC mode when HCPP is enabled (flutter/flutter#168027) 2025-05-12 victorsanniay@gmail.com Nav bar back label is not clipped mid-transition (flutter/flutter#168194) 2025-05-12 58529443+srujzs@users.noreply.github.com Add test for stack trace mapping and test expression eval tests using DDC library bundle format (flutter/flutter#168017) 2025-05-12 victorsanniay@gmail.com Remove CupertinoSliverNavigationBar background box when large title is extended (flutter/flutter#168407) 2025-05-12 1063596+reidbaker@users.noreply.github.com Label platform view modes using the unified naming scheme (flutter/flutter#168670) 2025-05-12 engine-flutter-autoroll@skia.org Roll Packages from 7814fab to 6a28ad9 (2 revisions) (flutter/flutter#168669) 2025-05-12 matanlurey@users.noreply.github.com Update `Engine-artifacts.md` to reflect the new `engine.version` verifier (flutter/flutter#168413) 2025-05-12 engine-flutter-autoroll@skia.org Roll Skia from 9f9e1f37917e to c97451da059f (1 revision) (flutter/flutter#168671) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC stuartmorgan@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Should the "Esc" key still close the drawer? I'm testing and I can't close it by clicking outside, but I think the "Esc" key shouldn't close the drawer either, but it currently can. |
You are correct - ESC shouldn't close it. I'll submit a PR to fix that. Thanks for flagging it! |
Good call, and I agree! Feel free to let me review it. |
here's the PR for review! => #173263 |
This PR entails making the modal barrier behind the Material Drawer dismissible / non-dismissible via a Scaffold flag named "drawerBarrierDismissible", as in:
This is so as to force the Drawer to be opened / closed programmatically using either:
or
and the user not closing the Drawer upon tapping on the modal barrier.
Pre-launch Checklist
///
).If you need help, consider asking for advice on the #hackers-new channel on [Discord].