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
Add MediaQuery.systemGestureInsets to support Android Q #37416
Add MediaQuery.systemGestureInsets to support Android Q #37416
Conversation
I'm currently working on the engine counterpart of this PR, so until that lands, this PR will remain a work in progress! |
I'm closing this for now, since this is not actionable given that we're blocked on the engine PR. I will create a new PR once the engine PR is completed. |
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
@@ -213,6 +215,45 @@ class MediaQueryData { | |||
/// property and how it relates to [padding] and [viewInsets]. | |||
final EdgeInsets viewPadding; | |||
|
|||
/// The parts of the display that contain system gestures, typically swipes |
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.
"contains" is kinds strange. Isn't this just the area where the system may detect certain gestures, handle them internally, and not forward those to your app?
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 will update this to
/// The parts of the display where the system may be detecting particular
/// gestures.
///
/// This value is typically a drag gesture from an edge of the screen.
Let me know if there is any additional improvement/clarification we could make
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.
It's important to make this extra clear and part of that will be explaining exactly what AndroidQ does. So:
The areas along the edges of the display where the system consumes certain input events and blocks delivery of those events to the app.
Starting with Android Q, simple swipe gestures that start within the [systemGestureInsets] areas are used by the system for page navigation and are not delivered to the app. Taps and swipe gestures that begin with a long-press are delivered to the app but simple press-drag-release swipe gestures which begin within the area defined by [systemGestureInsets] are not.
Apps should avoid locating gesture detectors within the system gesture insets area. There's no reason to avoid putting visuals within this area.
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.
There's a little nuance to this when I was testing it yesterday... The swipe gesture can still be registered by the app. I've gotten a drawer to open on a simple swipe, but it simultaneously triggers the back navigation arrow to appear. However, this is not consistent and sometimes, the drawer will appear and almost immediately disappear once the back navigation arrow starts to appear.
I'll try to share a gif, but I think the general idea is that there will be conflicts in how simple drag gestures are handled in these inset areas and that devs should just avoid it anyway.
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.
Did you have a chance to just place a Listener [1] in this area and see what events you actually get in Flutter?
[1] https://master-api.flutter.dev/flutter/widgets/Listener-class.html
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.
All this to say, maybe updating the wording to "simple swipe gestures that start within the [systemGestureInsets] areas are used by the system for page navigation and are may not be delivered to the app"
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.
Sounds OK to me. We can tighten up the wording in the future, if we need to.
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.
@goderbauer I just played with the Listener
, and it seems like it lets all gestures through and once you start doing a simple horizontal swipe, it sends a cancel event and begins the system gesture.
Oddly, if you do a diagonal swipe from the edge (ie. top-left to bottom-right or bottom-left to top-right), that seems to never trigger the system gesture. I decided to try this after reading about it as a "workaround" on some articles on the internet.
@@ -213,6 +215,45 @@ class MediaQueryData { | |||
/// property and how it relates to [padding] and [viewInsets]. | |||
final EdgeInsets viewPadding; | |||
|
|||
/// The parts of the display that contain system gestures, typically swipes | |||
/// from the edge of the screen for navigation. | |||
/// |
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.
Can you add a little more information here, e.g. that certain gestures in these areas my not be forwarded to the app and elements placed here may therefore not respond to them.
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.
Sounds good, I did the following:
/// This value is typically a drag gesture from an edge of the screen.
/// Gestures in these areas may not be forwarded to the app or may
/// be canceled by the system. Therefore, elements placed in these areas may
/// not properly respond to them.
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.
You might be able to reuse a version of #37416 (comment) here.
@@ -215,13 +215,18 @@ class MediaQueryData { | |||
/// property and how it relates to [padding] and [viewInsets]. | |||
final EdgeInsets viewPadding; | |||
|
|||
/// The parts of the display that contain system gestures, typically swipes | |||
/// from the edge of the screen for navigation. | |||
/// The parts of the display where the system may be detecting particular |
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.
Maybe:
The parts of the display where the system may be detecting certain gestures for system actions.
Those gestures are typically drags from the edge of the screen to perform system actions like opening the app drawer or the notification tray. Gestures reserved for system actions may not be forwarded to the app when performed in the area specified by these insets . Therefore, elements placed in these areas may not properly respond to them.
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 tweaked the example a little (replaced opening the app drawer with back navigation), but incorporated everything else in your suggestion
/// This value is typically a drag gesture from an edge of the screen. | ||
/// Gestures in these areas may not be forwarded to the app or may | ||
/// be canceled by the system. Therefore, elements placed in these areas may | ||
/// not properly respond to them. |
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.
We should say clearly on what systems this is used (e.g. Android starting with version Q)?
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.
That's a great idea, I just added it.
/// | ||
/// {@tool snippet --template=stateful_widget_material} | ||
/// | ||
/// When using Android Q with full gestural navigation, use |
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.
For apps that might be deployed on Android Q devices with full ...
/// Widget build(BuildContext context) { | ||
/// EdgeInsets systemGestureInsets = MediaQuery.of(context).systemGestureInsets; | ||
/// return Scaffold( | ||
/// appBar: AppBar(title: Text('Sample Gesture Nav')), |
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.
for the title, maybe 'Pad Slider to avoid systemGestureInsets'
/// EdgeInsets systemGestureInsets = MediaQuery.of(context).systemGestureInsets; | ||
/// return Scaffold( | ||
/// appBar: AppBar(title: Text('Sample Gesture Nav')), | ||
/// body: Padding( |
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.
Need a comment here to explain that, by default, the slider expands to fill the available width.
Not clear why we're padding top/bottom (we're assuming something about systemGestureInsets?). Maybe:
Container(
alignment: Alignment.center,
padding: systemGestureInsets, // only left,right padding is likely to be needed
child: Slider...
),
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 mainly pad the top/bottom to make it look cleaner in the sample app. I'll just set top and bottom to 0 for the sake of the sample to avoid confusion, which seems like a worthy tradeoff.
I think setting it to systemGestureInsets
altogether might be a little confusing since there are systemGestureInsets on the top and bottom, but for this example, the Slider is in the body of the Scaffold and does not need to be padded by those values.
@@ -91,6 +93,7 @@ void main() { | |||
expect(copied.padding, const EdgeInsets.all(9.10938)); | |||
expect(copied.viewPadding, const EdgeInsets.all(11.24031)); | |||
expect(copied.viewInsets, const EdgeInsets.all(1.67262)); | |||
expect(copied.systemGestureInsets, const EdgeInsets.all(1.5556)); |
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.
Separate PR: define these double values with a consts and explain why we're using strange values.
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.
Filed #39012
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
/// return Scaffold( | ||
/// appBar: AppBar(title: Text('Pad Slider to avoid systemGestureInsets')), | ||
/// body: Padding( | ||
/// padding: EdgeInsets.fromLTRB( // only left and right padding are likely to be needed |
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.
In this case you could use EdgeInsets.only
/// | ||
/// For apps that might be deployed on Android Q devices with full gesture | ||
/// navigation enabled, use [MediaQuery.systemGestureInsets] with [Padding] | ||
/// to avoid overlapping a [Slider] with system gesture navigation. |
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.
To avoid having the left and right edges of the slider appear within the area reserved for system gesture navigation.
/// and may not be delivered to the app. Taps and swipe gestures that begin | ||
/// with a long-press are delivered to the app, but simple press-drag-release | ||
/// swipe gestures which begin within the area defined by [systemGestureInsets] | ||
/// may not. |
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.
may not => may not be
/// may not. | ||
/// | ||
/// Apps should avoid locating gesture detectors within the system gesture | ||
/// insets area. There is no reason to avoid putting visual elements within |
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.
[my fault] "There is no reason" is probably overstating it. Maybe: Apps should feel free to put visual elements within
Description:
This passes gestural navigation inset information to the
MediaQuery.systemGestureInsets
. This becomes necessary as Flutter app developers want to take advantage of their entire screen with full gestural navigation mode coming with Q. This allows Flutter developers to determine the minimum distance from each edge needed for any swipe-able widgets like sliders.Related Issues
Related to #32748, #34678
Depends on flutter/engine#10413
Tests
I added the following tests:
copyWith
and defaulting to source.Checklist
Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes (
[x]
). This will ensure a smooth and quick review process.///
).flutter analyze --flutter-repo
) does not report any problems on my PR.Breaking Change
Does your PR require Flutter developers to manually update their apps to accommodate your change?