Last Comment Bug 286933 - Key events in the autocomplete popup should be hidden from page scripts
: Key events in the autocomplete popup should be hidden from page scripts
Status: NEW
[parity-trident][parity-blink][parity...
: regression, site-compat, testcase, top100
Product: Toolkit
Classification: Components
Component: Autocomplete (show other bugs)
: Trunk
: All All
: P2 major with 15 votes (vote)
: ---
Assigned To: Nobody; OK to take it and work on it
:
Mentors:
http://search.msn.com/
: 297329 322658 342342 408176 901325 1011780 1064001 (view as bug list)
Depends on: 372015 374881 1121040
Blocks: 364733 819616 autocompleteLeak
  Show dependency treegraph
 
Reported: 2005-03-20 04:11 PST by Ali Ebrahim
Modified: 2015-11-03 15:08 PST (History)
46 users (show)
gijskruitbosch+bugs: firefox‑backlog+
asa: blocking1.8rc1-
mconnor: blocking‑firefox2-
gijskruitbosch+bugs: in‑testsuite?
gijskruitbosch+bugs: qe‑verify+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (1.15 KB, text/html)
2005-10-08 12:09 PDT, Jesse Ruderman
no flags Details
testcase modified to "work" (1.33 KB, text/html)
2005-10-21 01:43 PDT, Daniel Veditz [:dveditz]
no flags Details
patch (4.68 KB, patch)
2006-09-19 09:26 PDT, Mano (::mano, needinfo? for any questions; not reading general bugmail)
mconnor: first‑review+
jaymoz: approval1.8.1.2+
Details | Diff | Review
input.html (569 bytes, text/html)
2013-08-04 08:17 PDT, Alice0775 White
no flags Details

Description Ali Ebrahim 2005-03-20 04:11:49 PST
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050319
Firefox/1.0+

On search.msn.com, when you enter a search term that brings up the autocomplete
box, and you use arrows to select the entry you want to autocomplete and then
press enter, MSN search captures what you entered into the search box, not the
entry that was autocompleted.

Steps to Reproduce:
1. http://search.msn.com/
2. Start typing a partial search term that will bring up the autocomplete box
3. Arrow key down to the thing you want to select
4. Hit enter

Expected Results:

Assume your term in step 2 was 'moz', and from step 3 'mozilla', it should
perform a search on 'mozilla'.

Actual Results:

It performed a search on 'moz'.

We should hide the autocomplete enter key press from page scripts. Jesse thinks
this might be a regression from bug 270697, so I'm marking it as such.
Comment 1 Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 2005-10-02 11:23:37 PDT Comment hidden (obsolete)
Comment 2 Jesse Ruderman 2005-10-08 11:52:23 PDT
I still see this bug at http://search.msn.com/ (as described in comment 0) using
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20051005
Firefox/1.6a1.
Comment 3 Jesse Ruderman 2005-10-08 12:09:05 PDT
Created attachment 198950 [details]
testcase
Comment 4 Jesse Ruderman 2005-10-08 12:12:05 PDT
To reproduce with the testcase, save the testcase and use the local copy.  The
testcase's behavior gets even more strange when it is loaded from https.
Comment 5 Adam Guthrie 2005-10-12 13:54:32 PDT
*** Bug 297329 has been marked as a duplicate of this bug. ***
Comment 6 tyl2 2005-10-14 16:57:14 PDT
I believe that this bug is related to Bug #311795. Both bugs were once fixed on
the 1.0.x branch and have resurfaced sometime before 1.5b1 was released and now
exist everywhere.
Comment 7 tyl2 2005-10-14 17:00:34 PDT
I believe that this bug is related to Bug #311795. Both bugs were once fixed on
the 1.0.x branch and have resurfaced sometime before 1.5b1 was released and now
exist everywhere. They seem to point to a more general problem of keypresses
being sent to page scripts instead of the autocomplete dropdown.
Comment 8 Asa Dotzler [:asa] 2005-10-14 23:07:25 PDT Comment hidden (obsolete)
Comment 9 Adam Guthrie 2005-10-15 10:56:30 PDT
With the latest branch builds on Windows XP I still see this. 

1. Get test, testing, and testing123 in the autocomplete history.
2. Type "te" into the form and arrow down to "testing123".
3. Hit <enter>.

Note that "te" was searched for instead of "testing123".
Comment 10 Asa Dotzler [:asa] 2005-10-17 11:25:57 PDT
I still cannot reproduce with those steps on today's build on windowsXP. I don't
think we can block on a bug like this that's not easily reproduced. 
Comment 11 Mike Beltzner [:beltzner, not reading bugmail] 2005-10-17 13:52:27 PDT
Asa, I can confirm this, although I'm not knowledgeable enough to tell if it's
the way JS is being (ab)used by the various sites or the way we're interpreting
things. However, using the steps in comment 9 (at either staples.com or
search.msn.com as two examples) I do notice that although the browser renders
the full "testing123" into the search box and then loads the results page, the
actual submitted search term is "te"!
Comment 12 Mike Beltzner [:beltzner, not reading bugmail] 2005-10-17 18:03:23 PDT
Renominating for 1.8rc1 - I've been able to recreate this with new and existing
profiles, and the behaviour is (unintentionally) sneaky enough to actually make
a user thing that they've searched for "football" when, in fact, they've
searched for "foo".
Comment 13 Tracy Walker [:tracy] 2005-10-18 11:58:05 PDT
I can repoduce this also.  In an autocomplete list at search.msn.com "Enter"
submits what's typed in the search field, "enter should behave like clicking on
the autocomplete item; it fills in the search field.

Doing steps from comment 9 at google.com does what is expected. A second "Enter"
is then required to submit the search. (or click the search button)
Comment 14 Asa Dotzler [:asa] 2005-10-18 14:24:33 PDT
Tracy, can you get us a regression window so we can get blame for it? Thanks.
Comment 15 Adam Guthrie 2005-10-18 16:41:07 PDT
Jesse said he thought this might be caused by bug 270697. I checked and this
regressed between 2005-02-17 07:00 and 2005-02-18 07:00, during which the patch
for bug 270697 was checked in. 

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=PhoenixTinderbox&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2005-02-17+07&maxdate=2005-02-18+07&cvsroot=%2Fcvsroot
Comment 16 Jesse Ruderman 2005-10-18 17:34:41 PDT
Dan, this regression appeared when you fixed bug 270697, "Autocomplete data leak".
Comment 17 Asa Dotzler [:asa] 2005-10-20 18:03:00 PDT
bryner and johnny, can you guys look at this? 

Dveditz, can you put your analysis here so bryner and jst can see what you've
determined so far. 
Comment 18 Daniel Veditz [:dveditz] 2005-10-21 01:39:35 PDT
The problem is that the form's key listener code snags the return and submits
the form, and the autocomplete control never gets the enter keypress. If you
turn off Javascript then the autocomplete works correctly. If you change the
event listener to not eat the event then the autocomplete HandleEnter() does get
called and does the right thing populating the field, but the form data has
already been submitted without the new data.

I don't know that this behavior is actually wrong. Clearly it's not what the
page author wants, but I don't think we want to make autocomplete be a magic
widget that gets events regardless of whether page content is trying to claim
them. Are any of our other widgets like that? The problem is the page was
written with the buggy pre-270697 behavior in mind, but I argue strongly that we
shouldn't back that out. It was a privacy security issue, and also didn't behave
like the IE autocomplete.

IE's autocomplete dropdown seems to eat the event -- the page's keypress handler
submits the page only when Enter is pressed and autocomplete popup is closed.

The page can be re-written to work as intended: change the keypress handler to
not eat the event and call the submit in a timeout. Or really, if they simply
dropped the the handler it would behave like IE: if the autocomplete popup is
open Enter fills the field but doesn't submit, and if the popup is closed Enter
submits.

I'd like to call this a Tech Evangelism bug.
Comment 19 Daniel Veditz [:dveditz] 2005-10-21 01:43:51 PDT
Created attachment 200328 [details]
testcase modified to "work"

The modified testcase is identical to the first except that the submit is
called in a timeout and the event is passed on. Comments mark the changed
lines.
Comment 20 Jesse Ruderman 2005-10-21 01:50:41 PDT
I wasn't saying that we should back bug 270697 out, but that pages shouldn't get
key events that are directed at autocomplete menus.  When you're navigating
within autocomplete, it could be argued that you're interacting with browser UI,
so pages shouldn't get events just like they shouldn't get events when you
navigate browser menus.
Comment 21 Daniel Veditz [:dveditz] 2005-10-21 15:43:49 PDT
(In reply to comment #20)
> When you're navigating within autocomplete, it could be argued that you're
> interacting with browser UI, so pages shouldn't get events just like they
> shouldn't get events when you navigate browser menus.

It's a reasonable argument, but the last days of 1.5 isn't the time to re-write
the way all of our XBL-based widgets work.

Comment 22 tyl2 2005-10-31 07:38:41 PST
The calendar pages on yahoogroups.com now succumb to this behavior as well. I haven't checked out the other pages on the same website. I'm not sure if I should file a new evangelism bug for each new site I come across that displays this behavior, so sorry if I'm spamming. I rather think it's a UI issue, but maybe I shouldn't report every site that offends, either. Please advise. Thanks.
Comment 23 Mike Connor [:mconnor] 2006-01-05 17:57:16 PST
I don't think page events should be able to eat the event pointed at the autocomplete widget.  We can either match IE's behaviour, or we can try to TE the world.  In this case, IE does the right thing, IMO, so we need to find a way to do that right.
Comment 24 Adam Guthrie 2006-01-08 22:27:49 PST
*** Bug 322658 has been marked as a duplicate of this bug. ***
Comment 25 Peter Flynn (Oracle) 2006-03-21 14:00:12 PST
I agree with Mike Connor. FF can't ask every site to submit every form on a timeout just because autocomplete might be enabled.

Further, users probably don't want selecting an autocomplete item (by pressing enter) to submit the form right away. What if the autocomplete was for the first field out of several needing to be filled out?
Comment 26 kitchin 2006-06-21 21:40:01 PDT
*** Bug 342342 has been marked as a duplicate of this bug. ***
Comment 27 kitchin 2006-06-21 22:03:17 PDT
Throws an excpetion to the JS console, by the way:

Permission denied to set property XULElement.selectedIndex'
when calling method: [nsIAutoCompletePopup::selectedIndex]"  nsresult:
"0x8057001e (NS_ERROR_XPC_JS_THREW_STRING)"  location: "JS frame...

For what it's worth, I agree Autocomplete events belong to chrome not to the user.  If the page author wants to capture Enter it is probably to submit the form.  A useful script for the current FF would need to know if Enter came from Autocomplete or not.  The meaning of Enter is not the same in Autocomplete. So using Timeout just corrects one part of the problem, but replicates the other problem, a little later.  The user did not use Enter to mean "Submit" or "Done".  It just meant "select this item."  If the user used a mouse click to select the Automcomplete item, the same meaning, it does not trigger the event.
Comment 28 Mike Connor [:mconnor] 2006-07-17 23:15:06 PDT
Moving to nom list for reconsideration, likely to be minused if we can't get traction really soon.
Comment 29 Michael Wu [:mwu] 2006-07-20 10:00:07 PDT
Why is the timeout necessary? Switching the event handler from capturing to bubbling seems to have fixed the testcase fairly easily. (though we should still eat the enter keypress event)
Comment 30 Mike Connor [:mconnor] 2006-07-20 10:10:39 PDT
We're not going to block on this at this point, but will consider taking a fix if a safe patch appears in the next day or so.
Comment 31 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2006-09-19 09:26:05 PDT
Created attachment 239184 [details] [diff] [review]
patch

This should be safe to do since the keypress handler bails out if there's no input focused (!mFocusedInput).
Comment 32 Mike Connor [:mconnor] 2006-09-19 09:46:09 PDT
Comment on attachment 239184 [details] [diff] [review]
patch

r=me

simple and sane, thanks for fixing this!
Comment 33 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2006-09-19 09:49:55 PDT
mozilla/toolkit/components/satchel/src/nsFormFillController.cpp 1.75
mozilla/toolkit/components/satchel/src/nsFormFillController.h 1.14
Comment 34 Ryan VanderMeulen [:RyanVM] 2006-12-10 15:22:24 PST
Is this safe enough for 2.0.0.2?
Comment 35 Jay Patel [:jay] 2006-12-27 15:05:14 PST
Comment on attachment 239184 [details] [diff] [review]
patch

Approved for 1.8 branch, a=jay for drivers.
Comment 36 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2007-01-15 16:56:12 PST
1.8 branch:
mozilla/toolkit/components/satchel/src/nsFormFillController.cpp 1.51.4.18
mozilla/toolkit/components/satchel/src/nsFormFillController.h 1.11.4.4
Comment 37 Carsten Book [:Tomcat] 2007-01-29 13:31:06 PST
Verified Fixed for 1.8.1.2 - after tests with Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.8.1.2pre) Gecko/20070129 BonEcho/2.0.0.2pre ID:2007012903 on XP x64 and on Linux Fedora FC 6
Comment 38 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2007-04-04 12:55:51 PDT
Backed-out from both trunk and branch (with a=dveditz), nice try I say. :-/
Comment 39 Jesse Ruderman 2007-12-13 10:22:35 PST
*** Bug 408176 has been marked as a duplicate of this bug. ***
Comment 40 Nickolay_Ponomarev 2010-01-09 08:01:06 PST
Added similar bug 364733, which is about keydown (patches here seem to only affect the keypress event).
Comment 41 Tobias Hoffmann 2011-05-16 05:05:39 PDT Comment hidden (obsolete)
Comment 42 Leonardo Telles 2011-06-29 12:17:10 PDT Comment hidden (obsolete)
Comment 43 White Tiger 2011-10-10 11:52:03 PDT
This is a 6-year old bug. Please fix it. It completely ruins user experience that the Enter key of Autocomplete is propagated to the form _before_ the autocomplete value is set in the field. Either block this Enter completely from reaching the user's script or at least fill the field with the autocomplete value before propagating the event.
Comment 44 Leonardo Telles 2011-10-10 12:05:02 PDT
(In reply to White Tiger from comment #43)
> This is a 6-year old bug. Please fix it. It completely ruins user experience
> that the Enter key of Autocomplete is propagated to the form _before_ the
> autocomplete value is set in the field. Either block this Enter completely
> from reaching the user's script or at least fill the field with the
> autocomplete value before propagating the event.

Not really sure if filling the field before propagating to the user's script would be a solution. In case the user is logging in a website(usually email and password) he would get a wrong password message just for using the autocomplete.
But it should be patched, anyway.
Comment 45 Wilson Pascoal 2012-01-10 11:13:24 PST
This is still a bug in Firefox 9.0.1.

I have an onkeypress event on the form which is generated by ASP.NET. If users press enter to select an item from their autocomplete, it submits the incomplete form. This is terrible for user registration forms.

Any keypress events within an autocomplete dropdown should NOT propagate to the user scripts in any way.
Comment 46 John Snyders 2012-08-15 12:45:27 PDT
This is still a bug in Firefox 14.0.1
The keys used to navigate and select an item from the drop down list of autocomplete choices should not be sent as DOM keypress or possibly keydown events. Sending as keyup is OK. The enter key is the most problematic since it is commonly used to submit a form.

I tested to see which events could detect the enter key when pressed to select an autocomplete choice in a few browsers:
IE 9: keydown, keyup
Chrome 21: keyup
Firefox 14: keydown, keypress, keyup

This leaves now way in Firefox to distinguish the enter key used to select an autocomplete choice from an enter key pressed while in the text input.

My code is currently checking for enter in keypress so it works with autocomplete in Chrome and IE but not Firefox. I think the behavior in chrome makes the most sense.

The workaround requires the user to use right arrow rather than enter to select an autocomplete choice.
Comment 47 Alice0775 White 2013-08-04 08:17:15 PDT
Created attachment 785507 [details]
input.html

Press ENTER in the dorpdown list(list item/autocomprete) fires keypress event.
Comment 48 Loic 2013-08-04 09:23:27 PDT
*** Bug 901325 has been marked as a duplicate of this bug. ***
Comment 49 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2013-08-04 22:23:14 PDT
Yes, and for some reason I see more and more pages affected lately. I'm going to try to make my patch from 2006 work without reintroducing bug 372015.
Comment 50 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2013-08-04 22:26:17 PDT
By the way, a similar issue applies to <select>s. See eBay's advanced search page for some (very annoying!) examples.
Comment 52 m.kurz 2014-01-09 02:17:52 PST Comment hidden (off-topic)
Comment 53 m.kurz 2014-01-09 02:46:58 PST
Informations and another discussion about this bug can be found here too: http://stackoverflow.com/q/2303955
Comment 54 :Gijs Kruitbosch 2014-09-08 13:23:15 PDT
*** Bug 1064001 has been marked as a duplicate of this bug. ***
Comment 55 :Gijs Kruitbosch 2014-09-08 13:34:23 PDT
*** Bug 1011780 has been marked as a duplicate of this bug. ***
Comment 56 :Gijs Kruitbosch 2014-09-08 14:58:59 PDT
From poking at this for half an hour: this is all fun and games because we have approximately these requirements (caveat emptor per the previous indication of how long I looked at this):

1) if the popup isn't open, listen for the key event in the default action phase, and only then open the popup if it's not been preventDefaulted
2) if the popup is open, listen for the key event as early as possible (probably on window, as a capturing event listener, potentially system group?) and kill the event then and there if and only if we act on it
3) do this for all of keydown, keypress and keyup, but don't act twice (e.g. single key down / key press / key up should move only 1 item down in the autocomplete popup, but we should stop all 3 events from reaching the webpage)
4) do this for both the handling inside the autocomplete popup binding (without leaking the world if the binding is destroyed / disconnected) and nsFormFillController

I do think we should fix this sooner rather than later, so I'm setting backlog+. Dolske, can you adjudicate whether this should be prioritized more than just that? :-)
Comment 57 :Gijs Kruitbosch 2014-09-08 15:00:49 PDT
(In reply to :Gijs Kruitbosch from comment #56)
> 1) if the popup isn't open, listen for the key event in the default action
> phase, and only then open the popup if it's not been preventDefaulted

(from: https://bugzilla.mozilla.org/show_bug.cgi?id=633058#c8 )
Comment 58 dE 2014-09-16 01:46:12 PDT Comment hidden (advocacy)
Comment 59 Justin Dolske [:Dolske] 2014-10-04 22:28:23 PDT
(In reply to :Gijs Kruitbosch from comment #56)

> I do think we should fix this sooner rather than later, so I'm setting
> backlog+. Dolske, can you adjudicate whether this should be prioritized more
> than just that? :-)

We should fix this, but it doesn't have any particular urgency (he says, commenting on a 9+ year old bug). Might be worth fleshing out comment 56 a bit to make it more actionable by someone who wants to pick it up?
Comment 60 Matthew N. [:MattN] (behind on reviews) 2015-03-02 14:10:04 PST
In bug 1121040 I hid the RETURN keypress from pages when an entry is selected in the autocomplete popup. This bug should handle keypress events for other keys and also deal with hiding keydown/keyup from the page at the same time as keypress. See comment 56.

Note You need to log in before you can comment on or make changes to this bug.