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

RESOLVED FIXED

Status

()

P3
normal
RESOLVED FIXED
18 years ago
10 years ago

People

(Reporter: bugzilla, Assigned: morse)

Tracking

Trunk
x86
Windows 98
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(7 attachments)

(Reporter)

Description

18 years ago
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.
(Reporter)

Updated

18 years ago
Depends on: 48860
(Reporter)

Comment 1

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

Updated

18 years ago
No longer depends on: 48860

Updated

18 years ago
Blocks: 48860

Comment 2

18 years ago
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?
(Assignee)

Comment 3

18 years ago
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).
Status: NEW → ASSIGNED
Target Milestone: --- → M20
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...?
(Reporter)

Comment 5

18 years ago
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).
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...
QA Contact: sairuh → tpreston
(Assignee)

Updated

18 years ago
Summary: Double clicking in a textfield should show list of available autofill entries → [x]Double clicking in a textfield should show list of available autofill entries
Target Milestone: M20 → ---

Comment 7

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

Comment 8

18 years ago
It's morse's own tracking system, I asked the same thing this am
(Assignee)

Comment 9

18 years ago
*** Bug 59852 has been marked as a duplicate of this bug. ***
(Assignee)

Updated

18 years ago
Summary: [x]Double clicking in a textfield should show list of available autofill entries → Double clicking in a textfield should show list of available autofill entries
Whiteboard: [x]
(Assignee)

Comment 10

18 years ago
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.
(Assignee)

Comment 11

18 years ago
Created attachment 20799 [details] [diff] [review]
patch to prefill text field when double-clicked
(Assignee)

Comment 12

18 years ago
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.
(Assignee)

Updated

18 years ago
Depends on: 48923
(Reporter)

Comment 13

18 years ago
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.

(Assignee)

Comment 14

18 years ago
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.
(Assignee)

Comment 15

18 years ago
Created attachment 20803 [details] [diff] [review]
type fixed, testing moved to within the switch statement
(Reporter)

Comment 16

18 years ago
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...
(Assignee)

Comment 17

18 years ago
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.
(Assignee)

Comment 18

18 years ago
Created attachment 20806 [details] [diff] [review]
Changing event.originalTarget to event.target in switch statement
(Reporter)

Comment 19

18 years ago
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.
(Assignee)

Comment 20

18 years ago
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.
(Assignee)

Comment 21

18 years ago
Created attachment 20807 [details] [diff] [review]
Changing back to originalTarget in one place that I missed before
(Reporter)

Comment 22

18 years ago
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
(Assignee)

Comment 23

18 years ago
Created attachment 20828 [details] [diff] [review]
patch to address reviewer's comments
(Assignee)

Comment 24

18 years ago
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.
(Reporter)

Comment 25

18 years ago
> 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*
(Reporter)

Comment 26

18 years ago
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.
(Reporter)

Comment 27

18 years ago
Argh...var rv = middleMousePaste();, not var rv = middleMousePaste(event);, of 
course.
(Assignee)

Comment 28

18 years ago
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.
(Assignee)

Comment 29

18 years ago
Created attachment 20830 [details] [diff] [review]
Include fix for unrelated bug into patch
(Assignee)

Comment 30

18 years ago
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.
(Reporter)

Comment 31

18 years ago
No problem.

But this is :-) :

+      if (middleMousePaste() {

( closing ')' )
(Assignee)

Comment 32

18 years ago
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.
(Assignee)

Comment 33

18 years ago
Created attachment 20858 [details] [diff] [review]
Patch with missing paren added
(Reporter)

Comment 34

18 years ago
+    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

18 years ago
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.
(Assignee)

Comment 36

18 years ago
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.
(Assignee)

Comment 37

18 years ago
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.
(Assignee)

Comment 38

18 years ago
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.
(Assignee)

Comment 39

18 years ago
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.
(Assignee)

Comment 40

18 years ago
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → FIXED
(Assignee)

Updated

18 years ago
Whiteboard: [x]

Updated

10 years ago
Component: Form Manager → Form Manager
Product: Core → Toolkit
QA Contact: tpreston → form.manager
You need to log in before you can comment on or make changes to this bug.