976531 - Keyboard lowering causes a re-draw that breaks page layout.

Status: RESOLVED WORKSFORME

(Firefox for Android Graveyard :: Toolbar, defect, P5)

Description

[Stuart Colville [:muffinresearch]]

This is specific to marketplace see bug 972532 for steps to reproduce.

It appears to be a platform issue that causes the page layout to be broken following the keyboard being lowered. This is reproducible in currently nightly (30.0a1), (aurora (29.0a2) and stable (27) albeit with slightly different symptoms.

In stable + aurora the layout is noticeably zoomed after clicking the button whereas in nightly the behaviour is slightly different; the bottom of the 'viewport' is not drawn and clicking the input then results in the same 'zoomed' behaviour seen in stable.

Videos to follow.

I've put this in general as I'm not sure which component is the best fit.

Comment 1

[Stuart Colville [:muffinresearch]]

Attachment: Video showing symptoms in stable (aurora is the same)

Comment 2

[Stuart Colville [:muffinresearch]]

Attachment: Video showing symptoms in nightly

Comment 3

[Wesley Johnston (:wesj)]

Attachment: Patch

We've always wated to remove this reflow when the keyboard appears and just draw the window behind the keyboard, but we don't know the size of the keyboard making it pretty tough.

This is sorta an alternate approach. We still resize our Native window, but instead of resizing the browser window, we try to just adjust the margins around it. I determined if the resize was due to the keyboard by using the looking at events where only y is resized.

This likely has tons of problems, but wanted some feedback/reviews.

Comment 4

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8386443 [details] [diff] [review]
Patch

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

It's been a while since I've dabbled in this code so I don't know how useful my review is going to be here. Chris can probably comment more usefully, specially on the use of margins to accomplish this effect (which seems weird to me but perhaps might work). Bouncing review over to him.

Comment 5

[Chris Lord [:cwiiis]]

Comment on attachment 8386443 [details] [diff] [review]
Patch

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

I think the approach could work, but there are too many questions for me to r+ this and I'm pretty sure it could be broken via either rotating, or a specially crafted page (or both).

::: mobile/android/base/gfx/GeckoLayerClient.java
@@ +246,5 @@
>              return;
>          }
>  
> +        // If only the screen height is changing, this is probably the keyboard appearing.
> +        // Just add/remove a margin to the viewport to account for the missing space

This comment needs to be more comprehensive if this code is going to be committed. I'm also not confident that setting margins like this will necessarily work correctly...

What happens when focusing entries either at the very top or bottom of the page? And what if there's fixed position content at the top/bottom of the page? What if the page changes size while the keyboard is up? And if you rotate while the keyboard is up, then hide the keyboard?

Comment 6

[Andy McKay]

Any progress on this, anything we can do to help?

Comment 7

[Wesley Johnston (:wesj)]

This patch did cause widespread bustage. No one working on this right now though. Pings help keep it on the radar. :) I'll mark it to be mentored as well.

Comment 8

[Wesley Johnston (:wesj)]

Just a note to anyone who decides to give this a try that it likely won't be an easy fix. This margins code is a bit fragile and there are lots of edge cases around the web.

Comment 9

[Brad Lassey [:blassey] (use needinfo?)]

filter on [mass-p5]

Comment 10

[Chris Lord [:cwiiis]]

I wouldn't be the right person to mentor this now (maybe kats?), but I think it also makes sense to just hang this off the fennec apz work. Likelihood is margins will be reworked considerably as part of this.

Comment 11

[Kartikaya Gupta (email:[e-mail])]

I don't think this is an appropriate bug for mentoring as it is very unclear what the path forward here is. We have a similar bug on B2G where I do have a better idea of how to implement it, so maybe after switching to the C++ APZ code we can reuse that approach.

Comment 12

[Wesley Johnston (:wesj)]

Close this out if you do something better kats :)

Comment 13

[Kartikaya Gupta (email:[e-mail])]

I tested the STR from bug 972532 comment 0 using Nightly 52. The success message was pinned to the bottom of the screen just fine. Closing this as WFM.