Add fullscreen padding workarounds to v2 android embedding #18193
Conversation
| // Decide if we want to zero the padding of the sides. When in Landscape orientation, | ||
| // android may decide to place the software navigation bars on the side. When the nav | ||
| // bar is hidden, the reported insets should be removed to prevent extra useless space | ||
| // on the sides. |
There was a problem hiding this comment.
Does this mirror an Android API? If so, consider adding a See: link or identifier names to help readers link the Flutter/Android concepts/APIs.
There was a problem hiding this comment.
This information seems to be intentionally hidden from us to prevent people from locking users out of their phone by preventing touches to the navbar
| @@ -381,6 +383,68 @@ protected void onSizeChanged(int width, int height, int oldWidth, int oldHeight) | |||
| sendViewportMetricsToFlutter(); | |||
| } | |||
|
|
|||
| // TODO(garyq): Add support for notch cutout API | |||
There was a problem hiding this comment.
Include a link to the bug tracking this work.
| BOTH | ||
| } | ||
|
|
||
| ZeroSides calculateShouldZeroSides() { |
There was a problem hiding this comment.
What visibility should this and ZeroSides have? I'd probably shoot for private rather than expose this utility method until we have people asking us to do so.
Since this class is non-final, and this is used from onApplyWindowInsets below, we should probably require that any overrides call super.onApplyWindowSets() (if that's reasonable -- I admit I didn't read through all the code) or expect that one day someone will want us to expose this publicly.
There was a problem hiding this comment.
Ahh yes, these should definitely be private. Not sure why I didn't make them private in the original code.
| // can be used. | ||
| @TargetApi(20) | ||
| @RequiresApi(20) | ||
| int calculateBottomKeyboardInset(WindowInsets insets) { |
There was a problem hiding this comment.
Similar comment to calculateShouldZeroSides with respect to visibility.
| return ZeroSides.NONE; | ||
| } | ||
|
|
||
| // TODO(garyq): Use clean ways to detect keyboard instead of heuristics if possible |
There was a problem hiding this comment.
FWIW, you can update this todo too for the new getInsets IME API
| // can be used. | ||
| @TargetApi(20) | ||
| @RequiresApi(20) | ||
| private int calculateBottomKeyboardInset(WindowInsets insets) { |
There was a problem hiding this comment.
can we call this guessBottomKeyboardInset to reflect the certainty? :)
|
This PR needs tests. Fortunately everything is mockable in robolectric and should have some examples in FlutterViewTest.java |
| Activity activity = (Activity) getContext(); | ||
| int orientation = activity.getResources().getConfiguration().orientation; | ||
| int rotation = activity.getWindowManager().getDefaultDisplay().getRotation(); | ||
| Context context = getContext(); |
There was a problem hiding this comment.
This avoids casting to Activity which can be unsafe
|
Excellent tests. LGTM |
| .getDefaultDisplay()); | ||
| display.setRotation(1); | ||
| assertEquals(0, flutterView.getSystemUiVisibility()); | ||
| when(flutterView.getWindowSystemUiVisibility()) |
There was a problem hiding this comment.
are you sure this works? You're altering the response of the spy instance but I'm assuming FlutterView is calling getWindowSystemUiVisibility() against itself internally and not the spy.
There was a problem hiding this comment.
It seems like it does. It is getting the correct values internally, and executing the right branches of code. Removing this breaks it.
| when(flutterEngine.getRenderer()).thenReturn(flutterRenderer); | ||
|
|
||
| // When we attach a new FlutterView to the engine without any system insets, the viewport | ||
| // metrics |
GaryQian
added
the
waiting for tree to go green
|
This pull request is not suitable for automatic merging in its current state.
|
fluttergithubbot
removed
the
waiting for tree to go green
|
Manually landing, Mac iOS engine passed on rerun: https://ci.chromium.org/p/flutter/builders/try/Mac%20iOS%20Engine/2214 |
These workarounds are necessary to maintain the correct behavior for padding when fullscreen.
The v1 embedding had it, but the workarounds were not moved to the v2 embedding. This PR restores the workarounds to the v2 embedding to fix the regression.
Fixes flutter/flutter#56449