Closed Bug 522312 Opened 10 years ago Closed 10 years ago

keyboard arrow keys should let you move between form fields

Categories

(Firefox for Android Graveyard :: General, defect)

x86
macOS
defect
Not set

Tracking

(fennec1.0+)

VERIFIED FIXED
fennec1.0
Tracking Status
fennec 1.0+ ---

People

(Reporter: madhava, Assigned: vingtetun)

References

Details

(Whiteboard: formfill)

Attachments

(2 files, 1 obsolete file)

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.
tracking-fennec: --- → ?
Summary: keyboard arrow keys should let you move between form fields (snav) → keyboard arrow keys should let you move between form fields
tracking-fennec: ? → 1.0+
Once bug 486184 is landed it should not to hard to do.
Depends on: 486184
Whiteboard: formfill
(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?
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.
(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
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?
Attached patch PatchSplinter Review
Implementation of the previous behavior that I mentionned
Assignee: nobody → 21
Attachment #411800 - Flags: review?(mark.finkle)
Vivien: this should also work in chrome for moving between fields (like weave username/password)
(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.
Also "content" form helper and "xul" form helper interact badly
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)
Attachment #413098 - Attachment is obsolete: true
Attachment #411800 - Attachment is obsolete: true
Attachment #411800 - Flags: review?(mark.finkle)
Attachment #413299 - Flags: review?(mark.finkle)
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
Attachment #411800 - Attachment is obsolete: false
Attachment #411800 - Flags: review?(mark.finkle)
(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 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.
Attachment #411800 - Flags: review?(mark.finkle) → review+
pushed:
https://hg.mozilla.org/mobile-browser/rev/75e3e2bdca1b
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Post-B5
Attachment #413299 - Attachment description: Patch → Patch (HTML + XUL)
Attachment #413299 - Flags: review?(mark.finkle)
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.
Status: RESOLVED → VERIFIED
Flags: in-litmus?
litmus testcase https://litmus.mozilla.org/show_test.cgi?id=9804 has been updated to regression test this bug
Flags: in-litmus? → in-litmus+
You need to log in before you can comment on or make changes to this bug.