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

VERIFIED FIXED

Status

Firefox for Metro
General
VERIFIED FIXED
5 years ago
3 years ago

People

(Reporter: jimm, Assigned: mbrubeck)

Tracking

({feature})

Trunk
x86_64
Windows 8.1
feature

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
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)

Updated

5 years ago
Duplicate of this bug: 784814
(Assignee)

Updated

5 years ago
Assignee: nobody → mbrubeck
Whiteboard: [metro-preview]
(Reporter)

Comment 2

5 years ago
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.
(Reporter)

Comment 3

5 years ago
Created attachment 661764 [details]
forms test case

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

Updated

5 years ago
Whiteboard: [metro-preview]
(Assignee)

Comment 4

5 years ago
Created attachment 669281 [details] [diff] [review]
patch

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+
(Assignee)

Comment 6

5 years ago
(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)
(Reporter)

Updated

5 years ago
Component: General → General
Product: Firefox → Firefox for Metro

Updated

5 years ago
Keywords: feature
Whiteboard: [metro-mvp]
(Assignee)

Comment 8

5 years ago
(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
(Assignee)

Comment 9

5 years ago
Created attachment 673289 [details] [diff] [review]
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.
Attachment #673289 - Flags: review?(fryn)
(Assignee)

Comment 10

5 years ago
Created attachment 673291 [details] [diff] [review]
part 1: resize the browser when the keyboard is on-screen

Same as the previous patch except it now includes a test.
Attachment #669281 - Attachment is obsolete: true
Attachment #673291 - Flags: review?(fryn)

Updated

5 years ago
Attachment #673289 - Flags: review?(fryn) → review+

Updated

5 years ago
Attachment #673291 - Flags: review?(fryn) → review+
(Assignee)

Comment 11

5 years ago
https://hg.mozilla.org/projects/elm/rev/35c731535659
https://hg.mozilla.org/projects/elm/rev/5de4b9c9d812
Whiteboard: [metro-mvp] → [metro-mvp][completed-elm]
(Assignee)

Comment 12

5 years ago
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
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Assignee)

Updated

5 years ago
Blocks: 831980

Comment 13

5 years ago
There's no way to easily verify this but we'll cover that when we verify the story at bug 831980.
Status: RESOLVED → VERIFIED

Updated

5 years ago
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.