48982 - Double clicking in a textfield should show list of available autofill entries

Status: RESOLVED FIXED

(Toolkit :: Form Manager, defect, P3)

Description

[Blake Ross]

Double-clicking in an HTML textbox (not a textarea, and not a password textbox) 
should open a menu containing available autofill entries for that field, like 
IE has.

If possible, in situations like Username/Password combinations, when the user 
double-clicks in the username textfield and chooses a username from the 
autofill list, the corresponding password should be automatically filled in the 
password box.  However, I don't think our current functionality supports this 
feature (unfortunately).

Split off from bug 48860.

Comment 1

[Blake Ross]

sorry for spam, cc rods.  Rod, how difficult would this be?

Comment 2

[Jeffrey Baker]

What would you do if the input box had contents?  In that case I would expect
the clicked-upon word to be selected.  I would not expect a menu to pop-up.

IMHO a better feature would be for autocompletion to work in a textfield the
same way that it works in the URL bar: a pop-up list of choices doesn't steal
focus, so you can ignore it.  You could use the context menu to show choices
without any typing.  Are there usability observations on this particular
behavior?

Comment 3

[Stephen P. Morse]

Much as I think this idea has merit, I would have to classify it as a new 
feature and as such it can't get implemented until after first release.  Pushing 
out to M20 (at least).

Comment 4

[sairuh (rarely reading bugmail)]

Jeffrey raises a good point about selection being invoked when there are 
contents in an html textbox --am cc'ing some editor folks who might be 
interested in/have input on this. also cc'ing radha to see if she has any 
thoughts on the autocompletion aspect.

Jeffrey, question about the context menu/popup idea --would this be a context 
menu that's different from the usual one raised (when using right mouse button 
click over content, links, etc.)? if yes, that could be problematic as that 
context menu could get...quite long. unless you meant a different context menu 
for html textboxes...?

Comment 5

[Blake Ross]

This menu should only appear if you dbl click in an empty textbox (I should 
have mentioned that).  This bug only covers this dbl clicking aspect of 
autocomplete.  A new bug (which I'll file soon, if no one else does) will cover 
the fact that typing in an HTML textbox should bring up the autocomplete menu 
(as does typing in the URL bar).

Comment 6

[sairuh (rarely reading bugmail)]

spam: mass-moving open password manager (single signon) and form manager
(autofill) bugs to Terri for qa contact. unfortunately, i cannot cc myself with
this form, so feel free and add me if you want to keep me in the loop with any
(but, pls not all :) of these... will also go thru 'em meself, a bit later...

Comment 7

[Simon Fraser [no longer active]]

um, what do [x] [y] and [z] mean?

Comment 8

[Terri Preston]

It's morse's own tracking system, I asked the same thing this am

Comment 9

[Stephen P. Morse]

*** Bug 59852 has been marked as a duplicate of this bug. ***

Comment 10

[Stephen P. Morse]

Attaching patch.  The way I've implemented it is to automatically prefill an 
empty text field when it is double-clicked on.  For the most part there is just 
a single saved value for each field so no pop-up list appears -- the prefill 
occurs immediately.  In those few cases where there are multiple values, the 
pop-up would be desirable and it is in the patch.  However it has several 
problems (see comments in the patch) and so is commented out for the present 
time.  A separate bug will be filed to get the pop-up list turned on once this 
patch is checked in.

The place that I found to insert this code is in contentAreaClick.js.  I don't 
know if this is the best place for it, or if it violates any abstraction model 
that the designer of that module had in mind.  So if this is the wrong place for 
it, please tell me what the right place is.

Blake Ross, this is your module.  So please code-review this patch.  Thanks.

Comment 11

[Stephen P. Morse]

Attachment: patch to prefill text field when double-clicked

Comment 12

[Stephen P. Morse]

BTW, this patch relies on a new API in the wallet module that is in the process 
of being checked in.  See patch in bug 48923 which is currently going through 
the approval process.  So the patch in this bug report cannot be checked in 
until the patch in bug 48923 is checked in.

Comment 13

[Blake Ross]

contentAreaClick.js is a place to handle clicks in browser-based content 
areas.  All of the functionality in the file will eventually be wrapped into a 
single <browser/> widget, so if you want textfields to have this prefill 
capability wherever they are (sidebar, mail message, etc.) -- and it sounds 
like they should -- then this is probably the correct place.

I haven't yet looked at the whole patch, but I did notice one problem -- you're 
calling prefillTextField() whenever a click in the content area is handled, and 
then you check to ensure that it was on the right node (a textfield), etc.  I 
would much prefer if you added those node checks to the |switch| structure 
already in place, then use those other checks (button, click number, 
|event.target.value.length|, etc.) as prerequisites to calling prefillTextField
(), instead of including them in the function itself.  This allows 
prefillTextField to be a little more flexible and generic, and the function 
won't even be called in this case if the right preconditions aren't met 
(contentAreaClick is supposed to be a condition-handler; when all the right 
conditions are met, the appropriate function is dispatched).

Also, I'm not sure if this check is even needed, but:

event.button == "1" // right mouse button

Isn't |1| the left mouse button?

Thanks for the patch!  I'll look at it more thoroughly after the above are 
addressed.

Comment 14

[Stephen P. Morse]

Oops, typo in comment.  That should have said "left mouse button"

Good point about moving the call inside the switch so that all the testing is 
not done unless we are on a text element.

However the switch is based on event.originalTarget whereas I need to test for 
event.target to determine if I have an element to prefill.  In this case 
event.originalTarget is returning "div" and I'm not even sure that that means in 
this context.  So I'm still following that with a check for event.target being 
"input".  I'm not sure that this is all necessary but I'm doing it to be safe in 
case there's such a thing as an original target being "div" but the target not 
being an input element.

Attaching new diff with the typo in the comment corrected and the testing moved 
to within the switch statement.

Comment 15

[Stephen P. Morse]

Attachment: type fixed, testing moved to within the switch statement

Comment 16

[Blake Ross]

Hmm.  dbaron says inputs construct anonymous divs in C++.  We can't really do 
that div check in the latest patch...how about just making the switch check for 
event.target for the time being?

e.g.:

  function contentAreaClick(event) 
  {
    var target = event.target;
    var linkNode;
    switch (target.localName.toLowerCase()) {
      case "a":
        linkNode = target;
        break;
      case "area":
        if (target.href) 
          linkNode = target;
        break;
      default:
        linkNode = findParentNode(event.originalTarget, "a");
        break;
    }
    if (linkNode) {
      handleLinkClick(event, linkNode.href);
      return true;
    }
    if (pref && event.button == 2 &&
        !findParentNode(event.originalTarget, "scrollbar") &&
        pref.GetBoolPref("middlemouse.paste")) {
      middleMousePaste(event);
    }
    return true;
  }

Also, that prefilling code doesn't belong in this file, but since jag and I are 
working to restructure all of this anyways (I have a bug on it), I wouldn't 
worry about it...

Comment 17

[Stephen P. Morse]

Blake,

So let me see if I understand what you are saying.

1. Change the switch condition by modifying the existing line
      var target = event.originalTarget;
to
      var target = event.target;
but make sure to still use the originalTarget test later on when checking for 
event.button == 2.  Presumably you understand this well enough to know that this 
change on the switch will not affect existing behavior.

2. Put the call to my code into the switch under a new case, namely "input".

Attaching new patch for what I think you are saying.

Comment 18

[Stephen P. Morse]

Attachment: Changing event.originalTarget to event.target in switch statement

Comment 19

[Blake Ross]

originalTarget is useful for XBL and anonymous content.  So changing to 
event.target could conceivably make some of this added functionality not work 
in obscure cases, but it's not that big of a deal (the worst that could really 
happen is that, for example, shift+click on a certain link wouldn't save 
it...).  jag and I will be addressing this soon in our cleanup, anyways.

You should use the rewritten function I provided, because we still need to 
check for event.originalTarget in findParentNode() and when testing for the 
scrollbar.

Comment 20

[Stephen P. Morse]

I thought I was using the rewritten function that you provided but I see that I 
missed changing to originalTarget in one place as you pointed out.  Attaching 
re-revised patch.

Comment 21

[Stephen P. Morse]

Attachment: Changing back to originalTarget in one place that I missed before

Comment 22

[Blake Ross]

Some stylistic nitpicks:

+  // If double-clicking on an empty text field, attempt to prefill it

Please change this to something more descriptive and appropriate (since the 
function is no longer specific to double clicking on an empty textfield), or 
remove it if you want.

+    var value = walletService.WALLET_PrefillOneElement
+      (window._content, event.target);

The line right above it is much longer, so why break this one but not that 
one?  Let's be consistent.  This is also a weird spot to break at...

+    if (value != "") {

How about |if (value)|?

+      var BREAK = value[0];

Is there a reason for this to be all-caps?

I'd probably check for type="text" (or type="") first in the |case "input"|, 
since we don't care what button or how many clicks if isn't a textbox.
Also, passing in the whole event can be expensive, and it looks like you don't 
need it.  You only need |event.target|, so why not just pass in |target| (the 
variable in contentAreaClick())?

I didn't really look over the commented out code since that will be handled in 
another bug.  However, I noticed that when there are multiple entries, a dialog 
is opened...I think a menu-type widget similar to what IE has is much 
preferable, and provides the quick/easy access and convenience that another 
dialog can't.  I think we'll want to create a new xbl widget or extend a 
current one to do this, since a simple menu won't suffice (it needs additional 
functionality, like scrolling, resizing, and so forth -- although Ben does have 
resizable popups working).  Also, point 1 doesn't seem to be much of a problem -
- why not just create a new .properties file?

I don't think any of the above warrants my review of a new patch (although I 
recommend attaching one for the superreviewer).  r=blake with the recognition 
of the fact that this function doesn't belong in this file (my fault, not 
yours), but the assurance that jag and I are working to refactor all of this 
code, and will fix this at the same time.

cc alec for sr

Comment 23

[Stephen P. Morse]

Attachment: patch to address reviewer's comments

Comment 24

[Stephen P. Morse]

Above patch addresses the reviewers comments.  Howeve I have some comments on 
those comments:

> I'd probably check for type="text" (or type="") first in the |case "input"|, 
> since we don't care what button or how many clicks if isn't a textbox.

I can turn that argument around and say "we don't care what type of element it 
is if it isn't a double click."  Unless somebody has done a study to show that 
there are more instances of double-clicks than there are of textboxes, the 
ordering here is really insignificant.  However, since it makes you happier, 
I've reversed the ordering.

> I didn't really look over the commented out code since that will be handled
> in another bug.  However, I noticed that when there are multiple entries, a
> dialog is opened...I think a menu-type widget similar to what IE has is much
> preferable

That was exactly my thought as well, and is another reason that I commented out 
this code for the present.

> Also, point 1 doesn't seem to be much of a problem -- why not just create a
> new .properties file?

I didn't say it couldn't be done, I just said it wasn't "simple", meaning it 
was not as simple as adding an entry to the existing .dtd file.  Adding a new 
.properies file means adding an entry to a manifest so that's yet another file 
that needs to be changed.  However this is all a moot point since I plan on 
using a drop-down menu and that doesn't involve any localizable strings.

Comment 25

[Blake Ross]

> I can turn that argument around and say "we don't care what type of element it
> is if it isn't a double click."  Unless somebody has done a study to show 
> that there are more instances of double-clicks than there are of textboxes, 
> the ordering here is really insignificant.  However, since it makes you 
> happier, I've reversed the ordering.

Agreed that it doesn't really matter.  My thinking was that, since there's many 
types of |input|s, we'd eventually want a |switch| or long |if| block in there 
to test for them all, when the need arose...in which case it'd be a bit easier 
if that check was first.  *shrug*

Comment 26

[Blake Ross]

Sorry, one more thing (doesn't affect my r=):

-      middleMousePaste(event);
+      middleMousePaste(event.target);

middleMousePaste() takes the whole event, so this isn't okay, but I just 
realized we don't need to pass in the whole event.  I recommend removing the 
event parameter, and doing |event.preventBubble| in the |if| block instead if 
middleMousePaste() returns true (since we really should let it bubble if we're 
not handling it...)

e.g.:

    if (pref && event.button == 2 &&
        !findParentNode(target, "scrollbar") &&
        pref.GetBoolPref("middlemouse.paste")) {
      var rv = middleMousePaste(event);
      if (rv)
        event.preventBubble();
    }

Or something like that.

Comment 27

[Blake Ross]

Argh...var rv = middleMousePaste();, not var rv = middleMousePaste(event);, of 
course.

Comment 28

[Stephen P. Morse]

Blake, the latest comment of yours has nothing to do with the current bug 
(actually feature request) of prefilling when double-clicking on a text field.  
Nor does it have anything to do with the patch presented here.  It sounds like 
it's a completely unrelated bug that you just found which happens to be in the 
same file.  That really should go into a separate bug report.  The risk of 
piggy-backing this new bug along with the patch I'm presenting here is that the 
super-reviewer might find problems with the fix for this new bug and that would 
prevent getting the patch for the current problem checked in.

Having said that, I'm including your changes and generating a new patch.  But I 
hope that doesn't delay the fix for the original bug.

Comment 29

[Stephen P. Morse]

Attachment: Include fix for unrelated bug into patch

Comment 30

[Stephen P. Morse]

Wait, I misunderstood your last comment.  On rereading, I understand better what 
you were saying.  It turns out my last patch introduced the problem of 
event.target instead of event and that's what you were commenting on.  In that 
case, your latest comment was indeed in order for this patch so it's a good 
thing that I decided to incorporate it.  Sorry.

Comment 31

[Blake Ross]

No problem.

But this is :-) :

+      if (middleMousePaste() {

( closing ')' )

Comment 32

[Stephen P. Morse]

I can't believe I did that.  I guess I was just getting tired of redoing this 
patch that I finally got sloppy.  But it does prove my point that by trying to 
piggyback an additional change in, we would get ourselves in trouble.

Reattaching new patch with the missing paren added.

Comment 33

[Stephen P. Morse]

Attachment: Patch with missing paren added

Comment 34

[Blake Ross]

+    var value = walletService.WALLET_PrefillOneElement(window._content, 
target);

|window| doesn't need to be specified there.  Don't care if you leave it or 
not...

r=blake with the change

Comment 35

[Alec Flett]

ok, looks good. sr=alecf on the most recent patch, I think it's useful to be
explicit with window.*, esp when other parts of the file are consistent about that.

Comment 36

[Stephen P. Morse]

It's true that the window parameter is currently not being used by the API.  
It's there in case the wallet module decides in the future to put up a modal 
dialog.  I've been burned before by not having the parent window available for 
such dialogs, so this was just anticipation to avoid future headaches.

Comment 37

[Stephen P. Morse]

This bug now has r= and sr=.  However I can't check it in until the patch in 
48923 is checked in since that one contains the other side of the API that 
this patch uses.  That bug report is still awaiting reviews.

If anybody wants to expidite that bug by doing a review on it, that would be 
appreciated.

Comment 38

[Stephen P. Morse]

Forget what I said above about the window parameter not beeing used -- I was 
talking from memory instead of really looking at the code).  I definitely do 
need the window parameter in the API.  It is used to obtain the nsIDocument 
which I need in order to initialize certain wallet functions.

Sorry if I confused anybody.

Comment 39

[Stephen P. Morse]

And, besides, my comment was totally irrelevant.  I thought that Blake was 
referring to the parameter not being needed whereas all he was referring to was 
that the "window." prefix on the parameter was unnecessary.

Comment 40

[Stephen P. Morse]

Fix checked in.  The outstanding issue of displaying a list if there is more 
than one possible value is now the topic of bug 63320.