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

VERIFIED FIXED

Status

Firefox for Android Graveyard
General
--
major
VERIFIED FIXED
9 years ago
8 years ago

People

(Reporter: aakashd, Assigned: vingtetun)

Tracking

Fennec 1.1
Bug Flags:
in-litmus +

Details

(Whiteboard: formfill)

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

9 years ago
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.
Flags: in-litmus?
(Reporter)

Updated

9 years ago
tracking-fennec: --- → ?
Whiteboard: formfill
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.
> 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?
(Reporter)

Comment 3

9 years ago
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.
> 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
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.
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → WONTFIX
tracking-fennec: ? → ---
(Reporter)

Comment 6

9 years ago
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?
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
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.
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"?
(Reporter)

Comment 9

8 years ago
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.
Summary: Form Fill UI does not dissapear when un-focusing from fields → Form Fill UI does not dissapear when unfocusing from fields
Summary: Form Fill UI does not dissapear when unfocusing from fields → Form Fill UI does not dissapear when un-focusing from fields
The bar blocking chrome UI is bug 524566
Created attachment 457192 [details] [diff] [review]
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.
Assignee: nobody → 21
Attachment #457192 - Flags: ui-review?
Attachment #457192 - Flags: review?(mark.finkle)
Attachment #457192 - Flags: ui-review? → ui-review?(madhava)
Comment on attachment 457192 [details] [diff] [review]
Patch

OK for code. Let's see what Madhava wants to do
Attachment #457192 - Flags: review?(mark.finkle) → review+
Created attachment 458367 [details] [diff] [review]
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
Attachment #457192 - Attachment is obsolete: true
Attachment #458367 - Flags: ui-review?
Attachment #458367 - Flags: review?(mark.finkle)
Attachment #457192 - Flags: ui-review?(madhava)
Attachment #458367 - Flags: ui-review? → ui-review?(madhava)
Comment on attachment 457192 [details] [diff] [review]
Patch

UI-R+ based on the description
Attachment #457192 - Flags: ui-review+
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
Attachment #458367 - Flags: review?(mark.finkle) → review+
Created attachment 458476 [details] [diff] [review]
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.
Attachment #458367 - Attachment is obsolete: true
Attachment #458476 - Flags: review?(mark.finkle)
Attachment #458367 - Flags: ui-review?(madhava)
Attachment #458476 - Flags: review?(mark.finkle) → review+
http://hg.mozilla.org/mobile-browser/rev/af67cbcea09d
Status: REOPENED → RESOLVED
Last Resolved: 9 years ago8 years ago
Resolution: --- → FIXED
(Reporter)

Comment 18

8 years ago
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
Status: RESOLVED → VERIFIED
(Reporter)

Updated

8 years ago
Assignee: 21 → mozaakash
(Reporter)

Updated

8 years ago
Assignee: mozaakash → 21
Flags: in-litmus? → in-litmus?(mozaakash)
(Reporter)

Comment 19

8 years ago
litmus testcase https://litmus.mozilla.org/show_test.cgi?id=12585 updated to regression test this bug.
Flags: in-litmus?(mozaakash) → in-litmus+
You need to log in before you can comment on or make changes to this bug.