Fix Top, Left, and Right padding for fullscreen android apps.#6282
Fix Top, Left, and Right padding for fullscreen android apps.#6282GaryQian merged 2 commits intoflutter:masterfrom
Conversation
|
Fixes: |
GaryQian
changed the title
|
@GaryQian I worked around some similar issues for accessibility here: https://github.com/flutter/engine/blob/master/shell/platform/android/io/flutter/view/AccessibilityBridge.java#L635 I would verify that these don't interact poorly - maybe we could ditch my hack? |
GaryQian
force-pushed
the
paddingfix
branch
from
67f051a to
c8ef33a
Compare
|
I added a distinction between API < 23 always having navbar on right (if it is on the sides at all) and API >= 23 placing the navbar on the "bottom" of the device (left or right side depending on orientation) I think as of now, the hacks are still necessary as the android API does not seem to provide clean ways to do it. |
| super.onSizeChanged(width, height, oldWidth, oldHeight); | ||
| } | ||
|
|
||
| // Decide if we want to zero the sides to remove padding for hidden navbar. |
There was a problem hiding this comment.
Add a bit more detail. Specifically: 'zero the sides' -> 'zero the left/right-side padding of the window when the system navbar is hidden'.
|
|
||
| // Decide if we want to zero the sides to remove padding for hidden navbar. | ||
| enum ZeroSides { NONE, LEFT, RIGHT, BOTH } | ||
| ZeroSides calculateShouldZeroSides(WindowInsets insets) { |
There was a problem hiding this comment.
I assume we'll need to update this code to handle the new cutout APIs. If that's the case, please add a TODO to help out the future implementor while you're here.
| } | ||
|
|
||
| // This callback is not present in API < 20, which means lower API devices will see | ||
| // worse/improper padding as of now when the status and/or nav bar are hidden. |
There was a problem hiding this comment.
s/worse\/// and s/as of now // since it's unclear when 'now' is to the reader.
| } | ||
|
|
||
| // This callback is not present in API < 20, which means lower API devices will see | ||
| // worse/improper padding as of now when the status and/or nav bar are hidden. |
There was a problem hiding this comment.
Could we preserve the existing behaviour for API < 20? We do something along those lines in the iOS code.
There was a problem hiding this comment.
The behavior for API < 20 is already what we do now. Since this method is never called, the changes made here have no impact on older devices.
| zeroSides = calculateShouldZeroSides(insets); | ||
| } | ||
|
|
||
| mMetrics.physicalPaddingTop = !statusBarVisible ? 0 : insets.getSystemWindowInsetTop(); |
There was a problem hiding this comment.
Uber-nitpick: I'd invert the condition to save readers one brain operation.
| // Bottom system inset (keyboard) should adjust scrollable bottom edge (inset). | ||
| mMetrics.physicalViewInsetTop = 0; | ||
| mMetrics.physicalViewInsetRight = 0; | ||
| // TODO(garyq): Detect and distinguish between bottom nav padding and keyboard padding. |
There was a problem hiding this comment.
Thanks for adding this. It's surprising to me that Android doesn't include specific APIs for getting the keyboard frame or even height (or perhaps I'm searching for the wrong things) :(
There was a problem hiding this comment.
It seems that the way most people do it is through heuristics. They compare the height of the padding to the height of the screen and over a certain amount is considered a keyboard.
|
LGTM on the logic. Just a couple minor nits/questions. |
GaryQian
force-pushed
the
paddingfix
branch
from
6f3a5c2 to
502e3bb
Compare
flutter/engine@2e8e96f...f3d51b0 git log 2e8e96f..f3d51b0 --no-merges --oneline f3d51b0 Fix Top, Left, and Right padding for fullscreen android apps. (flutter/engine#6282) 7ec1d21 Compute maxIntrinsicWidth based on the width of styled runs added to the line breaker (flutter/engine#6281) 2b25fd7 Don't use unix or win namespaces (flutter/engine#6277) 60c5bb8 Return null instead of throwing if an instance method is passed to PluginUtilities.getCallbackHandle (flutter/engine#6260) 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, who should be CC'd on the roll, and stop the roller if necessary.
flutter/engine@2e8e96f...1501359 git log 2e8e96f..1501359 --no-merges --oneline 1501359 Roll src/third_party/skia 1d6281d4bb47..9369031fde2d (27 commits) (flutter/engine#6284) f3d51b0 Fix Top, Left, and Right padding for fullscreen android apps. (flutter/engine#6282) 7ec1d21 Compute maxIntrinsicWidth based on the width of styled runs added to the line breaker (flutter/engine#6281) 2b25fd7 Don't use unix or win namespaces (flutter/engine#6277) 60c5bb8 Return null instead of throwing if an instance method is passed to PluginUtilities.getCallbackHandle (flutter/engine#6260) 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, who should be CC'd on the roll, and stop the roller if necessary.
flutter/engine@2e8e96f...71457a6 git log 2e8e96f..71457a6 --no-merges --oneline 71457a6 Roll dart to c688d0c0c3ad3dece3a79ce0e115d787a94707ea. (flutter/engine#6285) 1501359 Roll src/third_party/skia 1d6281d4bb47..9369031fde2d (27 commits) (flutter/engine#6284) f3d51b0 Fix Top, Left, and Right padding for fullscreen android apps. (flutter/engine#6282) 7ec1d21 Compute maxIntrinsicWidth based on the width of styled runs added to the line breaker (flutter/engine#6281) 2b25fd7 Don't use unix or win namespaces (flutter/engine#6277) 60c5bb8 Return null instead of throwing if an instance method is passed to PluginUtilities.getCallbackHandle (flutter/engine#6260) 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, who should be CC'd on the roll, and stop the roller if necessary.
flutter/engine@2e8e96f...2302bea git log 2e8e96f..2302bea --no-merges --oneline 2302bea Roll src/third_party/skia 9369031fde2d..c9873a5f8b8e (11 commits) (flutter/engine#6287) 71457a6 Roll dart to c688d0c0c3ad3dece3a79ce0e115d787a94707ea. (flutter/engine#6285) 1501359 Roll src/third_party/skia 1d6281d4bb47..9369031fde2d (27 commits) (flutter/engine#6284) f3d51b0 Fix Top, Left, and Right padding for fullscreen android apps. (flutter/engine#6282) 7ec1d21 Compute maxIntrinsicWidth based on the width of styled runs added to the line breaker (flutter/engine#6281) 2b25fd7 Don't use unix or win namespaces (flutter/engine#6277) 60c5bb8 Return null instead of throwing if an instance method is passed to PluginUtilities.getCallbackHandle (flutter/engine#6260) 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, who should be CC'd on the roll, and stop the roller if necessary.
5 participants
The android WindowInsets does not take into account status and nav bars that are hidden. This change ensures that the top, left, and right edges have properly zeroed padding when it is necessary to correct for the hidden bars.
Top is zeroed when the status bar is hidden (called "Fullscreen" in android)
Left or Right or Both are zeroed when the navigation bar would appear on the respective sides when the device is in landscape mode.
This patch does not handle bottom padding when in portrait mode or landscape mode yet due to the issues with providing proper insets for on-screen keyboards.