Closed Bug 785939 Opened 12 years ago Closed 11 years ago

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

Categories

(Firefox for Metro Graveyard :: General, defect)

x86_64
Windows 8.1
defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: jimm, Assigned: mbrubeck)

References

Details

(Keywords: feature)

Attachments

(3 files, 1 obsolete file)

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
Assignee: nobody → mbrubeck
Whiteboard: [metro-preview]
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.
Attached file forms test case
The first single line text edit below the text area will scroll off the screen when tapped.
Whiteboard: [metro-preview]
Attached patch patch (obsolete) — Splinter Review
This is a simple fix, plus a comment that should help explain the fix.
Attachment #669281 - Flags: review?(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)
Attachment #669281 - Flags: review?(ally) → feedback+
(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...
(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)
Product: Firefox → Firefox for Metro
Keywords: feature
Whiteboard: [metro-mvp]
(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.  :)
Status: NEW → ASSIGNED
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.
Attachment #673289 - Flags: review?(fryn)
Same as the previous patch except it now includes a test.
Attachment #669281 - Attachment is obsolete: true
Attachment #673291 - Flags: review?(fryn)
Attachment #673289 - Flags: review?(fryn) → review+
Attachment #673291 - Flags: review?(fryn) → review+
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.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Blocks: 831980
There's no way to easily verify this but we'll cover that when we verify the story at bug 831980.
Status: RESOLVED → VERIFIED
Whiteboard: [metro-mvp][completed-elm]
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.