526045 - Form Fill UI does not dissapear when un-focusing from fields

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Aakash Desai [:aakashd]]

Build Id:
Mozilla/5.0 (X11; U; Linux armv7l; Nokia N900; en-US; rv:1.9.2b2pre) Gecko/20091102 Namoroka/3.6b2pre Fennec/1.0b5

and

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

Steps to Reproduce:
1. Go to twitter.com/login
2. Use form UI to fill out the username and password fields
3. Tap on the Sign In button with your stylus/finger

Actual Results:
The focus is now on the submit button by user interaction, but the Form Fill UI is still up and usable. 

Expected Results:
The form fill UI should disappear when a user interacts with the page outside the focus of the form fill UI's purpose.

Comment 1

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

I'm pretty sure buttons are still part of Form Fill UI purpose. You can probably find other parts of the web page that should cause the UI to dismiss.

Comment 2

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

> Steps to Reproduce:
> 1. Go to twitter.com/login
> 2. Use form UI to fill out the username and password fields
> 3. Tap on the Sign In button with your stylus/finger
> 
> Actual Results:
> The focus is now on the submit button by user interaction, but the Form Fill UI
> is still up and usable. 

Normally the Form Assistant should disappear automatically on a page load, it seems to work for me when I push the sign in button. Can you retry and confirm it didn't work for you please?

Comment 3

[Aakash Desai [:aakashd]]

The bug was filed to have the formfill UI dissapear not on page load, but when the submit button is pressed. To me, I wouldn't expect the formfill UI to work on anything but input fields. 

Unfortunately, I think this behavior is due to design as the submit button is a part of the elements that are parsed by the next/previous buttons, so its up to you guys to determine if the behavior should either change or be charged as wontfix.

Comment 4

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

> Unfortunately, I think this behavior is due to design as the submit button is a
> part of the elements that are parsed by the next/previous buttons, so its up to
> you guys to determine if the behavior should either change or be charged as
> wontfix.

Yeah that's really a question of design/behavior because it doesn't matter if the button is a part of the elements that are parsed by next/previous. For example you can click on a checkbox and it won't dismiss the formfill UI which sounds correct even if the checkbox are not handle by the formfill code.

But maybe we should dismiss the UI on some special kind of interactions. Feel free to add comments here: https://wiki.mozilla.org/Mobile/Fennec/FormAssistant

Comment 5

[Madhava Enros [:madhava]]

We want the form assistant to remain open until the user is explicitly done with it.  Buttons _are_ part of the form assistant's responsibility, but more generally, a user may well need information from elsewhere on the page in order to fill out the current field (e.g. the image for a CAPTCHA) so will be tapping and panning outside of the form.  The form assistant should stay in these cases.

Comment 6

[Aakash Desai [:aakashd]]

I think the purpose of the reported behavior was when the user begins to access parts of the page that don't belong to login. 

Hasn't the user explicitly completed their use of the form fill ui when they un-focus from any of the elements it touches?

Comment 7

[Madhava Enros [:madhava]]

What about the case where you have to pan over to get at another of your tabs to get your password from your webmail?  I guess I feel like it's easy enough to turn off the form assistant that we might as well not make it disappear when they might still be using it.

Comment 8

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

Aakash, after have used Fennec and the form assistant for a while do you still think this bug is valid or can we close it as "resolved"?

Comment 9

[Aakash Desai [:aakashd]]

Ah, I didn't realize that I hadn't answered Madhava's question. 

Madhava, opening up the tab sidebar also brings up another implementation issue....the new tab button will be 'behind' the form fill bar. If they want to open a new tab to get to their webmail, then they'll need to close it anyways. I think the use case that tab is already open and they want to get to the other tab is a very small one. The bigger problem is the formfill bar getting in the way of the user when they interact with the page.

After using formfill for some time, I think it's gotten more annoying with forms that are very short (gmail) and/or long forms that I only want to select specific fields (i..e bugzilla and trulia.com) where I don't really need the bar. 

IMO, we need to make the UI a lot more usable for this feature because it's an annoyance.

Comment 10

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

The bar blocking chrome UI is bug 524566

Comment 11

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

Attachment: Patch

This patch dismiss the form assistant bar when the user tap somewhere else on the page but the form assistant is not dismissed if the user pan and/or zoom the page.

Comment 12

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

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

OK for code. Let's see what Madhava wants to do

Comment 13

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

Attachment: Patch v0.2

This patch hide the form assistant when:
 * there is a click on a submit button (type=submit or type=image)
 * there is a click on a disabled form element
 * there is a click anywhere outside the form

Comment 14

[Madhava Enros [:madhava]]

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

UI-R+ based on the description

Comment 15

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

Comment on attachment 458367 [details] [diff] [review]
Patch v0.2


>diff -r d39aff00f0f5 chrome/content/forms.js

>   open: function(aElement) {

>+    if (!this._isValidElement(aElement)) {
>+      let passiveButtons = { button: true, checkbox: true, file: true, radio: true, reset: true };
>+      if ((aElement instanceof HTMLInputElement || aElement instanceof HTMLButtonElement) && passiveButtons[aElement.type])
>+        return this._open = false;
>+
>+      sendSyncMessage("FormAssist:Hide", { });
>+      return this._open = false;

We really need some comments in here. Two branches are returning "this._open = false", but one is also hiding the form assistant, while the other is not. Explain the difference please.

>   _isValidElement: function formHelperIsValidElement(aElement) {
>-    let formExceptions = {button: true, checkbox: true, file: true, image: true, radio: true, reset: true, submit: true};
>+    let formExceptions = { button: true, checkbox: true, file: true, image: true, radio: true, reset: true, submit: true };
>     if (aElement instanceof HTMLInputElement && formExceptions[aElement.type])
>       return false;
> 
>     if (aElement instanceof HTMLButtonElement ||
>         (aElement.getAttribute("role") == "button" && aElement.hasAttribute("tabindex")))
>       return false;

Why not add the passive button test here?

r+, but need the comment and would like rationale for not putting passivebutton test in _isValidElement

Comment 16

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

Attachment: Patch v0.3

(In reply to comment #15)
> Comment on attachment 458367 [details] [diff] [review]
> Patch v0.2
> 
> 
> >diff -r d39aff00f0f5 chrome/content/forms.js
> 
> >   open: function(aElement) {
> 
> >+    if (!this._isValidElement(aElement)) {
> >+      let passiveButtons = { button: true, checkbox: true, file: true, radio: true, reset: true };
> >+      if ((aElement instanceof HTMLInputElement || aElement instanceof HTMLButtonElement) && passiveButtons[aElement.type])
> >+        return this._open = false;
> >+
> >+      sendSyncMessage("FormAssist:Hide", { });
> >+      return this._open = false;
> 
> We really need some comments in here. Two branches are returning "this._open =
> false", but one is also hiding the form assistant, while the other is not.
> Explain the difference please.

Thanks for catching this. That's a bug!
The _open property is an internal var use for return false if needed and let the click go thought the target element (to be able to reposition caret), but in the case where the form helper is not close we don't really want to turn _open to false.

> 
> >   _isValidElement: function formHelperIsValidElement(aElement) {
> >-    let formExceptions = {button: true, checkbox: true, file: true, image: true, radio: true, reset: true, submit: true};
> >+    let formExceptions = { button: true, checkbox: true, file: true, image: true, radio: true, reset: true, submit: true };
> >     if (aElement instanceof HTMLInputElement && formExceptions[aElement.type])
> >       return false;
> > 
> >     if (aElement instanceof HTMLButtonElement ||
> >         (aElement.getAttribute("role") == "button" && aElement.hasAttribute("tabindex")))
> >       return false;
> 
> Why not add the passive button test here?

Because I don't want to fire a message into a function use for checking something, that's too misleading.

I'm asking r? again for validating the comment and because I've correct my mistake with _open.

Comment 17

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

http://hg.mozilla.org/mobile-browser/rev/af67cbcea09d

Comment 18

[Aakash Desai [:aakashd]]

verified FIXED on builds:

Mozilla/5.0 (X11; U; Linux armv71; Nokia N900; en-US; rv:2.0b2pre) Gecko/2010720 Namoroka/4.0b2pre Fennec/2.0a1pre

and

Mozilla/5.0 (Android; U; Linux armv71; en-US; rv:2.0b2pre) Gecko/20100720 Namoroka/4.0b2pre Fennec/2.0a1pre

Comment 19

[Aakash Desai [:aakashd]]

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