Page jumps around/flickers when keyboard appears or disappears

RESOLVED FIXED in Firefox 21

Status

()

Firefox for Android
Toolbar
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: cwiiis, Assigned: cwiiis)

Tracking

(Depends on: 1 bug)

Trunk
Firefox 21
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
When the page size changes due to the keyboard appearing or disappearing, there is often a flicker of blank, or of the page rendered too far down or up before it settles on the correct rendering.

Other GL-based apps appear able to have the keyboard appear without flickering, so this is likely fixable, at least on a subset of devices.

A definite fix would be to stop the keyboard from resizing the UI, but I'm not sure if it's possible to ascertain how much of the screen an input method covers when doing this, so I'm suggesting to fix it some other way. I have working patches, which I'll attach once I've verified and tidied them up.
(Assignee)

Comment 1

5 years ago
Created attachment 699345 [details] [diff] [review]
Fix flickering on surface resize

This splits up the resize into two phases - The resize of the UI container (where it makes sure a synchronous draw happens to prevent flickering), and the resize of the surface view (where the widget resize and surface reallocation happens).
Attachment #699345 - Flags: review?(bugmail.mozilla)
(Assignee)

Comment 2

5 years ago
I was seeing deadlock without the patch in bug #826300 - I think I fixed the cause of it, but I'm not confident it won't happen without that patch, so marking it as a dependency.
Depends on: 826300
Comment on attachment 699345 [details] [diff] [review]
Fix flickering on surface resize

Review of attachment 699345 [details] [diff] [review]:
-----------------------------------------------------------------

r- for API level requirements. I'll do a more thorough review on the next iteration since I suspect this will change a lot to work around that problem. Also it would be good to get BenWa to look at it for the compositor pieces.

::: mobile/android/base/gfx/GeckoLayerClient.java
@@ +622,5 @@
> +    public void sizeChanged(int width, int height) {
> +        // We need to make sure a draw happens synchronously at this point,
> +        // but resizing the surface before the SurfaceView has resized will
> +        // cause a visible jump.
> +        compositionResumeRequested(mWindowSize.width, mWindowSize.height);

Could this not be done with a renderRequested() call instead?

::: mobile/android/base/gfx/LayerView.java
@@ +41,5 @@
>   * A view rendered by the layer compositor.
>   *
>   * Note that LayerView is accessed by Robocop via reflection.
>   */
> +public class LayerView extends FrameLayout implements View.OnLayoutChangeListener {

View.OnLayoutChangeListener is only in API level 11 and up. We'll need to do something for gingerbread too.

@@ +341,5 @@
>          }
>      }
>  
> +    private void onSizeChanged(int width, int height) {
> +        if (mListener != null) {

I'm not sure I fully understand this method, can you add some comments for what the different cases mean? I would have thought that the mGLController.surfaceChanged call would go into the first block.
Attachment #699345 - Flags: review?(bugmail.mozilla) → review-
(Assignee)

Comment 4

5 years ago
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #3)
> ::: mobile/android/base/gfx/GeckoLayerClient.java
> @@ +622,5 @@
> > +    public void sizeChanged(int width, int height) {
> > +        // We need to make sure a draw happens synchronously at this point,
> > +        // but resizing the surface before the SurfaceView has resized will
> > +        // cause a visible jump.
> > +        compositionResumeRequested(mWindowSize.width, mWindowSize.height);
> 
> Could this not be done with a renderRequested() call instead?

No, the composition needs to happen synchronously (or there may be flicker) - renderRequested doesn't have the wait/lock on composition.

> ::: mobile/android/base/gfx/LayerView.java
> @@ +41,5 @@
> >   * A view rendered by the layer compositor.
> >   *
> >   * Note that LayerView is accessed by Robocop via reflection.
> >   */
> > +public class LayerView extends FrameLayout implements View.OnLayoutChangeListener {
> 
> View.OnLayoutChangeListener is only in API level 11 and up. We'll need to do
> something for gingerbread too.

Oh damn :| I guess we could sub-class SurfaceView and override the onLayout method instead. Little more tedious, but no biggie.

> @@ +341,5 @@
> >          }
> >      }
> >  
> > +    private void onSizeChanged(int width, int height) {
> > +        if (mListener != null) {
> 
> I'm not sure I fully understand this method, can you add some comments for
> what the different cases mean? I would have thought that the
> mGLController.surfaceChanged call would go into the first block.

Yeah, sorry, this function was the victim of iteration - it really just needs to do if (!mGLController.hasValidSurface()) mGLController.surfaceChanged(width, height); at the start, then the rest of it would read a lot more simply. I'll add comments as well though.

I'll ask for BenWa's review on the next patch.
(Assignee)

Comment 5

5 years ago
Created attachment 699891 [details] [diff] [review]
Fix flickering on surface resize v2

r?Benwa for the CompositorParent changes (remove mWidgetSize, use FrameMetrics.mCompositionBounds instead).
Attachment #699345 - Attachment is obsolete: true
Attachment #699891 - Flags: review?(bugmail.mozilla)
Attachment #699891 - Flags: review?(bgirard)
Comment on attachment 699891 [details] [diff] [review]
Fix flickering on surface resize v2

Review of attachment 699891 [details] [diff] [review]:
-----------------------------------------------------------------

Seems ok to me but the synchronous stuff scares me. I already get deadlocks all the time from bug 803154.

::: mobile/android/base/gfx/LayerView.java
@@ +415,5 @@
> +     */
> +    private class LayerSurfaceView extends SurfaceView {
> +        LayerView mParent;
> +
> +        public LayerSurfaceView(Context aContext, LayerView aParent) {

You don't really need a reference to the LayerView, private non-static classes have access to their parent instance's methods. Should be able to just call surfaceChanged() directly.
Comment on attachment 699891 [details] [diff] [review]
Fix flickering on surface resize v2

Review of attachment 699891 [details] [diff] [review]:
-----------------------------------------------------------------

Great work! If this reduces the flickering across all version this is very nice. This was a very difficult problem last time I looked pre native fennec.

r+ given some additional comment clarification testing since I've seen this behavior change between versions.

::: mobile/android/base/gfx/GeckoLayerClient.java
@@ +618,5 @@
>          }
>      }
>  
>      /** Implementation of LayerView.Listener */
> +    public void sizeChanged(int width, int height) {

I'm a fan of @Override. We get bit by having the parent signature change from time to time. Sadly this isn't enforced :(

::: mobile/android/base/gfx/LayerView.java
@@ +334,5 @@
> +     *
> +     * The second phase is the SurfaceView size change. At this point, the
> +     * backing GL surface is resized and another synchronous draw is performed.
> +     * Gecko is also sent the new window size, and this will likely cause an
> +     * extra draw a few frames later, after it's re-rendered and caught up.

It's not clear how you deal with the compositing that is already happening asynchronously on the compositor and pending updates in the Gecko thread or in transaction in the compositor's queue. Also how is the draw valid when the LayerView and the SurfaceView have a different notions of the size. Can you clarify these point?

I also found that the resize behavior varied between android 2,3,4 although I dealt with the problem very differently. Has this been tested on these versions?

@@ +415,5 @@
> +     */
> +    private class LayerSurfaceView extends SurfaceView {
> +        LayerView mParent;
> +
> +        public LayerSurfaceView(Context aContext, LayerView aParent) {

You can access all of the parent. You also don't need aContext. You can get the unambiguous instance of the enclosing class using 'LayerView.this'.
Attachment #699891 - Flags: review?(bgirard) → review+
Comment on attachment 699891 [details] [diff] [review]
Fix flickering on surface resize v2

Review of attachment 699891 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with the onLayout comments addressed and a green try run/basic testing on older android versions.

::: mobile/android/base/gfx/GeckoLayerClient.java
@@ +622,5 @@
> +    public void sizeChanged(int width, int height) {
> +        // We need to make sure a draw happens synchronously at this point,
> +        // but resizing the surface before the SurfaceView has resized will
> +        // cause a visible jump.
> +        compositionResumeRequested(mWindowSize.width, mWindowSize.height);

Just to make sure I understand this correctly: you're using mWindowSize here instead of the parameters passed in because mWindowSize reflects the size of the surface at this point (which hasn't been resized yet) whereas the parameters reflect the size of the LayerView (which has been resized), right? I think that's what your comment implies, but just double-checking.

::: mobile/android/base/gfx/LayerView.java
@@ +420,5 @@
> +            super(aContext);
> +            mParent = aParent;
> +        }
> +
> +        protected void onLayout(boolean changed, int left, int top, int right, int bottom) {

Add an @Override tag on this. Also it should be calling super.onLayout(). I'm not sure if it's a no-op or not, but it may change in the future.
Attachment #699891 - Flags: review?(bugmail.mozilla) → review+
(Assignee)

Comment 9

5 years ago
Confirmed green on try, patch works for an armv6 Android 2.3 phone (Samsung Galaxy Y, I think) as well as an Intel Android 4.0 phone (Motorola Razr i).

Pushed to inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/934e8b48a539
https://hg.mozilla.org/mozilla-central/rev/f9ef21529956
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 21
https://hg.mozilla.org/mozilla-central/rev/f9ef21529956
https://hg.mozilla.org/mozilla-central/rev/f9ef21529956
Depends on: 832173
I've seen a big improvement since this landed on GN+JB.
You need to log in before you can comment on or make changes to this bug.