173569 - Mousing down (clicking)(double-clicking) in focused textbox should open autocomplete results (as in IE)

Status: RESOLVED FIXED

(Firefox :: Address Bar, enhancement, P3)

Description

[Blake Ross]

Like in IE.

Comment 1

[Devin Henderson]

i think the summary should be changed here. i'm not sure what 'mousing down' is.
in IE you must /double click/ an empty text field for the auto-complete list to
appear.

Comment 2

[Asa Dotzler [:asa]]

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

Comment 3

[Ben Grimm]

Actually in IE if the text box has the focus a single click brings up the
AutoComplete.  So it really only *appears* that it happens on a double click
when it's really just happening on the second click. 

Comment 4

[djk]

The click must be in an *empty* focused text box to bring up the list.  At least
in IE6.

Someone with Power, please adjust the summary to:
"Mousing down (clicking) in empty focused textbox should open autocomplete results"

PS- a nice autocomplete testing site is here:
http://wssg.berkeley.edu/public/projects/SecurityInfrastructure/reports/AutoComplete/index.html#Test%20AutoComplete

Comment 5

[David P James]

Taking QA. Sorry for the bugspam

Comment 6

[Pham]

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

Comment 7

[Kevin Ar18]

In reply to Comment #4.
Actually you can bring up autocomplete in a textbox that has characters in it if
the characters begin a word that's in the autocomplete list.  Mozilla Firebird
should do the same (it already does it when you press the down arrow).  I just
thought I'd mention this, so it doesn't get overlooked.

Comment 8

[Pham]

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

Comment 9

[David P James]

Pathetic. You'd think the Autocomplete QA would know his bugs better than to
file a dupe of an existing one.

Dupe-proofing the summary.

Comment 10

[Ben Goodger (use ben at mozilla dot org for email)]

-> me. 

Comment 11

[Ben Goodger (use ben at mozilla dot org for email)]

-> 0.9

Comment 12

[:deanis74]

Ben, gimme this one if you want.  I've got an inklin' of how to do this easily.

Comment 13

[Ben Goodger (use ben at mozilla dot org for email)]

Ask and you shall receive. 

Comment 14

[:deanis74]

Attachment: cvs diff -u toolkit/components/satchel/src

This adds double-click to show the drop-down.  I'm not sure if I like
single-click, because that can often happen accidentally.

Comment 15

[Kevin Ar18]

Dean, did you get the email I sent you about the single click?

Comment 16

[Kevin Ar18]

Well, of course I can't word it the same as a personal email, but anyways...  I
wanted to comment on the choice to go with double click in your proposed patch.
 I think that a single click would be a better option, and I was hoping to get
it reconsidered before the feature is accepted and checked in.

Background:
When I used to use IE, I used the autocomplete feature quite a bit.  The single
click access to the autocomplete was very useful.  Since I used the feature a
lot, quick access was always nice.
However, I believe that by using a double click method, the process would not be
as smooth for users who use this feature a lot.
In your comment, you noted that one of the reasons for using double click was
that it would prevent the autocomplete from happening accidentally.  However,
from what I can tell, even if the user brings it up accidentally, it would not
interfere with anything.
Basically, the autocomplete does not affect the texfield it any way; it doesn't
affect the focus, interfere with the typing, or affect any text selections.  In
addition, the autocomplete field is positioned below the textfield so that it
causes little or no interference.
 
Essentially, the autocomplete feature doesn't seem to cause any interference in
most cases, even if accidentally opened.  On the other hand, a single click
would be slightly prefered because of how it makes the process that much
smoother (and quicker); and for people who use the feature a lot, it's nice to
have even that little bit.

-------- 
Just so were are one the same track, here is a description of IE's method:
Assume that the textbox does not have focus.
In IE, the first click will give the box focus, and the second click will bring
up the drop down.
However, drag operations never bring up the autocomplete option.
In addition, repeated clicks will not cause the autocomplete to open and close
in a cycle; it will simply stay open.
Also, the autocomplete (like in Firefox) is positioned so that it does not
interfere with the texfield, modify the textfield, or take away its focus.
And finally, the autocomplete (like in Firefox) only shows up when stuff can be
autocompleted.  (For example, if you have the letter "e" in the texfield, and
the words "every" and "everyone" are in the autcomplete memory, then it would
show up, but if you typed in the letter "c", it would not.)

Comment 17

[:deanis74]

Attachment: patch v2

OK, try this one.  As far as I can tell it works just like IE does.

- double-click into an unfocused field will show the popup.
- single-click in a focused field will show the popup, regardless of where
  the cursor is in the field.
- the results are always filtered based on what the text is in the field.

The only difference I've found is that the popup will briefly hide when
double-clicking and it's already open, whereas IE's stays open.  I'm not going
to worry about that, it's only cosmetic.

The one thing I'm not happy with is the mIgnoreClicksTimer part.  If the user
clicks to focus the field, the focus event comes first and then the click
event.	Without this timer, clicking to focus a field would show the popup. 
There has to be a better way to tell if the focus event was caused by a mouse
click, but I couldn't find it today.

The only real change to |nsFormFillController::AddKeyListener()| is that I
removed the setting of |mFocusedInput|.  All other changes are just changing
|if (aInput)| to |if (!aInput)|.  That's the only change to
|RemoveKeyListener()| as well.

Comment 18

[[e-mail]]

(In reply to comment #17)
>There has to be a better way to tell if the focus event was caused by a
>mouse click, but I couldn't find it today.
Possibly by setting a shouldDropDown flag in the mousedown handler and clearing
it in the focus handler?

Comment 19

[:deanis74]

Attachment: patch v3

Thanks Neil, that was close enough to help me.	I set the flag in MouseDown and
clear it in MouseClick.  This patch works great now, and I'm happy with all of
it.  Ben, it's definitely ready for review now.

Comment 20

[:deanis74]

Attachment: patch v4

I had an extra QI in MouseDown().

Comment 21

[:deanis74]

Comment on attachment 144502 [details] [diff] [review]
patch v4

>-  // Start listening for key events
>-  AddKeyListener(aInput);
>+  // Start listening for key and mouse events
>+  AddInputListeners(aInput);
>+  mFocusedInput = aInput;

The comment is wrong.  It really doesn't need to be there at all, the function
name is self-explanatory.

Comment 22

[:deanis74]

Attachment: patch v5 (cvs diff -u mozilla/toolkit)

updated to the trunk

Comment 23

[:deanis74]

Attachment: last time - really!

cvs diff -u mozilla/toolkit

I missed one HandleText() call somehow.

Comment 24

[Kevin Ar18]

I would have to agree with Dean Tessman on the importance of this bug (I noticed
he proposed it as blocking Firefox 1.0).  This is very much a key feature that
Firefox is lacking for people migrating from IE.

Comment 25

[:deanis74]

Comment on attachment 146994 [details] [diff] [review]
last time - really!

Brian, care to review this?

Comment 26

[Ben Goodger (use ben at mozilla dot org for email)]

+ing since there's a patch.

Comment 27

[Kevin Ar18]

Jayfromtaiwan from the mozillazine forums provided a test build with this feature.
Here's the URL:
http://pryan.org/firefox/jayfromtaiwan/Fx-040504-Trunk-Oxp-GALF6-crc32-SVG-IEac.exehttp://pryan.org/firefox/jayfromtaiwan/Fx-040504-Trunk-Oxp-GALF6-crc32-SVG-IEac.exe

I did some basic testing of it so far, and it appears to work just fine:  it
seems to work just like IE's method.

Comment 28

[Brian Ryner (not reading)]

Comment on attachment 146994 [details] [diff] [review]
last time - really!

>--- components/autocomplete/src/nsAutoCompleteController.cpp	17 Apr 2004 05:53:33 -0000	1.16
>+++ components/autocomplete/src/nsAutoCompleteController.cpp	25 Apr 2004 17:33:04 -0000
>@@ -121,7 +121,7 @@
>   mInput->GetTextValue(newValue);
>   
>   // Reset all search state members to default values
>-  mSearchString = newValue;
>+  SetSearchString(newValue);

This changes the code to call a virtual method... I don't see any real benefit
to that.

>@@ -157,14 +157,14 @@
> NS_IMETHODIMP
> nsAutoCompleteController::StartSearch(const nsAString &aSearchString)
> { 
>-  mSearchString = aSearchString;
>+  SetSearchString(aSearchString);

Same.

>@@ -198,7 +198,7 @@
>     // way when you get a chance. -dwh
>     ClearResults();
> 
>-  mSearchString = newValue;
>+  SetSearchString(newValue);

Same.

>@@ -877,7 +889,7 @@
>   if (!value.IsEmpty()) {
>     mInput->SetTextValue(value);
>     mInput->SelectTextRange(value.Length(), value.Length());
>-    mSearchString = value;
>+    SetSearchString(value);

Same.

>--- components/satchel/src/nsFormFillController.cpp	18 Mar 2004 01:52:51 -0000	1.26
>+++ components/satchel/src/nsFormFillController.cpp	25 Apr 2004 17:33:04 -0000
>@@ -649,15 +652,108 @@
>   if (mSuppressOnInput)
>     return NS_OK;
> 
>-  return mController->HandleText();
>+  return mController->HandleText(PR_FALSE);
> }
> 
>+////////////////////////////////////////////////////////////////////////
>+//// nsIDOMMouseListener
>+
>+NS_IMETHODIMP
>+nsFormFillController::MouseDown(nsIDOMEvent* aMouseEvent)
>+{
>+  mIgnoreClick = PR_FALSE;
>+
>+  nsCOMPtr<nsIDOMEventTarget> target;
>+  aMouseEvent->GetTarget(getter_AddRefs(target));
>+  if (!target)
>+    return NS_OK;
>+
>+  nsCOMPtr<nsIDOMHTMLInputElement> targetInput = do_QueryInterface(target);
>+  if (targetInput && targetInput != mFocusedInput) {
>+    // A new input will be taking focus.  Ignore the first click
>+    // so that the popup is not shown.

How well does this work when the input clicked on does not take focus?	For
example, if it's disabled?

Comment 29

[:deanis74]

> >   // Reset all search state members to default values
> >-  mSearchString = newValue;
> >+  SetSearchString(newValue);
> 
> This changes the code to call a virtual method... I don't see any real benefit
> to that.

Those changes were made so that I can set the search string from
nsFormFillController::MouseClick.  Or do you mean just keep setting
mSearchString from within nsAutoCompleteController.cpp?  I thought I'd have
everything go through the same code path, in case we added something later on to
SetSearchString.

> How well does this work when the input clicked on does not take focus?	For
> example, if it's disabled?

It should work fine because the drop-down is only shown when an input has focus,
being when mFocusedInput is assigned.  mFocusedInput is cleared by
nsFormFillController:StopControllingInput, which is called by
nsFormFillController::Blur.  I'm pretty sure I did testing around this specific
issue.

Thanks for the review Brian.  Let me know how you want me to proceed with the
SetSearchString() calls.

Comment 30

[Kevin Ar18]

Note:
I ran accross a minor bug in this feature (something unexpected), that might be
worth noting.  I've already sent Dean an email with the details.  However, I'll
also include it here.  (Note: I have removed one of the steps that I sent to Dean.)

Steps to Reproduce:
1) Go to http://bugzilla.mozilla.org/query.cgi
2) Place your cursor in one of the textfields and cause the
autocomplete feature to come up (either using the mouse or keyboard or as part
of finishing a word you type).
3) While the autocomplete option is still open, hit enter to submit the
form.
4) The search results page will load, when it does, left click on the page.

Result:
An autocomplete window, will appear in the top, left corner of the screen.


-----------------------------
On another note, Brian asked:
> How well does this work when the input clicked on does not take focus?	For
> example, if it's disabled?
> 
Although I can't comment on the code part, I can say that during normal usage of
this feature, the autocomplete only showed up when the field first had focus. 
In addition, I also tested it on a disabled field just to make sure.  I first
populated the text field with autocomplete values and submitted the form.  Then
I disabled the textfield using javscript.  When I clicked on the disabled
textfield nothing happened (no autocomplete appeared).  When I re-enabled the
textfield using javascript, it began working like normal (the autocomplete
showed up once the textfield had focus).

Comment 31

[:deanis74]

Attachment: new patch

This addresses bryner's comment about calling SetSearchString() unnecessarily
and fixes the bug that kevin found.  I had to add code to
nsIFormListener::Submit in nsFormFillController.cpp.  To do that I had to move
the form listener from the input to the window.  There's sufficient
null-checking in there.

Comment 32

[:deanis74]

Comment on attachment 148815 [details] [diff] [review]
new patch

Brian, how's this one?

Comment 33

[José Jeria]

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

Comment 34

[Brian Ryner (not reading)]

Comment on attachment 148815 [details] [diff] [review]
new patch

>--- components/autocomplete/src/nsAutoCompleteController.cpp	25 Apr 2004 19:53:15 -0000	1.17
>+++ components/autocomplete/src/nsAutoCompleteController.cpp	19 May 2004 08:08:59 -0000
>@@ -164,7 +164,7 @@
> }
> 
> NS_IMETHODIMP
>-nsAutoCompleteController::HandleText()
>+nsAutoCompleteController::HandleText(const PRBool aIgnoreSelection)

I think the parameter type is just 'PRBool', not 'const PRBool'.


>--- components/satchel/src/nsFormFillController.cpp	3 May 2004 21:48:35 -0000	1.27
>+++ components/satchel/src/nsFormFillController.cpp	19 May 2004 08:08:59 -0000
>@@ -504,9 +507,13 @@
> {
>   nsCOMPtr<nsIDOMEventTarget> target;
>   aEvent->GetTarget(getter_AddRefs(target));
>+  if (!target)
>+    return NS_OK;
>   

The do_QueryInterface below is null-safe, you don't need to add this check.

>   nsCOMPtr<nsIDOMHTMLInputElement> input = do_QueryInterface(target);

>@@ -555,7 +561,12 @@
> NS_IMETHODIMP
> nsFormFillController::KeyPress(nsIDOMEvent* aEvent)
> {
>+  if (!mFocusedInput || !mController)
>+    return NS_OK;
>+

mFocusedInput should never be null if we get a key event; you should at least
assert that this is unexpected.  mController is created in the ctor... a better
way to do this would be to have an Init() method that creates mController, have
that fail if allocation fails, and don't worry about it other than that.

>   nsCOMPtr<nsIDOMKeyEvent> keyEvent = do_QueryInterface(aEvent);
>+  if (!keyEvent)
>+    return NS_OK;
> 

This would be quite unexpected (i.e. impossible) and we either shouldn't bother
checking or it should be an error.

>+////////////////////////////////////////////////////////////////////////
>+//// nsIDOMMouseListener
>+
>+NS_IMETHODIMP
>+nsFormFillController::MouseDown(nsIDOMEvent* aMouseEvent)
>+{
>+  mIgnoreClick = PR_FALSE;
>+
>+  nsCOMPtr<nsIDOMEventTarget> target;
>+  aMouseEvent->GetTarget(getter_AddRefs(target));
>+  if (!target)
>+    return NS_OK;
>+
>+  nsCOMPtr<nsIDOMHTMLInputElement> targetInput = do_QueryInterface(target);

same comment as above about do_QI being null-safe.

>+NS_IMETHODIMP
>+nsFormFillController::MouseClick(nsIDOMEvent* aMouseEvent)
>+{
>+  if (mIgnoreClick) {
>+    mIgnoreClick = PR_FALSE;
>+    return NS_OK;
>+  }
>+
>+  if (!mFocusedInput)
>+    return NS_OK;
>+
>+  nsCOMPtr<nsIDOMMouseEvent> mouseEvent(do_QueryInterface(aMouseEvent));
>+  if (!mouseEvent)
>+    return NS_OK;

Same comment as above about the unexpectedness of the event not QI'ing.

Looks good otherwise.  r=me with those changes.

Comment 35

[chris hofmann]

do we need sr/more review on this?  if not let get it in.

Comment 36

[Blake Ross]

Reassigning to me, I'll check it in and reassign the bug back to Dean
afterwards. I think he's busy and unable to checkin right now.

Comment 37

[Brian Ryner (not reading)]

checked in on branch and trunk.  reassigning to dean as blake suggested.

Comment 38

[Brian Ryner (not reading)]

... and marking fixed.

Comment 39

[Bill Gianopoulos [:WG9s]]

There appears to be a side effect of the check-in for this bug.  The symptoms
are similar to those mentioned in comment #30.  To see the issue, you need to
have used the searchbar so that there is autocomplete data present for the
search bar.  Next visit www.google.com and don't type anything or click anywhere
on the page.  Then select a page to load via a bookmark.  Then click anywhere on
the page.  The autocomplete data for the searchbar appears in a box at the upper
left corner of the window.

I had trouble duplicating this at first but once it starts happening it seems to
happen all the time.

This was first reported here:

http://forums.mozillazine.org/viewtopic.php?p=684980#684980

Comment 40

[Bill Gianopoulos [:WG9s]]

I forgot to mention that clicking on anything in the autocomplete list produces
a crash.  Here is the talkback incident:

http://talkback-public.mozilla.org/talkback/fastfind.jsp?search=2&type=iid&id=443679

Comment 41

[Ger Teunis]

(In reply to comment #39)
> There appears to be a side effect of the check-in for this bug.  The symptoms
> are similar to those mentioned in comment #30.  To see the issue, you need to
> have used the searchbar so that there is autocomplete data present for the
> search bar.  Next visit www.google.com and don't type anything or click anywhere
> on the page.  Then select a page to load via a bookmark.  Then click anywhere on
> the page.  The autocomplete data for the searchbar appears in a box at the upper
> left corner of the window.
> 
> I had trouble duplicating this at first but once it starts happening it seems to
> happen all the time.
> 
> This was first reported here:
> 
> http://forums.mozillazine.org/viewtopic.php?p=684980#684980

A bug has been filed on this behavure:
http://bugzilla.mozilla.org/show_bug.cgi?id=253709

Comment 42

[Asa Dotzler [:asa]]

This is broken. If clicking again a single time should dismiss the pop-up and it
doesn't. 

Comment 43

[Kevin Ar18]

(In reply to comment #42)
> This is broken. If clicking again a single time should dismiss the pop-up and it
> doesn't. 
By design, the feature was to remain open on subsequent clicks.  The part where
the autocomplete disappears for a split second and then reappears again is
actually a bug.  It was supposed to remain open the whole time.

Comment 44

[:deanis74]

That's correct.  IE's popup remains open with subsequent single clicks.  Back to
FIXED.

Comment 45

[James Slaughter]

Is the 'fix' to this bug the reason that, under certain circumstances, I
sometimes get a list of search terms I've entered into google appear in the
top-left corner of my screen when clicking on a page after selecting a bookmark,
even though the new page doesn't contain an edit box and I didn't double-click
on anything?

Comment 46

[Jesse Ruderman]

James, that regression was bug 253709, which is now fixed.

Comment 47

[Darin Fisher]

I know this is toolkit code, and we haven't reached 1.0 yet, but wouldn't it be
a good idea to rev the UUID for nsIAutoCompleteController?  I highly doubt there
are any external consumers of this interface, but you never know.  Better safe
than sorry, right?

Comment 48

[:deanis74]

Good idea, in retrospect.  It's been checked in for a month now, is it too late?

Comment 49

[Jim Ginn]

This probably caused Bug 258543.