Closed Bug 651956 Opened 9 years ago Closed 8 years ago

Esc key should not fire an input event

Categories

(Toolkit :: Form Manager, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla10

People

(Reporter: marvintam, Assigned: mounir)

References

(Depends on 1 open bug)

Details

(Keywords: dev-doc-complete, regression, testcase)

Attachments

(2 files)

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
Attached file Simple test case
> 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.
Status: UNCONFIRMED → NEW
Component: DOM: Events → Editor
Ever confirmed: true
QA Contact: events → editor
Would be nice to figure out when this behavior changed.
I thought oninput was sent by the layout...
OS: Mac OS X → All
Hardware: x86 → All
Version: unspecified → Trunk
FireOnInput is called from nsTextInputListener::EditAction.
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.
Blocks: 320462
Component: Editor → Form Manager
Product: Core → Toolkit
QA Contact: editor → form.manager
Keywords: testcase
Assignee: nobody → mounir.lamouri
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.
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?
(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.
(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***."
(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?  :(
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.
Attached patch Patch v1Splinter Review
Attachment #535015 - Flags: review?(bzbarsky)
Attachment #535015 - Flags: review?(dolske)
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|?
Status: NEW → ASSIGNED
Whiteboard: [needs review]
I don't understand that "NOTE" comment...
(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.
Oh, because autocomplete may not actually be changing the value anymore?
(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.
Attachment #535015 - Flags: review?(bzbarsky) → review+
(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?
(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.
Dolske, any update regarding the review?
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!
Attachment #535015 - Flags: review?(dolske) → review+
Flags: in-testsuite+
Whiteboard: [needs review] → [inbound]
Pushed:
https://hg.mozilla.org/mozilla-central/rev/9a7ca7ecaa1e

Thanks for reporting this bug Marvin. It will be fixed in Firefox 10.
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Summary: Esc key should not fire onInput → Esc key should not fire an input event
Whiteboard: [inbound]
Target Milestone: --- → mozilla10
Mentioned on Firefox 10 for developers.
Depends on: 1292520
You need to log in before you can comment on or make changes to this bug.