851657 - Remove form element navigation from form assistant

Status: RESOLVED FIXED

(Firefox for Metro Graveyard :: Input, defect)

Description

[Jim Mathies [:jimm]]

Attachment: patch

Ran into this code today trying to get a basic caret displayed in form inputs. So I ripped it out. 

Brian would you be ok with reviewing this? I know you have been working on this area of the code a lot. Maybe you've already started this in another bug.

Comment 1

[Brian R. Bondy [:bbondy]]

(In reply to Jim Mathies [:jimm] from comment #0)
> Created attachment 725585 [details] [diff] [review]
> patch
> 
> Ran into this code today trying to get a basic caret displayed in form
> inputs. So I ripped it out. 
> 

Did you try first applying the patch in bug 849699?

What's the goal of this patch? Just to show a cursor.

Comment 2

[Jim Mathies [:jimm]]

(In reply to Brian R. Bondy [:bbondy] from comment #1)
> (In reply to Jim Mathies [:jimm] from comment #0)
> > Created attachment 725585 [details] [diff] [review]
> > patch
> > 
> > Ran into this code today trying to get a basic caret displayed in form
> > inputs. So I ripped it out. 
> > 
> 
> Did you try first applying the patch in bug 849699?

no but I can merge it with that if it breaks.

> What's the goal of this patch? Just to show a cursor.

just removing a bunch of old code we no longer use.

Comment 3

[Brian R. Bondy [:bbondy]]

I just meant if you couldn't get a cursor that bug makes that work.
It applies cleanly even with that bug by the way I tried.

ok so the goal of this patch is just to remove unused stuff.

Comment 4

[Jim Mathies [:jimm]]

(In reply to Brian R. Bondy [:bbondy] from comment #3)
> I just meant if you couldn't get a cursor that bug makes that work.
> It applies cleanly even with that bug by the way I tried.

Yeah, looking at that, it's unrelated. I ran into that because I had to add something to the click handler to invoke the selection handler earlier today, but it turned out you had already cleaned it up. :)

> 
> ok so the goal of this patch is just to remove unused stuff.

Yes. On android we had this form mode where a couple graphic arrows would come up on the screen and you would use those to navigate through forms rather than tapping. We disabled this and removed some of the code for it a long time ago, but nobody has had the time to go in and remove the form assistant cruft. I ran into this because formassist.open was mucking with the caret location.

Comment 5

[Brian R. Bondy [:bbondy]]

Comment on attachment 725585 [details] [diff] [review]
patch

Review of attachment 725585 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/metro/base/content/contenthandlers/FormHelper.js
@@ +284,5 @@
>            }
>            return;
>          }
>  
> +        if (focusedIndex != -1 && this._currentElement != focusedElement)

Although this won't cause any problems, focusedIndex no longer exists, so undefined != -1 is not needed.

Comment 6

[Jim Mathies [:jimm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/df0eeae20a56

Comment 7

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/df0eeae20a56