Navigation Menu

Skip to content
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

Merged

Conversation

shihaohong
Copy link
Contributor

@shihaohong shihaohong commented Aug 1, 2019

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:

  • Updated existing tests such as 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.

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I signed the CLA.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I updated/added relevant documentation (doc comments with ///).
  • All existing and new tests are passing.
  • The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require Flutter developers to manually update their apps to accommodate your change?

  • Yes, this is a breaking change (Please read Handling breaking changes). Replace this with a link to the e-mail where you asked for input on this proposed change.
  • No, this is not a breaking change.

@shihaohong shihaohong added c: new feature Nothing broken; request for a new capability platform-android Android applications specifically framework flutter/packages/flutter repository. See also f: labels. work in progress; do not review customer: crowd Affects or could affect many people, though not necessarily a specific customer. labels Aug 1, 2019
@shihaohong shihaohong changed the title (WIP) Add MediaQuery.systemGestureInsets (WIP) Add MediaQuery.systemGestureInsets to support Android Q Full Gestural Navigation Aug 1, 2019
@shihaohong
Copy link
Contributor Author

I'm currently working on the engine counterpart of this PR, so until that lands, this PR will remain a work in progress!

@shihaohong
Copy link
Contributor Author

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.

@shihaohong shihaohong closed this Aug 14, 2019
@shihaohong shihaohong reopened this Aug 20, 2019
@shihaohong shihaohong changed the title (WIP) Add MediaQuery.systemGestureInsets to support Android Q Full Gestural Navigation Add MediaQuery.systemGestureInsets to support Android Q Full Gestural Navigation Aug 20, 2019
@shihaohong shihaohong changed the title Add MediaQuery.systemGestureInsets to support Android Q Full Gestural Navigation Add MediaQuery.systemGestureInsets to support Android Q Aug 20, 2019
@shihaohong shihaohong changed the title Add MediaQuery.systemGestureInsets to support Android Q (WIP) Add MediaQuery.systemGestureInsets to support Android Q Aug 20, 2019
@shihaohong shihaohong changed the title (WIP) Add MediaQuery.systemGestureInsets to support Android Q Add MediaQuery.systemGestureInsets to support Android Q Aug 20, 2019
Copy link
Member

@goderbauer goderbauer left a 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
Copy link
Member

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?

Copy link
Contributor Author

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

Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Member

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

Copy link
Contributor Author

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"

Copy link
Contributor

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.

Copy link
Contributor Author

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.
///
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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
Copy link
Member

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.

Copy link
Contributor Author

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.
Copy link
Member

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)?

Copy link
Contributor Author

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
Copy link
Contributor

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')),
Copy link
Contributor

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(
Copy link
Contributor

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...
),

Copy link
Contributor Author

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));
Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Filed #39012

Copy link
Contributor

@HansMuller HansMuller left a 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
Copy link
Contributor

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.
Copy link
Contributor

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.
Copy link
Contributor

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
Copy link
Contributor

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

@shihaohong shihaohong merged commit 3b74104 into flutter:master Aug 22, 2019
@shihaohong shihaohong deleted the android-q-system-gesture-navigation branch September 10, 2019 16:20
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 4, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
c: new feature Nothing broken; request for a new capability customer: crowd Affects or could affect many people, though not necessarily a specific customer. framework flutter/packages/flutter repository. See also f: labels. platform-android Android applications specifically
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants