651956 - Esc key should not fire an input event

Status: RESOLVED FIXED

(Toolkit :: Form Manager, defect)

Description

[Marvin Tam]

User-Agent:       Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0.1) Gecko/20100101 Firefox/4.0.1
Build Identifier: "Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0.1) Gecko/20100101 Firefox/4.0.1"

My use case is a simple <input type="text"> element, listening to the onInput. When the input field is in focus and when I hit esc, onInput should NOT fire. 

According to whatwg [1], onInput should only fire when the element value changes. The esc key obviously wouldn't change element.value.

This works fine in Firefox 3.0 and 3.6, and only started breaking in Firefox 4.0. This also works correctly in Chrome.


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

Reproducible: Always

Steps to Reproduce:
1. Attach onInput to <input type=text>
2. Do something on the event handler function, say console.log('omg')
3. Focus on the element
4. Hit the escape key
Actual Results:  
onInput is fired

Expected Results:  
onInput doesn't fire

Comment 1

[Marvin Tam]

Attachment: Simple test case

Comment 2

[Boris Zbarsky [:bzbarsky]]

> The esc key obviously wouldn't change element.value.

It can in some cases, as I recall (when autocomplete is involved).  But yes, in your testcase it certainly isn't.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Would be nice to figure out when this behavior changed.

Comment 4

[Mounir Lamouri (:mounir)]

I thought oninput was sent by the layout...

Comment 5

[Boris Zbarsky [:bzbarsky]]

FireOnInput is called from nsTextInputListener::EditAction.

Comment 6

[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.

I guess we should make sure that SetUserInput isn't called when the user didn't change the value.

Comment 7

[Mounir Lamouri (:mounir)]

The fix doesn't seem hard. The only question is whether we should not fire the input event (and not change the value) if newValue == oldValue in SetUserInput or if we should make sure that this method isn't called when newValue == oldValue.

Comment 8

[Boris Zbarsky [:bzbarsky]]

If I have the text "bar" in a text input, highlight the "a" and hit the "a" key, should that fire oninput?  Does that operation make only one SetUserInput call?

Comment 9

[(no longer active)]

(In reply to comment #8)
> If I have the text "bar" in a text input, highlight the "a" and hit the "a"
> key, should that fire oninput?

I think so, yes.

> Does that operation make only one SetUserInput call?

That's what I would expect.

Comment 10

[Marvin Tam]

(In reply to comment #8)
> If I have the text "bar" in a text input, highlight the "a" and hit the "a"
> key, should that fire oninput?  Does that operation make only one SetUserInput
> call?

I'd say no, oninput should not fire. The specs are pretty explicit about triggering the event only when the user changes the value (I emphasized the relevant part):

"Examples of a user changing the element's value would include the user typing into a text field, pasting a new value into the field, or undoing an edit in that field. Some user interactions do not cause changes to the value, e.g. hitting the "delete" key in an empty text field, or ***replacing some text in the field with text from the clipboard that happens to be exactly the same text***."

Comment 11

[(no longer active)]

(In reply to comment #10)
> (In reply to comment #8)
> > If I have the text "bar" in a text input, highlight the "a" and hit the "a"
> > key, should that fire oninput?  Does that operation make only one SetUserInput
> > call?
> 
> I'd say no, oninput should not fire. The specs are pretty explicit about
> triggering the event only when the user changes the value (I emphasized the
> relevant part):
> 
> "Examples of a user changing the element's value would include the user typing
> into a text field, pasting a new value into the field, or undoing an edit in
> that field. Some user interactions do not cause changes to the value, e.g.
> hitting the "delete" key in an empty text field, or ***replacing some text in
> the field with text from the clipboard that happens to be exactly the same
> text***."

Hmm, so this means that they should be handled more or less like mutation events?  :(

Comment 12

[Mounir Lamouri (:mounir)]

All UA's do send an input event when the current value is pasted to an input element. So I do not think we should worry about that. We should probably fix that bug and see what to do with the copy/paste issue.

Comment 13

[Mounir Lamouri (:mounir)]

Attachment: Patch v1

Comment 14

[Mounir Lamouri (:mounir)]

I don't really like this fix because it's special casing for a very specific reason. Though, I do not see a way to prevent that except adding a similar code in "nsHTMLInputElement::SetUserInput" and that would have a behavior with might not want.

Dolske, when |RevertTextValue| can be called with |inputValue != oldValue|?

Comment 15

[Boris Zbarsky [:bzbarsky]]

I don't understand that "NOTE" comment...

Comment 16

[Mounir Lamouri (:mounir)]

(In reply to comment #15)
> I don't understand that "NOTE" comment...

I wonder if that's possible to have RevertTextValue called with inputValue != oldValue. IOW, I wonder if that can actually happen with our current code or if that's some remaining of old use cases.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Oh, because autocomplete may not actually be changing the value anymore?

Comment 18

[Mounir Lamouri (:mounir)]

(In reply to comment #17)
> Oh, because autocomplete may not actually be changing the value anymore?

Yes, I've never been able to trigger a situation where the value in the text field was changed and pressing ESC was reverting it.

Comment 19

[Justin Dolske [:Dolske]]

(In reply to comment #14)

> Dolske, when |RevertTextValue| can be called with |inputValue != oldValue|?

I don't know, you know this code as well as anyone else. :/

But, from a quick scan of call paths, I don't see any code that would explicitly prevent it. (Will bug 566489 let this happen?)

Also, is there a reason this function would still notify observers and call input->OnTextReverted() when the values are the same? Maybe just make it an early return when old==new?

Comment 20

[Mounir Lamouri (:mounir)]

(In reply to comment #19)
> (In reply to comment #14)
> 
> > Dolske, when |RevertTextValue| can be called with |inputValue != oldValue|?
> 
> I don't know, you know this code as well as anyone else. :/
> 
> But, from a quick scan of call paths, I don't see any code that would
> explicitly prevent it. (Will bug 566489 let this happen?)

I didn't see anything that could prevent it but I also see nothing that could enable it. Though, indeed, bug 566489 might enable this.

> Also, is there a reason this function would still notify observers and call
> input->OnTextReverted() when the values are the same? Maybe just make it an
> early return when old==new?

Actually, currently, we notify observers even if we seem to set the exact same value. So, to prevent any regressions, I prefer to keep doing that. I can do an early return if you think it's safe.

Comment 21

[Mounir Lamouri (:mounir)]

Dolske, any update regarding the review?

Comment 22

[Justin Dolske [:Dolske]]

Comment on attachment 535015 [details] [diff] [review]
Patch v1

Review of attachment 535015 [details] [diff] [review]:
-----------------------------------------------------------------

I realized I've been putting off this patch not because it's tricky, but just because I dread touching the autocomplete stuff. Sigh.

But look innocuous enough, let's try it!

Comment 23

[Mounir Lamouri (:mounir)]

Pushed:
https://hg.mozilla.org/mozilla-central/rev/9a7ca7ecaa1e

Thanks for reporting this bug Marvin. It will be fixed in Firefox 10.

Comment 24

[Eric Shepherd [:sheppy]]

Mentioned on Firefox 10 for developers.