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

Differentiate between Window padding and margins in dart:ui #12895

Closed
cbracken opened this issue Nov 6, 2017 · 8 comments
Closed

Differentiate between Window padding and margins in dart:ui #12895

cbracken opened this issue Nov 6, 2017 · 8 comments
Assignees
Labels
engine flutter/engine repository. See also e: labels. framework flutter/packages/flutter repository. See also f: labels.

Comments

@cbracken
Copy link
Member

cbracken commented Nov 6, 2017

There are two different sets of view insets that applications may want
to track in order to avoid unwanted interaction with system UI:

  1. OS UI that effectively shrinks the Flutter view from a UX point of
    view: e.g., when the keyboard opens, it occludes the bottom of the
    screen and the view should be adjusted such that the bottom, for the
    purposes of scrolling is just above the keyboard.
  2. OS UI that is overlaid over the application, but into which the
    application should draw. e.g., the Home indicator on the iPhone X
    typically appears near the bottom of the screen, overlaid over app
    content. Content should be rendered within this 'safe area' but apps
    should avoid requiring user interaction there. For example, list
    views may want to include some small amount of additional padding to
    ensure the last list item can scroll above this area.

Flutter does not currently distinguish between these two cases. For example, padding is used to:

  1. 'Shrink' the scrollable view area in the scaffold when the keyboard is opened.
  2. Add additional padding into which we draw for the iOS status bar.

We should track these insets separately:

  1. margin: a set of view insets that describe areas which apps should consider occluded by system UI (e.g. keyboard).
  2. padding: a set of insets (relative to the inner edge of the margin) that describe areas that the app should consider visible/drawable, but within which apps should avoid requiring user interaction, since it may be hijacked by the OS, such as near the iPhone X home indicator.
@cbracken cbracken self-assigned this Nov 6, 2017
@cbracken cbracken added device: iphone-x engine flutter/engine repository. See also e: labels. framework flutter/packages/flutter repository. See also f: labels. labels Nov 6, 2017
@Hixie Hixie added this to the 3: Current Milestone milestone Nov 7, 2017
@sroddy
Copy link
Contributor

sroddy commented Nov 9, 2017

If I understand correctly, the change you propose will break BC of a lot of widgets (internal and external).

In my mind there are 3 possible use cases for this API:

  1. Dev wants to entirely cover the screen
  2. Dev wants to layout something above the iPhone X soft home button (but have the keyboard cover it)
  3. Dev wants to layout something in a way that is always possible for the User to view/interact (so either above the keyboard frame or above the soft home button, depending on which one is bigger)

With you current API proposal it would be tricky to reach goal 2 because I expect the bottom padding to become 0 when the keyboard is shown (otherwise it wouldn't be anymore relative to the inner edge of the margin).

On the other hand if you decide to keep the padding always to the same value when the keyboard is open, the Dev would need to perform some math in order to reach goal 3 (because (s)he would have to sum the padding of the window only when the keyboard is not shown) . E.g. see this SO question: https://stackoverflow.com/questions/45399178/extend-ios-11-safe-area-to-include-the-keyboard.

I think that a better approach for this API would be to provide a way to reach goals 1, 2 and 3 without requiring additional efforts. Maybe something like margin for use case 2 and safeArea for use case 3 (which is the max between margin and the keyboard height.

This way you will be BC in a way that will require Dev intervention (in most of the cases they will just have to replace padding with safeArea but they can check easily every case).

I hope I was clear enough.

@cbracken
Copy link
Member Author

cbracken commented Nov 9, 2017

@sroddy thanks for the feedback!

With you current API proposal it would be tricky to reach goal 2 because I expect the bottom padding to become 0 when the keyboard is shown

Just to clear up what might be a misunderstanding: the intent here is not to blindly apply the iOS safe areas as additional padding within the margin, it's to do the appropriate rect intersections in the engine such that the correct margin and padding are exposed to the framework and developers without any additional work necessary from widget authors.

Today, the only known use-case for margin is for the insets (the bottom inset) caused by an open keyboard view. In the case where the keyboard is closed, the behaviour remains identical to today. In the case where the keyboard is open, the keyboard height will be exposed as margin instead of padding and bottom padding will be zero, since the bottom safe area is 'under' the keyboard (i.e. within the margin. Terrible ASCII-art rendition:

+------------------------+ <-- top margin: 0
|     top safe area      | <-- top padding: height of safe area
+------------------------+
|                        |
|                        |
|                        |
|                        |
|                        |
|                        |
|                        |
|                        |
|                        |
|                        |
+------------------------+ <-- bottom padding: 0
|                        |
|                        |
|     open keyboard      |
|                        | <-- bottom margin: height of keyboard
|                        |
+------------------------+
|    bottom safe area    |
+------------------------+

And a similarly terrible rendition with the keyboard closed:

+------------------------+ <-- top margin: 0
|     top safe area      | <-- top padding: height of safe area
+------------------------+
|                        |
|                        |
|                        |
|                        |
|                        |
|                        |
|                        |
|                        |
|                        |
|                        |
|                        |
|                        |
|                        |
|                        |
|                        |
|                        |
+------------------------+
|    bottom safe area    | <-- bottom padding: height of safe area
+------------------------+ <-- bottom margin: 0

This fixes an inconsistency in the framework today. Bottom padding is treated differently from all other padding. Specifically, in Scaffold, bottom padding is used to reduce the scaffold body height by the height of the keyboard; i.e., effectively positions the bottom of the body at the top edge of the keyboard to allow the user to scroll the entire view without it being under the keyboard. Top padding, on the other hand, is treatable as a drawable area (e.g., under the status bar) under which you want additional padding so that widgets the user may need to interact with aren't blocked by system chrome.

As part of this change, Scaffold will be updated to use margin to reduce its body height when then keyboard is up, rather than padding. For apps using Scaffold, things should 'just work' as-is. For those not using it and doing their own keyboard handling, you're correct that this is a breaking change, and definitely warrants a breaking change announcement.

That said, @Hixie and I were just discussing a case the current design doesn't account for. I'll add it in a followup comment momentarily, since this one is now pretty huge :)

@cbracken
Copy link
Member Author

cbracken commented Nov 9, 2017

The case this design doesn't cover is a Scaffold where resizeToAvoidBottomPadding is false instead of the default true. Today that means 'ignore bottom padding'. Under the proposed design, it would mean 'ignore bottom margin'.

Imagine such a scaffold where the body is a SafeArea containing a Placeholder running on an iPhone X -- which has a top safe area for the status bar, and a bottom safe area for the home indicator.

Under the proposed design, the placeholder will have a height equal to the view height minus top padding (status bar) minus bottom padding (home indicator). The problem is that when the keyboard is closed, bottom padding == safe Area height. When the keyboard is open, the proposed design specifies that padding goes to zero, which will result in the placeholder getting taller.

The goal with using a margin + inner padding model is to save widget implementors from doing the intersections themselves by doing it ourselves in the engine. That said, the above example demonstrates a case where an app developer benefits from having direct access to both types of inset relative to the view (rather than relative to each other).

The end result is likely that I'll adapt the current prototype implementation to use two sets of view-relative insets and add an additional framework widget (or adapt safearea) to handle both types.

Thanks again for prompting this discussion and for your feedback and input!

@sroddy
Copy link
Contributor

sroddy commented Nov 9, 2017

@cbracken Exactly! The Scaffold where resizeToAvoidBottomPadding is false is an example of the use case 2 I mentioned above.

The thing I was proposing for the API is to expose two "computed" values but in a way that imho would be more pragmatic for the developer:

  1. A property (call it padding or margin or anything else :) ) which is a "static" representation of the safe area of the screen (e.g. it accounts for the notch and the soft home button) and doesn't change when the keyboard opens. This will be perfect to handle case 2.
  2. A property (call it safeArea or anything else) which is the max between the one above and the keyboard and would be perfect for handling case 3 because you wouldn't need any other computation to achieve the expected goal of not placing the element below anything "external" that can cover it.

I would personally advise against using terms like padding and margin because they have a very precise meaning in the heads of developers and designers and would probably generate some confusion.

@cbracken
Copy link
Member Author

cbracken commented Nov 9, 2017

Yep. Given the discussion, we won't use margin. The original intent was to present these as something akin to the web concept of margin and padding, but as we're likely to have two view-relative sets of insets, we'll need a different name as you point out.

@cbracken
Copy link
Member Author

cbracken commented Dec 9, 2017

Work is now complete in the engine and framework.

MediaQuery.of(context).padding exposes safe areas. MediaQuery.of(context).viewInsets exposes view insets (today this is used for the keyboard only).

Engine work for iOS is complete.

For Android it may be useful to consider the desired behaviour when the system Navigation Bar (back/home/app switcher) is translucent and make similar adaptations. Similarly, the Essential phone allows for a translucent status bar (though I believe it requires whitelisting with Essential).

@cbracken
Copy link
Member Author

This is now also complete for Android.

@github-actions
Copy link

github-actions bot commented Sep 4, 2021

This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of flutter doctor -v and a minimal reproduction of the issue.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 4, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
engine flutter/engine repository. See also e: labels. framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
Development

No branches or pull requests

3 participants