598819 - Firefox magically restores old <input> value when ESC is pressed

Status: RESOLVED DUPLICATE of bug 1407085

(Toolkit :: Form Manager, defect)

Description

[ae]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US) AppleWebKit/534.3 (KHTML, like Gecko) Chrome/6.0.472.55 Safari/534.3
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.9.2) Gecko/20100115 Firefox/3.6

Now, this is really odd.

When you manually set the value of an input element to '' and the user presses ESC while the input is focused, Firefox magically restores the previous value (before it was set to '').

May not seem like a big thing, however, in the current web application I'm coding, it's a showstopper.

Reproducible: Always

Steps to Reproduce:
1. Go to the above URL
2. Click in the input field and type anything
3. The script will automatically clear the input periodically
4. If you press ESC, the previous input magically re-appears
Actual Results:  
INPUTs randomly get previous values inserted when pressing ESC

Expected Results:  
Pressing ESC should not mess with an input's value.

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

It seems that this is an editor's bug but I'm not sure.

Comment 2

[Boris Zbarsky [:bzbarsky]]

No, this is not editor.  The issue is that when you hit escape the form autocomplete code treats that as "cancel autocomplete" and then calls nsAutoCompleteController::RevertTextValue which does:

  input->SetTextValue(oldValue);

Clearly it has the wrong oldValue at that point!

Oh, and this happens even when autocomplete is disabled (to say nothing of cases when it's enabled but not active).  I tested both autocomplete="off" on the textfield and disabling it wholesale in preferences.

Why is this code running at all if autocomplete is disabled?

Comment 3

[Mounir Lamouri (:mounir)]

Attachment: testcase

Comment 4

[Mounir Lamouri (:mounir)]

Last good nightly: 2011-01-21 First bad nightly: 2011-01-22

Pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=8d52e3b68ca6&tochange=7b396ca54953

And I would say, the most likely candidate is bug 320462 given that we were not sending input events an autocomplete actions before.

Comment 5

[Mounir Lamouri (:mounir)]

Oups, wrong bug :(

Comment 6

[Paul LeBeau]

Struck this one myself today.

A test case can be found here: http://jsfiddle.net/RX2Uc/5/

Comment 9

[(no longer active)]

Dolske, zpao: who's the best person to look into this?  Comment 2 provides a pretty good summary of what's going wrong here...

Comment 10

[Mounir Lamouri (:mounir)]

Attachment: testcase

The jsfiddle test case.

Comment 11

[Mounir Lamouri (:mounir)]

So, nsFormFillController is listening to various events on the input element but a script is changing the value of the element so no event is thrown and the nsFormFillController doesn't hear about it. Thus, nsAutoCompleteController doesn't hear about it too. As a consequence, nsAutoCompleteController believes the value is still the one that was typed and ESC put it back because the old value isn't the same as the current value.

Unfortunately, there is no event that we could use (and that I'm aware of) to fix this.
I think the best way to fix this would be to change how nsAutoCompleteController behaves. For example, instead of reverting to the search string, we could keep a track of the previous value while the user navigates trough the autocomplete list.

Comment 12

[Scott González]

Mounir, do you know why the autocomplete code runs even when autocomplete is disabled?

Comment 13

[Mounir Lamouri (:mounir)]

(In reply to Scott González from comment #12)
> Mounir, do you know why the autocomplete code runs even when autocomplete is
> disabled?

In nsFormController::Focus, there is a check for autocomplete value but not being used. As a result, if you focus an text field with autofocus='off', nsFormController considers it should be watched as any other text field. I think it has been done to make autocomplete popup from <datalist>/@list works.
I have a patch fixing that. The downside is it reacts badly at dynamic change while typing. This said, the current implementation handle 'off'->'on' correctly but not 'on'->'off'. We probably don't care that much...

Comment 14

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #13)
> (In reply to Scott González from comment #12)
> > Mounir, do you know why the autocomplete code runs even when autocomplete is
> > disabled?
> 
> In nsFormController::Focus, there is a check for autocomplete value but not
> being used. As a result, if you focus an text field with autofocus='off',
> nsFormController considers it should be watched as any other text field. I
> think it has been done to make autocomplete popup from <datalist>/@list
> works.

AFAIK, autocomplete="off" only indicates that we won't save new values. I think we'll still complete results you have that match the field if they were previously saved.

Comment 15

[Scott González]

autocomplete="off" completely disables the UI; no results will be shown even if there are saved values. JS-based autocomplete implementations rely on this behavior.

Comment 16

[Mounir Lamouri (:mounir)]

(In reply to Paul O'Shannessy [:zpao] from comment #14)
> AFAIK, autocomplete="off" only indicates that we won't save new values. I
> think we'll still complete results you have that match the field if they
> were previously saved.

This is neither our behavior nor what the specifications require [1].
We do not save a value if there is autocomplete="off" and we do not show a list of suggestions in that case too.

[1] http://www.whatwg.org/specs/web-apps/current-work/multipage/common-input-element-attributes.html#the-autocomplete-attribute

Comment 17

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

My mistake. I was thinking about username/password fields. For those we won't autofill the fields even if they are the only match. We will attach autocomplete results if we have them, however we will not offer to save new entries. For regular form fields we seem to treat autocomplete=off more "correctly".

Comment 18

[Anton Averin]

Any progress on this issue?
Still reproduces 100% in Firefox 10.0.2.

Comment 19

[Mounir Lamouri (:mounir)]

Boris, Ehsan, Paul, Dolske, does anyone of you know when nsAutoCompleteController::RevertTextValue() is used? For the moment I don't see any real use of that method. When is that needed?

Comment 20

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

It's called by nsAutoCompleteController::HandleEscape().
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/autocomplete/nsAutoCompleteController.cpp#310

HandleEscape() is called when ESC keypress event is handled by nsFormFillController.
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/satchel/nsFormFillController.cpp#909

Comment 21

[Mounir Lamouri (:mounir)]

Yes, sure.
But in a regular <input>, there is no way that pressing ESC will change the current input value except in that case (which is a bug) so I wonder if there is any use case I might be missing where we want ESC to change the value.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Do we change the value shown if a user uses the down arrow to navigate into the autofill dropdown?  If we do, then I'd think ESC is supposed to revert that...

Comment 23

[Mounir Lamouri (:mounir)]

(In reply to Boris Zbarsky (:bz) from comment #22)
> Do we change the value shown if a user uses the down arrow to navigate into
> the autofill dropdown?  If we do, then I'd think ESC is supposed to revert
> that...

That was my first guess so I tried that but, at least with my testcase, navigating wasn't changing the value shown in the textfield. My only guess now is:
- we use that for something very specific (like the awesomebar or I don't know)
- we used to use that but don't use it anymore

Comment 24

[Mounir Lamouri (:mounir)]

... or we used to show the value from the autocomplete list in the textfield and we broke that someday and no one noticed ;)

Comment 25

[Scott González]

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #24)
> ... or we used to show the value from the autocomplete list in the textfield
> and we broke that someday and no one noticed ;)

Heh, that actually seems plausible, but I just tested in 3.6 and 2.0 and neither of them have that behavior. So it's either been broken for a really long time or it never worked that way.

FWIW, I would expect the behavior for autocomplete text fields to match the behavior of the awesomebar.

Comment 26

[Mounir Lamouri (:mounir)]

I pushed to try a version without calling RevertTextValue() and I got those test failures:
13886 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/toolkit/content/tests/chrome/test_autocomplete5.xul | Test timed out.
13889 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/toolkit/content/tests/chrome/test_autocomplete_delayOnPaste.xul | an unexpected uncaught JS exception reported through window.onerror - uncaught exception: [Exception... "Component returned failure code: 0xc1f30100 (NS_ERROR_FACTORY_EXISTS) [nsIComponentRegistrar.registerFactory]" nsresult: "0xc1f30100 (NS_ERROR_FACTORY_EXISTS)" location: "JS frame :: chrome://mochitests/content/chrome/toolkit/content/tests/chrome/test_autocomplete_delayOnPaste.xul :: <TOP_LEVEL> :: line 64" data: no] at :0
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_urlbarRevert.js | ESC reverted the location bar value - Got foobar, expected example.com

Full log:
https://tbpl.mozilla.org/php/getParsedLog.php?id=10165616&tree=Try&full=1

Try push:
https://tbpl.mozilla.org/?tree=Try&rev=58a92bf1177c

I will use that to investigate later why we need this.

Comment 27

[Chris Oakman]

I'm wondering if there is any progress on this bug?

I ran across it while building a JS AutoComplete component.

Here's another test case:
1. http://autocompletejs.com/examples/1000
2. add focus to the input element
3. type "app", press enter, the input field is empty
4. press "escape", the input field contains "app"

Comment 28

[Bodo Graumann]

I have come across this issue too. When using
> Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Firefox/45.0
it does occur for me. (As well as https://bugzilla.mozilla.org/show_bug.cgi?id=646811).

On the other hand when using
> Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Firefox/52.0
I can not reproduce the issue.


So I think someone with a recent version of Firefox on Linux should test it and then this bug and #646811 can probably be closed.

Comment 29

[Niki Romagnoli (TechNyquist)]

Attachment: Bug reproduction and workaround

I noticed that a focus trick prevents this to happen.
So this leads to a nasty workaround: after the input has been set to the empty string, move the focus to another focusable element and then back to input, this will prevent the bug to happen.
I've attached a minimal HTML file that demonstrates the bug and shows the workaround running.

I repeat myself: it's a workaround, not a fix. Could be of help until a real official fix will be provided.

Comment 30

[Matthew N. [:MattN]]

(In reply to Bodo Graumann from comment #28)
> I have come across this issue too. When using
> > Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Firefox/45.0
> it does occur for me. (As well as
> https://bugzilla.mozilla.org/show_bug.cgi?id=646811).
> 
> On the other hand when using
> > Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Firefox/52.0
> I can not reproduce the issue.

That sounds like it may be caused by multi-process support. Can you paste what you see for "Multiprocess Windows" in about:support of 52 when you can't reproduce the issue?

Comment 31

[Bodo Graumann]

Ok, I did some more tests.
First of all let me note that there are a lot of different example pages in this and bug #646811, so I boiled everything down to the following:

> <input id="input1" value="DefaultText">
> 
> <script>
> document.addEventListener('DOMContentLoaded', function () {
>     var element = document.getElementById('input1');
> 
>     element.addEventListener('keydown' , function (event) {
>         if (event.key == 'Escape' || event.keyCode == 27 || event.which == 27) {
>             element.value = 'SetValue';
>         }
>     });
> });
> </script>

What I found is the following:
- This example does trigger the bug (i.e. the new value is not set) in Firefox 45 as well as 52.
- There are multiple workarounds:
  1. Explicitely disabling autocomplete on the input element:
> <input id="input1" value="DefaultText" autocomplete="off">

  2. Disabling the default action in the event handler:
> event.preventDefault();

  3. Removing focus from the element in the event handler:
> element.blur();

- Many of the attached / linked examples already have one of those workarounds included, that is why I thought the bug did not occur in Firefox 52.
- On the other hand I thought it did occur even with the workarounds in Firefox 45, because I used the wrong profile, where the vimperator addon prevented the keydown event from firing at all when the escape key is pressed.

Comment 32

[Niki Romagnoli (TechNyquist)]

Also note that a keystroke is needed to show the bug, a hand-made event doesn't trigger anything.
To test it I did the following:

> var filter=false;
> 
> function eventCallback(ev){
>     if(ev.keyCode == 27) {
>         ...
>         if(!filter)
>         {
>             filter=true;
>             setTimeout(function(){ eventCallback(ev); filter=false; }, 5000);
>         }
>     }
> }

and even in blur status the timeout is able to clear the field (as expected) but not to show the text back again, thus no bug.

Comment 33

[Sergey Galich [:serg]]

Can not reproduce on Firefox 103 and Bug 1407085 has a patch that seems to fix exactly the issue described.