785939 - When the soft keyboard is displayed, form inputs should move above the keyboard boundary

Status: VERIFIED FIXED

(Firefox for Metro Graveyard :: General, defect)

Description

[Jim Mathies [:jimm]]

Form inputs currently don't move to a visible position when the soft keyboard is displayed. 

<mfinkle> jimm, we could probably do this easier now in native fennec
<mfinkle> since we could just move the gecko content layer above the keyboard
<mfinkle> not the entire window
<mbrubeck> same thing could work in XUL by moving/resizing the <browser> around within the window...
<mfinkle> indeed
<mfinkle> in fact nokia had a patch at one point that added a "buffer" <box> below the <browser> <deck> to do just that
<jimm> I like that idea better than mucking with gfx code :)
<mfinkle> jimm, it requires being able to get the rect used by the virtual keyboard
<jimm> we have that
<jimm> we fire observers (metro_softkeyboard_hidden, metro_softkeyboard_shown) which get picked up by browser.js. then you can query nsIWinMetroUtils for the client rects.
<jimm> "client rect" not rects
<mbrubeck> perfect

Comment 2

[Jim Mathies [:jimm]]

Interesting note, we actually do this via something in FormHelper.js. However it doesn't quite work right. In some cases the input will move to the right position, in others it's scrolled off the top of the screen.

I think when this was originally filed, FormHelper was disabled.

I'll post a test case.

Comment 3

[Jim Mathies [:jimm]]

Attachment: forms test case

The first single line text edit below the text area will scroll off the screen when tapped.

Comment 4

[Matt Brubeck (:mbrubeck)]

Attachment: patch

This is a simple fix, plus a comment that should help explain the fix.

Comment 5

[Allison Naaktgeboren :ally]

Comment on attachment 669281 [details] [diff] [review]
patch

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

I'm not sure I should qualify as a reviewer yet, but here is goes anyway.

This looks reasonable, but I have some questions

1) I am concerned about the lack of tests. Is this code already covered by an existing test that I don't know about? Is there some compelling reason this code isn't unit testable?

2) I am slightly confuzzled by the comment appearing in the patch but not the code is is about. Does the code already exist and is/was prone to misuse, or is that code new as well?

3) You might want to consider listing the styles in reverse order. The thing *most* consumers really want would be the 'viewable-width, viewable-height' but I can see one too many overclocked developers not reading the whole thing to realize that what they really want is the area without the toolbars *or* onscreen keyboard. (Just a thought, not a requirement)

Comment 6

[Matt Brubeck (:mbrubeck)]

(In reply to :Ally Naaktgeboren from comment #5)
> 1) I am concerned about the lack of tests. Is this code already covered by
> an existing test that I don't know about? Is there some compelling reason
> this code isn't unit testable?

Good call... as of a couple weeks ago we can write browser-chrome style tests that run in Metro (bug 771271), so we should be able to test what happens the Metro keyboard appears.  We don't yet have automation running these tests, but we can at least run them locally for now.  I'll work on a test for this.

> 2) I am slightly confuzzled by the comment appearing in the patch but not
> the code is is about. Does the code already exist and is/was prone to
> misuse, or is that code new as well?

This is existing code but it was poorly documented.  I found myself about to describe it in a Bugzilla comment explaining my patch.  But then I figured it would be better to add it as in-code documentation.

> 3) You might want to consider listing the styles in reverse order. The thing
> *most* consumers really want would be the 'viewable-width, viewable-height'

Actually it looks like window-width and window-height are currently the most used.

We might want to combine content-* and viewable-*; I'm not sure it's actually useful to keep them separate...

Comment 7

[Allison Naaktgeboren :ally]

(In reply to Matt Brubeck (:mbrubeck) from comment #6)
> Good call... as of a couple weeks ago we can write browser-chrome style
> tests that run in Metro (bug 771271), so we should be able to test what
> happens the Metro keyboard appears.  We don't yet have automation running
> these tests, but we can at least run them locally for now.  I'll work on a
> test for this.

\o/

I thought that might be the case and I support what you did. I just wanted to make sure I understood what was going on.


> We might want to combine content-* and viewable-*; I'm not sure it's
> actually useful to keep them separate...

I can't say I see a compelling reason for keeping them separate, but I am not an expert. I think that it might make a good followup bug. (keep this one from scope creeping and that work is probably lower priority than this bug)

Comment 8

[Matt Brubeck (:mbrubeck)]

(In reply to Matt Brubeck (:mbrubeck) from comment #6)
> Good call... as of a couple weeks ago we can write browser-chrome style
> tests that run in Metro (bug 771271), so we should be able to test what
> happens the Metro keyboard appears.

D'oh, it turns out we can't write a automated test for this using the actual keyboard.  In Metro, "applications cannot programmatically invoke the touch keyboard" so we can't make this happen in an automated test:
http://msdn.microsoft.com/en-us/library/windows/apps/hh465404.aspx#user-driven_invocation

We can test part of the logic by providing a mock implementation of nsIWinMetroUtils and synthesizing the keyboard changes ourselves.  This isn't as good, but it will have to do... until we can get a robot to drive an actual touch-screen device.  :)

Comment 9

[Matt Brubeck (:mbrubeck)]

Attachment: part 0: addTab convenience method for browser-chrome tests

This takes a common pattern from browser-chrome tests (loading a URL in a new tab, doing some stuff with that tab, then closing the tab) and moves it into a utility function in head.js.  This is just to reduce boilerplate in the tests.

Comment 10

[Matt Brubeck (:mbrubeck)]

Attachment: part 1: resize the browser when the keyboard is on-screen

Same as the previous patch except it now includes a test.

Comment 11

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/projects/elm/rev/35c731535659
https://hg.mozilla.org/projects/elm/rev/5de4b9c9d812

Comment 12

[Matt Brubeck (:mbrubeck)]

Resolving bugs in the Firefox for Metro product that are fixed on the elm branch.  Sorry for the bugspam.  Search your email for "bugspam-elm" if you want to find and delete all of these messages at once.

Comment 13

[Asa Dotzler [:asa]]

There's no way to easily verify this but we'll cover that when we verify the story at bug 831980.