Preserve safe area #8848

dnfield · 2019-05-04T14:27:43Z

On iOS today, we end up losing the system safe area dimension when the keyboard is visible. This works out for most cases, but some framework consumers still want to know the bottom padding value even if the keyboard is visible.

This is meant to be a non-breaking change that adds additional information to the window for that case. A subsequent patch to the framework will pump this through MediaQuery and look at usages for SafeArea and Scaffold etc.

cbracken

Will give a proper review today! Thanks for sending this.

/cc @Hixie for thoughts as well.

shell/platform/darwin/ios/framework/Source/FlutterViewController.mm

dnfield · 2019-05-07T03:28:39Z

The build_and_test_host failure is expected, since this will require updates to the framework to account for the new property on Window.

Hixie · 2019-05-07T16:18:07Z

this is getting quite complicated. I think we shouldn't do this unless we accompany it with detailed diagrams explaining all the cases. we also need to update media query and company.

dnfield · 2019-05-07T18:19:40Z

@Hixie - this needs to land before we can update MediaQuery etc.

For diagrams, do you think ASCII ones in the docs are good enough, or do we need actual drawings/screenshots for this? I could imagine some screenshots of where these insets are on an iPhone X would be helpful....

dnfield · 2019-05-09T02:41:07Z

I've added some ASCII diagrams inline in the docs to hopefully clarify this.

The basic thrust of this PR is that we shouldn't completely obscure the actual view padding from the framework - most applications just want to know the "padding" as we've been calculating it up until now, but sometimes you really want something positioned center screen regardless of whether they keyboard is visible and consuming that bottom padding or not.

Hixie · 2019-05-13T03:06:28Z

I meant real diagrams in the assets-for-api-docs sense. cc @gspencergoog and @Piinks who would be able to help as they have lots of experience doing those.

dnfield · 2019-05-13T03:17:14Z

One challenge with that is that assets-for-api-docs is Android based, and this change is really more geared towards iPhone X (and later models).

I could add some static images to the repo and link to them here though.

gspencergoog · 2019-05-13T15:06:39Z

We have diagrams of Cupertino widgets in the same way that we have Material widgets: Flutter doesn't actually need to be running on an iPhone to look like one. The thing that is missing is just a path that represents the notch, and a keyboard image to use as a placeholder.

dnfield · 2019-05-13T15:15:11Z

It's the keyboard and bottom soft button that impact this though. I don't think the diagram generator would capture the IME would it?

Hixie · 2019-05-13T15:49:14Z

A diagram doesn't have to literally include the IME. I just mean literally an explanation of what the values all man relative to each other. I would expect it to be schematic.

spkersten · 2019-05-16T06:45:20Z

lib/ui/window.dart

+/// represents the system keyboard, which can cover over the bottom view
+/// padding when visible.
+///
+/// The [Window.viewInsets] are the physical pixels into which the operation


typo: operating

cbracken

Just a few very minor doc nits.

cbracken · 2019-05-20T17:58:34Z

shell/platform/darwin/ios/framework/Source/FlutterViewController.mm

-  if (@available(iOS 11, *)) {
-    _viewportMetrics.physical_padding_bottom = self.view.safeAreaInsets.bottom * scale;
-  } else {
-    _viewportMetrics.physical_padding_top = [self statusBarPadding] * scale;


Interesting -- this looks like an oversight in the pre-existing code. Oops. Thanks for fixing.

cbracken · 2019-05-20T18:00:54Z

lib/stub_ui/lib/src/ui/window.dart

+  /// viewPadding and the padding will be equal, as there are no insets at play.
+  /// In this scenario, the [Window.viewInsets.bottom] will be 0, and the
+  /// [Window.padding.bottom] and [Window.viewPadding.bottom] will be some
+  /// positive number specified by the OS:


uber-nit: I wonder if s/OS/platform/ might be more accurate to account for things like web vs desktop etc.

These docs are going to be removed.

cbracken · 2019-05-20T18:02:25Z

lib/stub_ui/lib/src/ui/window.dart

+  ///  ╲▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁╱   _____________________________________
+  /// ```
+  ///
+  /// Whereas when the keyboard is showing, the viewPadding and padding will be


nit: starting with 'Whereas' seems wrong since there's no comparison within the sentence.

cbracken · 2019-05-20T18:05:10Z

shell/platform/darwin/ios/framework/Source/FlutterViewController.mm

-  // keyboard height. We also eliminate any bottom safe-area padding since they keyboard 'consumes'
-  // the home indicator widget.
+  // keyboard height. The Dart side will compute a value accounting for the keyboard-consuming
+  // bottom padding.


Consider re-wording as: "The framework is responsible for computing a value accounting for keyboard-consuming bottom padding." since, in theory it's possible for another framework to be written on top of the engine.

This is done in dart:ui though, not the framework.

Hixie · 2019-05-20T19:29:51Z

lib/stub_ui/lib/src/ui/window.dart

+  /// ┃                    ┃  _____________________________________
+  /// ┃      ━━━━━━━━      ┃  viewPadding.bottom == padding.bottom
+  ///  ╲▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁╱   _____________________________________
+  /// ```


ASCII art looks unprofessional, we should use real diagrams. (I believe we have some now yes?)

This was meant to be removed

Hixie · 2019-05-20T19:30:18Z

Tests?

Hixie · 2019-05-20T19:31:08Z

Assuming the documentation looks professional and the feature is tested, I have no objections. I defer to someone else for detailed review.

dnfield · 2019-05-22T05:28:54Z

Added tests.

goderbauer · 2019-05-29T22:48:17Z

lib/stub_ui/lib/src/ui/window.dart

  ///    design applications.
  WindowPadding get viewInsets => WindowPadding.zero;

+  WindowPadding get viewPadding => _viewPadding;


Why not just return WindowPadding.zero like viewInsets above?

goderbauer · 2019-05-29T22:48:58Z

lib/stub_ui/lib/src/ui/window.dart

  ///    design applications.
  WindowPadding get viewInsets => WindowPadding.zero;

+  WindowPadding get viewPadding => _viewPadding;


Doc comment?

stub_ui is going to have all its docs stripped.

goderbauer · 2019-05-29T23:01:52Z

lib/ui/window.dart

+/// padding when visible.
+///
+/// The [Window.viewInsets] are the physical pixels into which the operating
+/// system may place system UI, such as the keyboard, which would fully obscure


may place or did place? In your diagram it looks like this only has a value != 0 when the keyboard is actually up and the screen is obscured.

This was existing documentation. Maybe better to say "the system reserves for system UI"?

goderbauer · 2019-05-29T23:03:29Z

lib/ui/window.dart

+/// any content drawn in that area.
+///
+/// The [Window.viewPadding] are the physical pixels on each side of the display
+/// that may be partially obscured by system UI or by physical intrusions into


It is not really clear what system UI affects the viewInsets and what the viewPadding. This makes it hard to understand what the difference between the two really is.

Added some prose for this.

goderbauer · 2019-05-29T23:04:44Z

lib/ui/window.dart

+/// e.g. if you wish to draw a widget at the center of the screen with respect
+/// to the iPhone "safe area" regardless of whether the keyboard is showing.
+///
+/// Clients that want to position elements or detect gestures relative to the


Not sure what this paragraph is saying?

Rewrote it entirely.

goderbauer

LGTM

goderbauer · 2019-05-29T23:34:03Z

lib/ui/window.dart

+/// represents the system keyboard, which can cover over the bottom view
+/// padding when visible.
+///
+/// The [Window.viewInsets] are the physical pixels into which the operating


grammar: ~~into~~ which the OS reserves for system UI?

goderbauer · 2019-05-29T23:35:41Z

lib/ui/window.dart

+/// phone. Unlike the insets, these areas may have portions that show the user
+/// application painted pixels without being obscured, such as a notch at the
+/// top of a phone that covers only a subset of the area. Insets, on the other
+/// hand, the insets either partially or fully obscured, such as as opaque


Insets, on the other hand, the insets <- is something missing here?

dnfield · 2019-05-30T00:22:12Z

Updated docs. I'll hold off on merging this until the engine has rolled into the framework - autoroller is backed up by a few days now.

dnfield · 2019-05-31T16:24:22Z

Infra failure was a flake. Going to submit this on red (failing host test is expected because of framework updates needed).

flutter/engine@f867609...a32df2c git log f867609..a32df2c --no-merges --oneline a32df2c Roll src/third_party/skia da95a75be1dd..c4fec06e5a3b (1 commits) (flutter/engine#9154) 79c6ce1 Preserve safe area (flutter/engine#8848) cbef680 Roll src/third_party/skia b9658153032a..da95a75be1dd (2 commits) (flutter/engine#9153) The AutoRoll server is located here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md If the roll is causing failures, please contact the current sheriff ([email protected]), and stop the roller if necessary.

Preserve safe area on Window regardless of insets.

dnfield added 2 commits May 4, 2019 07:11

dart interface

a682828

remove specialization in embedder

8d9d46f

dnfield requested a review from cbracken May 4, 2019 14:27

googlebot added cla: yes labels May 4, 2019

update ui

515b29d

cbracken reviewed May 6, 2019

View reviewed changes

shell/platform/darwin/ios/framework/Source/FlutterViewController.mm Outdated Show resolved Hide resolved

Update FlutterViewController.mm

1af35f1

dnfield requested a review from Hixie May 7, 2019 13:43

yuridiniz mentioned this pull request May 7, 2019

PlatformView crash when it is displayed on cutout / notch screens flutter/flutter#32100

Closed

dnfield and others added 9 commits May 7, 2019 16:15

Merge branch 'master' into media_query

423354c

diagram and remove unused var

302121a

Merge branch 'media_query' of github.com:dnfield/engine into media_query

d4cb67f

diagram

e3533b6

merge

4d51334

put diagram in right file

c7749c5

space

64d65ec

re-apply lints

3e3e098

fix diagram for stub_ui

f98a2bc

dnfield force-pushed the media_query branch from 98106b7 to f98a2bc Compare May 9, 2019 02:39

dnfield added a commit to dnfield/assets-for-api-docs that referenced this pull request May 14, 2019

Animation for flutter/engine#8848

eceabdf

spkersten reviewed May 16, 2019

View reviewed changes

typos

5857dab

cbracken reviewed May 20, 2019

View reviewed changes

Hixie reviewed May 20, 2019

View reviewed changes

dnfield added 2 commits May 21, 2019 22:11

remove stub_ui docs

c534f2c

tests

d905de4

dnfield requested review from cbracken and goderbauer May 28, 2019 20:39

goderbauer reviewed May 29, 2019

View reviewed changes

update docs

3d534a1

goderbauer approved these changes May 29, 2019

View reviewed changes

..

3350812

dnfield merged commit 79c6ce1 into flutter:master May 31, 2019

dnfield deleted the media_query branch May 31, 2019 16:24

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 31, 2019

79c6ce1 Preserve safe area (flutter/engine#8848)

4053961

engine-flutter-autoroll mentioned this pull request May 31, 2019

Roll engine f867609c01a9..79c6ce19a1e9 (2 commits) flutter/flutter#33664

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 31, 2019

79c6ce1 Preserve safe area (flutter/engine#8848)

2c00c21

engine-flutter-autoroll mentioned this pull request May 31, 2019

Roll engine f867609c01a9..a32df2c92800 (3 commits) flutter/flutter#33667

Merged

This was referenced Jun 6, 2019

Updating MediaQuery with viewPadding flutter/flutter#33999

Merged

Preserving SafeArea : Part 2 flutter/flutter#34298

Merged

huqiuser pushed a commit to huqiuser/engine that referenced this pull request Jun 12, 2019

Preserve safe area (flutter#8848)

66f2834

Preserve safe area on Window regardless of insets.

Piinks mentioned this pull request Aug 5, 2019

Flutter doesn't have a way to handle bottom view insets/notches on Android devices flutter/flutter#35404

Closed

Piinks mentioned this pull request Jun 11, 2020

Interactive Scaffold elements should respect SafeArea by default flutter/flutter#59272

Closed

Preserve safe area #8848

Preserve safe area #8848

Uh oh!

Conversation

dnfield commented May 4, 2019

Uh oh!

cbracken left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dnfield commented May 7, 2019

Uh oh!

Hixie commented May 7, 2019

Uh oh!

dnfield commented May 7, 2019

Uh oh!

dnfield commented May 9, 2019

Uh oh!

Hixie commented May 13, 2019

Uh oh!

dnfield commented May 13, 2019

Uh oh!

gspencergoog commented May 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dnfield commented May 13, 2019

Uh oh!

Hixie commented May 13, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cbracken left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hixie commented May 20, 2019

Uh oh!

Hixie commented May 20, 2019

Uh oh!

dnfield commented May 22, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

goderbauer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

gspencergoog commented May 13, 2019 •

edited

Loading