Closed Bug 976531 Opened 8 years ago Closed 5 years ago
Keyboard lowering causes a re-draw that breaks page layout
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.
Summary: Keyboard lowering causes a re-draw that messes up page layout. → Keyboard lowering causes a re-draw that breaks page layout.
Component: General → Graphics, Panning and Zooming
OS: Mac OS X → Android
Hardware: x86 → ARM
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.
Attachment #8386443 - Flags: review?(bugmail.mozilla)
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.
Attachment #8386443 - Flags: review?(bugmail.mozilla) → review?(chrislord.net)
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?
Attachment #8386443 - Flags: review?(chrislord.net) → feedback+
Any progress on this, anything we can do to help?
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.
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.
filter on [mass-p5]
Priority: -- → P5
Whiteboard: [mentor=Cwiiis][lang=java] → [lang=java]
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.
Depends on: apz-fennec
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.
See Also: → overlaid-keyboard
Close this out if you do something better kats :)
Assignee: wjohnston → nobody
Status: ASSIGNED → NEW
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.
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.