486184 - make filling in forms easier

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Madhava Enros [:madhava]]

Right now, filling in forms is very difficult.  There are a couple of things that would make this easier:

- make it easier to focus form fields -- right now, tapping on them, like with links, often doesn't "take"
- when focusing a form field, we should zoom in to some smart zoom level around the form field

- create a finger-use-appropriate version of a combo box / drop down box

Comment 1

[Madhava Enros [:madhava]]

also - make sure that spatial nav (tabbing from field to field) works

Comment 2

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

I think this is important enough to start working on it for b3, not waiting for 1.0

Comment 3

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

For making it easier to focus form field, doesn't it help to focus him on a mousedown (like in Firefox) instead of a complete click ?

Maybe we can assume than if a user click on something like an input field or a links and move only for a few pixels he want to click instead of pan? We can make a kind of funnel with condition depending on where append the mouseup.
 
It that's ok I can try to propose a patch a least for trying to make it easier to focus form field and click on something. ( Maybe the things mentionned by Madhava are separated bugs ? )

Comment 4

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

(In reply to comment #0)

> - create a finger-use-appropriate version of a combo box / drop down box

Bug 472426

Comment 5

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

(In reply to comment #0)

> - make it easier to focus form fields -- right now, tapping on them, like with
> links, often doesn't "take"

This is working much better now.

> - when focusing a form field, we should zoom in to some smart zoom level around
> the form field

This is something we have not started at all. I'm open to ideas about the "smart" zoom. It does sound like a good idea. How do we guess at a good zoom?
 1. Just zoom to the form control, but add bigger margins? (might miss any labels)
 2. Look for a parent <form> and zoom to it? (form could be big or missing)
 3. Look for an associated <label> and zoom to the label+form?
 4. Some combination of #1 and #3?

Comment 6

[Madhava Enros [:madhava]]

I think I'm in favour of zooming to the label+field when the field is focused.  If a person just wants to get a closer look at the overall form, we should let him or her double-tap within the borders of the form and do our usual zoom to area zooming.  By the point a person is actually focusing a field, he or she probably wants to interact with it specifically, so zooming right in makes sense.  The tension between that tight zoom and wanting to find the next field is where the design pattern of having "next" and "previous" buttons, as in the iphone's Form Assistant, comes from -- we should probably do something similar.

Comment 7

[Madhava Enros [:madhava]]

Attachment: a form bar, to help get from field to field

Comment 8

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

Attachment: WIP-1

- I'll probably move the getLabelForElement method in FormsHelper

- label+field zoom still need work.

- not sure of want to use an exeption in InputHandler for forms elements. I'm wondering if I should do a xbl binding for all forms element that handle the click? (kill perf?)

- Point 3 of madhava's mockup is not implemented yet, I'm not quite sure of the right way to build my implementation for that.
 * merge select helper + forms helper ?
 * keep them separate and just do a UI glue ?

- some small bugs + css tweaks

Comment 9

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

Attachment: WIP-2.1

Mostly working but:
 - act as a modal so the user have to click 'Done' before being able to to click anywhere
 - doesn't allow to use next/previous in form element without a parent form
 - Opening the urlbar while using the form manager can hide the focus form field

Comment 10

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

Attachment: example of a select

Comment 11

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

Attachment: WIP-2.3

Mostly working but:
 - act as a modal so the user have to click 'Done' before being able to to
click anywhere
 - With the modal mode urlbar act strangely
 - Need some XXX removals

Comment 12

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

Attachment: WIP-2.5

- Remove the modal behavior (we have the same bug as 500848), but modal mode prevent us to pan or click or anything, and it's really annoying!
- Remove XXX

Comment 13

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

Attachment: patch v0.1

Zoom works better now

Comment 14

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

Vivien, can you update this to trunk?

Comment 15

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

Attachment: Patch v0.1 - updated on trunk

Comment 16

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 396930 [details] [diff] [review]
Patch v0.1 - updated on trunk

>diff -r 9e6d655f6dca chrome/content/browser-ui.js

>+  _getAll: function() {

This function seems to be called a lot (via _getNext/_getPrevious). Can we cache these results and save _currentIndex rather than _currentElement to avoid that?

Comment 17

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

(In reply to comment #16)
> (From update of attachment 396930 [details] [diff] [review])
> >diff -r 9e6d655f6dca chrome/content/browser-ui.js
> 
> >+  _getAll: function() {
> 
> This function seems to be called a lot (via _getNext/_getPrevious). Can we
> cache these results and save _currentIndex rather than _currentElement to avoid
> that?

Probably, but my test case was the advanced search of bugzilla and the fields change depending on your selection!

I'll dive more into it tomorrow morning.

Comment 18

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

Attachment: Patch v0.2

* Updated on trunk
* call _getAll only once

Comment 19

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

Attachment: patch 0.3 (updated to trunk)

* Updated to trunk
* forms-helper-* form-helper-*  (only one form at a time)

I tested this and two things jumped out at me:
* Next and Previous had a little trouble scrolling into the right spot. The select-container frequently covered the widget
* The urlbar positioning was getting messed up. The urlbar would slide left/right as I was panning left/right, which it should never do.

Comment 20

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

> * The urlbar positioning was getting messed up. The urlbar would slide
> left/right as I was panning left/right, which it should never do.

I guess this should be corrected by bug 511224

Comment 21

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

Attachment: Patch v0.4

(In reply to comment #19)
> Created an attachment (id=406399) [details]
> patch 0.3 (updated to trunk)
> 
> I tested this and two things jumped out at me:
> * Next and Previous had a little trouble scrolling into the right spot. The
> select-container frequently covered the widget

Should works now.

> * The urlbar positioning was getting messed up. The urlbar would slide
> left/right as I was panning left/right, which it should never do.

I have done some little tweaks in the patch about that it shoulds works better now. (and bug 511224 should correct the zoom/urlbar problem we know)


!important: With this patch (and all the previous one) the 'select' support in the right panel is broken.

Comment 22

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

Attachment: Patch v0.5

Correct some minor bugs with invisible elements nested into element with display:none.

Also change the border from 0.25mm to 0.1mm.

Still not handle select for the right panel though.

Comment 23

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

Attachment: Patch v0.6

* work with the right panel now
* minor code cleanup

Comment 24

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

Attachment: Patch v0.7

This should work on trunk now but it still needed a few cleanup.

The new zoom mechanism made it works much much smoother! Thanks Stechz!

Comment 25

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

Comment on attachment 407785 [details] [diff] [review]
Patch v0.7

Thoughts:
* Working pretty good
* Why using the "rootNode.insertBefore" code to handle selects?
* I wish we didn't have FormHelper and FormHelper.isOpen used in other places in the code.
* Are the additional vboxes and buffer spacers required?

I want this patch to land as soon as possible so we can get some back time before beta 5. Let's discuss these issues.

Comment 26

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

(In reply to comment #25)
> (From update of attachment 407785 [details] [diff] [review])
> Thoughts:
> * Why using the "rootNode.insertBefore" code to handle selects?

The main reason for that is for being able to show just the chrome-select list, not the form helper when you don't need it. For example the menulist for selecting your language.
I want to reuse the same xul/js code part when we need the form helper for a list and when we don't need it. This way the only difference between the 2 appproch are some small css tweaks.

> * I wish we didn't have FormHelper and FormHelper.isOpen used in other places
> in the code.

Sure. That's why I have said the patch needs some cleanup. These FormHelper.isOpen flags are ugly. Though, I need a way to limit the zoom limit when we use the form helper because I think that better for the user to have a context of what they are seeing instead of a full zoom.

> * Are the additional vboxes and buffer spacers required?

We probably need at least one of theses boxes. I'm trying to get a working patch with as few as possible.

> I want this patch to land as soon as possible so we can get some back time
> before beta 5. Let's discuss these issues.

Comment 27

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

Attachment: Patch v0.8

(In reply to comment #25)
> (From update of attachment 407785 [details] [diff] [review])
> * I wish we didn't have FormHelper and FormHelper.isOpen used in other places
> in the code.

Done!

> * Are the additional vboxes and buffer spacers required?

The patch use only one vbox and buffer spacer now

Comment 28

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

Ben, can you look over the changes to the zoom code?

Comment 29

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

Attachment: Patch v0.8

* Handle the resize of the window

Comment 30

[Madhava Enros [:madhava]]

Tried it out!  Works well.

Two things:
- can we make the prev/next buttons visually disabled when you're at the beginning or end of the form?  They already don't push in when this is the case, but can we make them "greyed" out.
- it seems to be slow to move next or previous when the fields involved are multi-select list boxes (I'd land it anyway, though)

Comment 31

[Benjamin Stover (:stechz)]

- What do we expect on double tap?  Close the form assistant and zoom back out?
- If you accidentally click a form element, "Done" doesn't take you back to where you were.  Maybe clicking Done should restore the original viewport?
- When panning up with form filler on, the url bar becomes visible, even when I'm not at the top of the page.

Comment 32

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

Great feedback in comment #30 and comment #31.

We could try to get the disabled buttons in on the initial landing. The performance and behavior questions in comment #31 are good for followup decisions and bugs.

Comment 33

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

Comment on attachment 408043 [details] [diff] [review]
Patch v0.8

Let's try to disable the buttons when we hit the beginning or end.

Comment 34

[Benjamin Stover (:stechz)]

Comment on attachment 408043 [details] [diff] [review]
Patch v0.8

>diff -r 9c6a437c3783 chrome/content/browser-ui.js
>+      if (labelRect.left < elRect.left) {
>+        let width = labelRect.width + elRect.width + (elRect.x - labelRect.x - labelRect.width);
>+        return new Rect(labelRect.x, labelRect.y, width, elRect.height).expandToIntegers();
>+        break;
>+      }

The break is redundant.

All of my other code review questions were answered in IRC.  My only issue is
with the helper spacer, which causes the urlbar to pan onto the screen when it
is visible.

Comment 35

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

Attachment: Patch v0.9

* correct the urlbar pan bug.

Comment 36

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

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

Nice work Vivien.

As we get feedback and find bugs, open new bugs. Don't add stuff in this bug.

Comment 37

[Aakash Desai [:aakashd]]

This is up and running even though there's a huge amount of touch points that we need to start testing now...and bugs that need to be filed because this patch did not take care of them and/or brought them up. If you guys are going to bring out new features like this, you better dogfood them.

verified FIXED on builds:

Mozilla/5.0 (X11; U; Linux armv7l; en-US; rv:1.9.2b1pre) Gecko/20091026
Fennec/1.0b5pre

and

Mozilla/5.0 (X11; U; Linux armv6l; en-US; rv:1.9.3a1pre) Gecko/20091026
Fennec/1.0b5pre


This definitely has to be in our BFTs. Al, do you think we'll need to create a new subgroup for this?

Comment 38

[Al Billings [:abillings - ex-MoCo]]

This is complex enough that it should have its own section, I think. We're probably going to be adding testcases to this for a while.

Comment 39

[Al Billings [:abillings - ex-MoCo]]

How do you want to address this, Aakash? This isn't a simple one-off testcase for litmus.

Comment 40

[Aakash Desai [:aakashd]]

Yeah, I know. I'd suggest re-enabling the subgroup on our BFTs and then creating one or two testcases that test basic functionality for smoketests as well as that BFT.

Comment 41

[Aakash Desai [:aakashd]]

litmus testcases 9803, 9804, 9805 were created to regression test this bug.