Closed Bug 486184 Opened 11 years ago Closed 10 years ago

make filling in forms easier

Categories

(Firefox for Android Graveyard :: General, defect)

defect
Not set

Tracking

(fennec1.0+)

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

People

(Reporter: madhava, Assigned: vingtetun)

References

Details

(Whiteboard: [fennec l10n])

Attachments

(3 files, 14 obsolete files)

441.80 KB, image/jpeg
Details
33.53 KB, image/png
Details
33.24 KB, patch
stechz
: review+
Details | Diff | Splinter Review
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
tracking-fennec: --- → ?
also - make sure that spatial nav (tabbing from field to field) works
tracking-fennec: ? → 1.0+
I think this is important enough to start working on it for b3, not waiting for 1.0
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 ? )
(In reply to comment #0)

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

Bug 472426
(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?
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.
Assignee: nobody → mark.finkle
Attached patch WIP-1 (obsolete) — Splinter Review
- 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
Attached patch WIP-2.1 (obsolete) — Splinter Review
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
Attachment #391459 - Attachment is obsolete: true
Attachment #391636 - Attachment is patch: false
Attachment #391636 - Attachment mime type: text/plain → image/png
Attached patch WIP-2.3 (obsolete) — Splinter Review
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
Attachment #391633 - Attachment is obsolete: true
Attached patch WIP-2.5 (obsolete) — Splinter Review
- 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
Attachment #391705 - Attachment is obsolete: true
Attached patch patch v0.1 (obsolete) — Splinter Review
Zoom works better now
Attachment #391744 - Attachment is obsolete: true
Vivien, can you update this to trunk?
Assignee: mark.finkle → 21
Whiteboard: [fennec l10n]
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?
(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.
Attached patch Patch v0.2 (obsolete) — Splinter Review
* Updated on trunk
* call _getAll only once
Attachment #396930 - Attachment is obsolete: true
Attached patch patch 0.3 (updated to trunk) (obsolete) — Splinter Review
* 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.
Attachment #401199 - Attachment is obsolete: true
> * 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
Attached patch Patch v0.4 (obsolete) — Splinter Review
(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.
Attachment #406399 - Attachment is obsolete: true
Attached patch Patch v0.5 (obsolete) — Splinter Review
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.
Attachment #406589 - Attachment is obsolete: true
Attached patch Patch v0.6 (obsolete) — Splinter Review
* work with the right panel now
* minor code cleanup
Attachment #406656 - Attachment is obsolete: true
Attachment #407020 - Flags: review?(mark.finkle)
Attached patch Patch v0.7 (obsolete) — Splinter Review
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!
Attachment #407020 - Attachment is obsolete: true
Attachment #407020 - Flags: review?(mark.finkle)
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.
(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.
Attached patch Patch v0.8 (obsolete) — Splinter Review
(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
Attachment #407785 - Attachment is obsolete: true
Attachment #408021 - Flags: review?(mark.finkle)
Attachment #408021 - Flags: review?(webapps)
Ben, can you look over the changes to the zoom code?
Attached patch Patch v0.8 (obsolete) — Splinter Review
* Handle the resize of the window
Attachment #408021 - Attachment is obsolete: true
Attachment #408043 - Flags: review?(webapps)
Attachment #408043 - Flags: review?(mark.finkle)
Attachment #408021 - Flags: review?(webapps)
Attachment #408021 - Flags: review?(mark.finkle)
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)
- 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.
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.
Attachment #408043 - Flags: review?(mark.finkle) → review+
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 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.
Attachment #408043 - Flags: review?(webapps) → review-
Attached patch Patch v0.9Splinter Review
* correct the urlbar pan bug.
Attachment #408135 - Flags: review?(webapps)
Attachment #408135 - Flags: review?(webapps) → review+
Attachment #408043 - Attachment is obsolete: true
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.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → B5
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?
Status: RESOLVED → VERIFIED
Flags: in-litmus?
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.
How do you want to address this, Aakash? This isn't a simple one-off testcase for litmus.
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.
litmus testcases 9803, 9804, 9805 were created 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.