522312 - keyboard arrow keys should let you move between form fields

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Madhava Enros [:madhava]]

When in a form, you should be able to move between fields using the keyboard's arrow keys.  This is useful in countering the occasional difficulty that exists in focusing a field, and also is a little quicker than moving back up to the touchscreen from a deployed hard keyboard.

Comment 1

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Once bug 486184 is landed it should not to hard to do.

Comment 2

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to comment #1)
> Once bug 486184 is landed it should not to hard to do.

humm. Finally this mean killing the ability to move the caret/select text into a textbox.
Maybe we should choose some other keys?

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

SNAV had logic to only use the up/down arrows (I think) and then, only move if the caret was at the beginning (up arrow) or end (down arrow) of the text in the textbox.

Comment 4

[Antonio Gomes (tonikitoo)]

(In reply to comment #3)
> SNAV had logic to only use the up/down arrows (I think) and then, only move if
> the caret was at the beginning (up arrow) or end (down arrow) of the text in
> the textbox.

i fixed this wrong (ihmo) behaviour in bug 464598. dougt had even r+ the patch but the chrome test were failing on MAC and got backed out. since i do not have a mac and try server did not ran chrome tests at that time ... :(

i have re-look at it though

Comment 5

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Right, if I understand well:
 * select, button : UP/DOWN to move to the next/previous element
 * single line input: UP/DOWN to move to the next/previous element
 * textarea/multiple line input: UP/DOWN to move to the next/previous element only if we are at the start/end of the text

We don't rely on SNAV here because we want to move only on a small subset of dom nodes.

Did it misses something?

Comment 6

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch

Implementation of the previous behavior that I mentionned

Comment 7

[Stuart Parmenter]

Vivien: this should also work in chrome for moving between fields (like weave username/password)

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #7)
> Vivien: this should also work in chrome for moving between fields (like weave
> username/password)

I don't think chrome is part of this bug, or maybe this bug should be split into two different bugs. The Form Assistant is not visible for chrome UI.

If you want a general purpose SNAV approach, one that works with Form Assistant, then we could do it in this bug.

Comment 9

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: dirty wip wichh not handle nsIDOMXULMenuListElement

Comment 10

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Also "content" form helper and "xul" form helper interact badly

Comment 11

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch (HTML + XUL)

I'm a bit worried that duplicate some code. I've tried to factorize it as much as possible if I want to keep a bit of readability.

Maybe I can factorize the goToNext, goToPrevious, handleEvent methods by doing the object as instanciable class that we instanciate when we need them (to avoid a cost at startup)

Comment 12

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 413299 [details] [diff] [review]
Patch (HTML + XUL)

>+var ChromeFormHelper = {

ChromeHelper is ok for now. There really isn't a form.

>+  handleEvent: function(aEvent) {

>+    let currentElement = aEvent.originalTarget;
>+    let isDifferent = currentElement != this._walker.currentNode;
>+    if (isDifferent && this._walker.Util.isValidElement(currentElement)) {
>+      this._walker.init(aEvent.explicitOriginalTarget.parentNode);

What are we sending in here? The parent container for the widget that has
focus?


What benefit should we see for the chrome helper? Moving between textboxes?
What about buttons?

We get moving between rows of a richlist for free, without needing this patch

Maybe I am changing my mind and thinking about landing just the content patch

Comment 13

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to comment #12)
> (From update of attachment 413299 [details] [diff] [review])
> >+var ChromeFormHelper = {
> 
> ChromeHelper is ok for now. There really isn't a form.
> 
> >+  handleEvent: function(aEvent) {
> 
> >+    let currentElement = aEvent.originalTarget;
> >+    let isDifferent = currentElement != this._walker.currentNode;
> >+    if (isDifferent && this._walker.Util.isValidElement(currentElement)) {
> >+      this._walker.init(aEvent.explicitOriginalTarget.parentNode);
> 
> What are we sending in here? The parent container for the widget that has
> focus?

We are getting the container of the target of the event. If it is an anonymous node we gonna get the binding parent (or upper if these are deeply nested).
I've used that because some forms elements are part of the binding (e.g: the textboxes for a bookmark row)

> 
> 
> What benefit should we see for the chrome helper? Moving between textboxes?
> What about buttons?
> 
> We get moving between rows of a richlist for free, without needing this patch

The benefit here is a mechanism for walking between forms elements, we could have called nsIFocusManager.moveFocus but it have some undesirable behavior for our UI and probably for addons author's UI. (like moving to some stuff that are not supposed to be focused).

So basically yes, we're moving between textboxes and menulist elements. Actually I have handle the same equivalent nodes than the content FormHelper, so no buttons (the FormHelper got an exception for "submit" buttons), no checkboxes, no radios, ...

> 
> Maybe I am changing my mind and thinking about landing just the content patch

Maybe we should land the content patch and open a followup for the chrome part which can let us the time to think about it and what we want it to handle.

Comment 14

[Mark Finkle (:mfinkle) (use needinfo?)]

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

Let's go with this patch for now. It limits the up/down arrows to web content only, which is the primary goal.

We can expand the functionality in followup bugs.

Comment 15

[Mark Finkle (:mfinkle) (use needinfo?)]

pushed:
https://hg.mozilla.org/mobile-browser/rev/75e3e2bdca1b

Comment 16

[Aakash Desai [:aakashd]]

verified FIXED on builds:

Mozilla/5.0 (X11; U; Linux armv7l; Nokia N900; en-US; rv:1.9.2b5pre) Gecko/20091130 Firefox/3.6b5pre Fennec/1.0b6pre

and

Mozilla/5.0 (X11; U; Linux armv6l; Nokia N8xx; en-US; rv:1.9.3a1pre) Gecko/20091130 Firefox/3.7a1pre Fennec/1.0b5


Also, we need to add this to our BFT's under Form Manager.

Comment 17

[Aakash Desai [:aakashd]]

litmus testcase https://litmus.mozilla.org/show_test.cgi?id=9804 has been updated to regression test this bug