Form helper does not come up selecting username/password fields on Wordpress

VERIFIED FIXED

Status

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

People

(Reporter: aakashd, Assigned: vingtetun)

Tracking

({polish})

Details

(Whiteboard: formfill)

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

9 years ago
Build Id:
Mozilla/5.0 (X11; U; Linux armv7l; Nokia N900; en-US; rv:1.9.2pre) Gecko/20100119 Firefox/3.6pre Fennec/1.1a1pre

Steps to Reproduce:
1. Go to www.wordpress.com
2. Select the username field
3. Select the password field

Actual Results:
Form helper does not come up

Expected Results:
Form helper bar will pop up on the bottom of the screen and the page will zoom into the selected field.

Comment 1

9 years ago
Additional info from me:

The behaviour on this page is weird. If you click username or password field at first, FA will not appear. 
BUT
If you follow steps below it will work:

1. Click language selector - form assistant will appear
2. Close form assistant
3. Now click on username or password field - form assistant will appear.
It sounds like wordpress is using an hidden input field, so whe you click you're not focusing this one directly but a div with text in it which act as a placeholder

Comment 3

8 years ago
Created attachment 439881 [details]
small tc

(In reply to comment #2)
> It sounds like wordpress is using an hidden input field, so whe you click
> you're not focusing this one directly but a div with text in it which act as a
> placeholder

There are spans on top of those input fields, they are hidden (display:none) once you hit some character.
(Reporter)

Updated

8 years ago
Keywords: polish
Whiteboard: formfill

Comment 4

8 years ago
Created attachment 485023 [details] [diff] [review]
A simple patch

With this simple patch the test case works. I was not able to test with the actual wordpress.com page as for some reason the network connection is now broken in my Fennec.

But this patch propably breaks something as I am not 100% familiar with the form assistant. So I don't really know why labels are considered as interesting elements when finding out the closest elements. Is label really that much a clickable element? I don't know what kind of Web2.0 magic can be done using just lebels :)
(In reply to comment #4)
> Created attachment 485023 [details] [diff] [review]
> A simple patch
> 
> With this simple patch the test case works. I was not able to test with the
> actual wordpress.com page as for some reason the network connection is now
> broken in my Fennec.
> 
> But this patch propably breaks something as I am not 100% familiar with the
> form assistant. So I don't really know why labels are considered as interesting
> elements when finding out the closest elements. Is label really that much a
> clickable element? I don't know what kind of Web2.0 magic can be done using
> just lebels :)

Thanks for tryng. Labels are interesting because they give you context about a field or a checkbox. I don't think we use labels alone otherwise.

Comment 6

8 years ago
Tapping on labels on other pages (for example gmail.com) does not activate the form assistant. And basically the reason for the reported bug is that the on the wordpress.com page it is the label element which gets found as the "interesting" element when tapping on the username input field. And, as gmail.com shows, tapping on label does not activate the form assistant :)

So they are interesting for the user, but why is the current form assistant implementation "semi-interested" in labels?
(In reply to comment #6)
> Tapping on labels on other pages (for example gmail.com) does not activate the
> form assistant. And basically the reason for the reported bug is that the on
> the wordpress.com page it is the label element which gets found as the
> "interesting" element when tapping on the username input field. And, as
> gmail.com shows, tapping on label does not activate the form assistant :)

Right. 
The code from getElementClickable is used to help users click on form elements for small zoom factor by searching the "best" target to dispatch the click. "Labels" are interesting because they can be associated to "checkbox" or "radio" buttons and are sometimes much easier to click than the button itself.

The second code you're modifying is used to tell the user the element is clickable by showing blue rect area over the element.

> So they are interesting for the user, but why is the current form assistant
> implementation "semi-interested" in labels?

I've misunderstood the question in your previous comment, and I've gave you a wrong not related to the patch you're doing, because your patch is not related to the form assistant or only indirectely. 
But to reply to this question, the Form Assistant is "semi-interested" in labels because it tries to display on screen (while zooming) the target field but also its associated label.

So for your patch, I'm curious, does the form assistant show up with the testcase?

Comment 8

8 years ago
(In reply to comment #7)
> So for your patch, I'm curious, does the form assistant show up with the
> testcase?

Yes, with the patch the test case from comment 3 works, form assistant is shown. I was not able to test the test sequence from the bug report as for some reason I cannot access any web pages currently with Fennec.
(In reply to comment #8)
> (In reply to comment #7)
> > So for your patch, I'm curious, does the form assistant show up with the
> > testcase?
> 
> Yes, with the patch the test case from comment 3 works, form assistant is
> shown. I was not able to test the test sequence from the bug report as for some
> reason I cannot access any web pages currently with Fennec.

I don't really want to remove "label" from the list of clickable elements. I would rather investigate why we're getting the label element instead of the input field (which is probably because the field is hidden)

Comment 10

8 years ago
(In reply to comment #9)
> I would rather investigate why we're getting the label element instead of the
> input field (which is probably because the field is hidden)

It doest not return the label element.

In ElementTouchHelper.getClosest, aWindowUtils.elementFromPoint returns span-element. Then is the check for early return which calls _isElementClickable with the span-element as the parameter. _isElementClickable goes to the for loop (the order of elements in the for loop would be span, input, label, body, html). Span does not match the selector, but in the next loop iteration label matches. _isElementClickable returns true and then getClosest returns early, returning the span as the closest element.

A little bit later, formAssistant.open is called with the span element as parameter.

If the early return is removed from getClosest, the function will still return span-element. The loop after nodesFromRect gets loops the elements span, input, label, html. So, first it checks span and calls _isElementClickable. _isElementClickable once again returns true for the span element (because its parentNode is label) and now getClosest sets the target-variable to the span element. While iterating the loop, there is no other clickable element which would be closer to (aX,aY) so finally the return value, ie. the closest element is the span element.

I still don't understand why the check does check also for labels. On all of the pages (maybe 5 pages) I tried, tapping on any label did not launch form assistant anyway.
(In reply to comment #10)

> I still don't understand why the check does check also for labels. On all of
> the pages (maybe 5 pages) I tried, tapping on any label did not launch form
> assistant anyway.

We do not want to show the Form Assistant when the user clicks on labels element, the isElementClickable function has nothing to do with the Form Assistant, it is here to facilitate the action of clicking on an element for small zoom level.


If you want to take this bug and provide a patch I would be happy to help. I just think your patch is not the appropriate solution for this bug (and in fact it does not resolve the real bug on wordpress.com)

I don't have any opinion or ideas about the correct solution for this bug so i really appreciate suggestions or patches :)

Comment 12

8 years ago
Created attachment 485704 [details] [diff] [review]
A proper patch

(In reply to comment #11)
> the isElementClickable function has nothing to do with the Form
> Assistant

Yes it has. Form assistant is opened for the element which is returned by a call to elementFromPoint whose return value greatly depends on what isElementClickable returns.

This patch adds formElementFromPoint function which is used only when trying to find out the proper element for form assistant. It uses selector which does not include "label" for finding out if the element is clickable.

Now that the network works again on my Fennec, I have seen that with this patch the bug in the bug report seems to be fixed, and the test case also works. Propably this also breaks the form assistant on some obscure web page, but I could not find any such pages yet.
(In reply to comment #12)
> Created attachment 485704 [details] [diff] [review]
> A proper patch
> 
> (In reply to comment #11) 
> Now that the network works again on my Fennec, I have seen that with this patch
> the bug in the bug report seems to be fixed, and the test case also works.
> Propably this also breaks the form assistant on some obscure web page, but I
> could not find any such pages yet.

Coud you upate your patch on the trunk please, this way I could try to test it on my build?

Also do you mind trying to click 10px above the labels of the radio buttons of http://www.w3schools.com/tags/tryit.asp?filename=tryhtml_label with and without your patch and tell me if there is any difference?

What I'm trying to says is because your patch removed "label" from the list of clickable element if will probably make it harder to click on them (and because labels are associated to radios and checkboxes, it will make life harder for them too)

Comment 14

8 years ago
(In reply to comment #13)
> Coud you upate your patch on the trunk please, this way I could try to test it
> on my build?

I thought it was. I updated this morning and hg pull gave me no updates now.

> Also do you mind trying to click 10px above the labels of the radio buttons of
> http://www.w3schools.com/tags/tryit.asp?filename=tryhtml_label with and without
> your patch and tell me if there is any difference?

Overall, even without the patch, I had difficulties in finding any logic on when the form assistant is invoked when tapping on the radio buttons. However, without the patch it was possible (sometimes) to get the form assistant when tappina sligtly above the "Male" text. With the patch I did not get the form assistant when tappin above the text.

I would not consider this a problem though. Usually there are plenty of radio boxes together in a small area forming a quite large area and activating the form assistang by tapping on that quite large area is a lot easier than tapping on Wordpress.com username field and not getting the form assistant at all. Trying again and not getting it. Not getting it. And so on...
(In reply to comment #14)
> (In reply to comment #13)
> > Coud you upate your patch on the trunk please, this way I could try to test it
> > on my build?
> 
> I thought it was. I updated this morning and hg pull gave me no updates now.

hg update?

Comment 16

8 years ago
(In reply to comment #15)
> (In reply to comment #14)
> > (In reply to comment #13)
> > > Coud you upate your patch on the trunk please, this way I could try to test it
> > > on my build?
> > I thought it was. I updated this morning and hg pull gave me no updates now.
> hg update?

Works also today:
$ hg revert --all
$ hg pull
$ hg update
$ patch -p1 < BMO_540613_patch.diff
patching file chrome/content/content.js
Hunk #4 succeeded at 400 (offset 1 line).

Comment 17

8 years ago
Uh... Is the form assistant even supposed to be launched when tapping on radio button?

http://mxr.mozilla.org/mobile-browser/source/chrome/content/forms.js#274

It seems like it is not, so I don't understand at all the issue for tapping radio or checkbox button labels.
(In reply to comment #17)
> Uh... Is the form assistant even supposed to be launched when tapping on radio
> button?

Tapping on a radio button will not make the Form Helper appear, if it is not visible. But tapping on a radio button will not hide the Form Helper, if it's already visible.

http://mxr.mozilla.org/mobile-browser/source/chrome/content/forms.js#105

Comment 19

8 years ago
(In reply to comment #18)
> (In reply to comment #17)
> > Uh... Is the form assistant even supposed to be launched when tapping on radio
> > button?
> Tapping on a radio button will not make the Form Helper appear, if it is not
> visible. But tapping on a radio button will not hide the Form Helper, if it's
> already visible.

And thats exactly how it works also with the patch. But tapping on the label next to the radio button usually hides the form helper, with the patch and without the patch.

So I think the patch fixes the reported problem and does not cause any additional problems for radio buttons or checkboxes. Or is there still some problem I have missed?

Updated

8 years ago
Attachment #485704 - Flags: review?(mark.finkle)
Comment on attachment 485704 [details] [diff] [review]
A proper patch

Your patch fix the problem in a very specific manner, the real problem is because of a focus redirection while the <span/> hide itself.

Changing the getClosest function does not resolve the real bug, your patch works because you are ignoring the label that contains the input field but clicking on a <label/> element should go thought the getClosest function because this makes life easier for everyone when you want to click on a radio button or a checkbox.
Attachment #485704 - Flags: review?(mark.finkle) → review-
Created attachment 487333 [details] [diff] [review]
Patch

The patch react on on focus redispatching, I've added a focusSync flag because I don't think we want the Form Assistant to come up automatically when loading google.com because of the automatic focus. The flag can be deleted once we will handle tri-state focus.
Attachment #485023 - Attachment is obsolete: true
Attachment #485704 - Attachment is obsolete: true
Attachment #487333 - Flags: review?(mark.finkle)

Comment 22

8 years ago
(In reply to comment #20)
> Changing the getClosest function does not resolve the real bug, your patch
> works because you are ignoring the label that contains the input field but
> clicking on a <label/> element should go thought the getClosest function
> because this makes life easier for everyone when you want to click on a radio
> button or a checkbox.

I still don't understand this point at all. I have tested three versions, the current, the one with my patch and the one with your patch and I could NEVER get the form assistant to show when clicking on a label of a radio button or checkbox.

Well, you know the component a lot better than I do. I think I'll do something else end I'll just be happy as long as this bug gets fixed :)
Attachment #487333 - Flags: review?(mark.finkle) → review+
Ca we add a test for this case?
(In reply to comment #23)
> Ca we add a test for this case?

JS Compartment makes things hard for adding a test case for this one. I've open bug 610341 to add some and to remove the workaround for the similar cases.
Version: 1.9.2 Branch → Trunk
http://hg.mozilla.org/mobile-browser/rev/35bf7da77ccd
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
(Reporter)

Comment 26

8 years ago
verified FIXED on builds:
Mozilla/5.0 (Maemo; Linux armv71; rv:2.0b8pre) Gecko/20101109 Namoroka/4.0b8pre Fennec/4.0b3pre

and

Mozilla/5.0 (Android; Linux armv71; rv:2.0b8pre) Gecko/20101109 Namoroka/4.0b8pre Fennec/4.0b3pre

Follow-up bug filed: https://bugzilla.mozilla.org/show_bug.cgi?id=610684
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.