357684 - onchange on textbox not fired when onblur changes text value

Status: VERIFIED FIXED

(Core :: Layout: Form Controls, defect)

Description

[Federico Bonarrigo]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1) Gecko/20061010 Firefox/2.0
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1) Gecko/20061010 Firefox/2.0

If you put some code in the onblur handler of a textbox that changes the content of the textbox itself you won't get the onchange event.
A typical example is a situation where you want to pretty print a monetary amount (with thousands separator or currency prefix). In this situation you add currency symbol in onblur (and remove it in onfocus)

Reproducible: Always

Steps to Reproduce:
1.you start with a textbox with "$1" as value
2.you focus over the box. Then value changes to "1"
3.you put "2" and click submit (or in general loose focus). Then you get "$2".



Actual Results:  
You see in the bottom of the page the events fired. You see only "focus" and "blur", and never "change".

Expected Results:  
It's expected to see that if you change your monetary amount from "1" to "2" you will get an onchange event.


It doesn't happen with firefox1.5

Comment 1

[Federico Bonarrigo]

Attachment: TestCase for the bug

Comment 2

[Jesse Ruderman]

Dup of bug 357021 or bug 355367?

Comment 3

[Federico Bonarrigo]

(In reply to comment #2)
> Dup of bug 357021 or bug 355367?
> 
I surely think they can be related. The refer to keydown or keyup events, I refer to onblur: the common thing is that our events change the text and this brings to a situation where onchange isn't fired. Bug 355367 however is slightly different in behaviour: if you leave the focus via Tab key you have the bug (onchange not fired), but if you leave focus clicking with mouse outside the textbox onchange is fired (ver 2.0 RC2)

Could the problem be related to the fact that the order of event firing is onfocus-onblur-onchange? (I think the correct way to do it is onfocus-onchange-onblur, like other browsers do)

Comment 4

[Martijn Wargers (dead)]

This behavior changed between 2006-07-05 and 2006-07-06:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2006-07-05+05&maxdate=2006-07-06+06&cvsroot=%2Fcvsroot
I bet this is a regression from bug 313337.

With Firefox builds, before this regressed, I get:
focus
blur
change

With IE7 and Opera9, I get this:
focus
change
blur

So I think we at least want the old behavior back, but maybe it is preferable even to be compatible with IE7/Opera9.

Comment 5

[Boris Zbarsky [:bzbarsky]]

So the basic problem is that the blur handler fires way earlier than onchange... and that the blur handler fires before the textframe's mHasFocus becomes false.

Our of curiousity, does doing preventDefault() in the blur handler in opera/IE prevent the focus from leaving the textfield?

Comment 6

[Martijn Wargers (dead)]

(In reply to comment #5)
> Our of curiousity, does doing preventDefault() in the blur handler in opera/IE
> prevent the focus from leaving the textfield?

No, not in Opera. preventDefault isn't working in IE (not sure anymore what to use instead), but from the documentation:
http://msdn.microsoft.com/workshop/author/dhtml/reference/events/onblur.asp
it says it isn't cancelable.

Comment 7

[Boris Zbarsky [:bzbarsky]]

> not sure anymore what to use instead

  onblur="return false;"

Out of curiousity, in the other browsers if it's the onfocus handler of whatever gets focused that changes the value, does onchange fire?

Comment 8

[Martijn Wargers (dead)]

(In reply to comment #7)
> > not sure anymore what to use instead
> 
>   onblur="return false;"

Yes, that doesn't work, so I think it's not possible to cancel the blur event.

> Out of curiousity, in the other browsers if it's the onfocus handler of
> whatever gets focused that changes the value, does onchange fire?

Well, the onchange event only fires when the user has changed something, it isn't invoked by scripts.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Yes, what I meant was, if the user changes the value, then clicks outside the box, the onfocus handler of the <body> or whatever fires before the onchange of the <input>.  If the onfocus handler changes the value again (either to whatever it was before the user edits, or to something else), does the onchange fire?

Comment 10

[Mats Palmgren (inactive)]

(In reply to comment #9)
> Yes, what I meant was, if the user changes the value, then clicks outside the
> box, the onfocus handler of the <body> or whatever fires before the onchange of
> the <input>.

That can't happen AFAICT, the order of events should be:
INPUT:change
INPUT:blur
BODY(or whatever):focus

Comment 11

[Boris Zbarsky [:bzbarsky]]

> That can't happen AFAICT, the order of events should be:

See the IE and Opera order in comment 4.

Comment 12

[Mats Palmgren (inactive)]

(In reply to comment #11)
> > That can't happen AFAICT, the order of events should be:
> 
> See the IE and Opera order in comment 4.

I'm pretty sure the first 'focus' there is from focusing the text input,
not from focusing something else when clicking outside it.
Anyway, I did a few more elaborate tests...

Comment 13

[Mats Palmgren (inactive)]

Attachment: Testcase #2

Comment 14

[Mats Palmgren (inactive)]

1. Load Testcase #2
2. left-click on the text input
3. type some text
4. TAB

IE7, WinXPSP2
  48:303 : focus 
  49:775 : blur 
  49:775 INPUT(ctltxt): focus 
  51:958 INPUT(ctltxt): change 
  51:958 INPUT(ctltxt): blur 
  51:968 INPUT(submit): focus 

Opera9, WinXPSP2
  06:515 INPUT(ctltxt): focus 
  09:319 INPUT(ctltxt): change 
  09:319 BODY(BODY): change (target=ctltxt)
  09:329 INPUT(ctltxt): blur 
  09:329 INPUT(submit): focus 

Safari 2.0.4, MacOSX 10.4.8
  19:654 INPUT(ctltxt): focus
  22:502 INPUT(ctltxt): change
  22:503 INPUT(ctltxt): blur
  22:503 INPUT(submit): focus

Firefox2, WinXPSP2
  58:740 (window): focus (target=#document)
  58:740 (window): focus (target=#document)
  00:002 INPUT(ctltxt): focus
  01:805 INPUT(ctltxt): blur
  01:815 INPUT(submit): focus
  01:815 (window): focus (target=submit)

Firefox trunk, Linux, with patch rev. 1
  47:476 (window): focus (target=#document)
  47:504 (window): focus
  48:867 INPUT(ctltxt): focus
  57:298 INPUT(ctltxt): change
  57:303 (window): change (target=ctltxt)
  57:316 INPUT(ctltxt): blur
  57:334 INPUT(submit): focus
  57:338 (window): focus (target=submit)

The first event that we're interested in is "INPUT(ctltxt): focus" (step 2),
the events before that are window/document focus events from loading
the testcase (could be interesting for some other bug perhaps).

IE, Opera, Safari are pretty compatible, the only difference is that
IE and Safari does not bubble the onchange.

I also tried "preventDefault()/return false" in onblur and/or onchange.
Only IE was affected by that, the other UAs had exactly the same events as
above. In IE, returning false from onchange cancels the blur *and* focus,
i.e. the text input element is still focused after step 4.
Returning false from onblur (only) did not affect the events in IE either.

I also tested having a changed value and calling submit(): none of the UAs
paid any attention to "return false" from onchange in this case.

Comment 15

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

Move the CheckFireOnChange() call from "Blur" to "pre-Blur".

Comment 16

[Mats Palmgren (inactive)]

For comparison:

Firefox 1.5.0.7, Linux
  38:835 (window): focus (target=#document)
  38:916 (window): focus (target=#document)
  41:404 INPUT(ctltxt): focus
  48:099 INPUT(ctltxt): blur
  48:107 INPUT(ctltxt): change
  48:110 (window): change (target=ctltxt)
  48:114 INPUT(submit): focus
  48:116 (window): focus (target=submit)

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 243876 [details] [diff] [review]
Patch rev. 1

This looks good to me, but I'd like smaug to check it out.  The only worry I have is whether running random script at this point of PreHandleEvent is OK.  If it is, then this is great.

We'll need a different patch for 1.8.1.1, of course, but this should not be too hard to port over...

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 243876 [details] [diff] [review]
Patch rev. 1

Should be ok.
See also http://lxr.mozilla.org/seamonkey/source/content/html/content/src/nsHTMLSelectElement.cpp#1823

What I don't like too much is that different implementations of nsIFormControlFrame::SetFocus do different things. Some dispatch 'change' some don't.
Though, that is probably something which can't be avoided easily.

Comment 19

[Mike Connor [:mconnor]]

We want to take this ASAP, but we will not block 1.8.1.1 for it at this point.

Comment 20

[Martijn Wargers (dead)]

Mats, the patch has r+ and sr+, can it be checked in?

Comment 21

[Mats Palmgren (inactive)]

Checked in to trunk at 2006-11-28 05:08 PST

-> FIXED

Comment 22

[Mats Palmgren (inactive)]

Attachment: Patch for 1.8 branch

Comment 23

[Ronny Hanssen]

Based on your expertice and experience: 

What is the expected time for this fix to be included in an automated FF2 update?

Comment 24

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 246784 [details] [diff] [review]
Patch for 1.8 branch

Makes sense

Comment 25

[Boris Zbarsky [:bzbarsky]]

Ronny, once the patch that I just reviewed is approved it will get checked in.  At that point it'll go out with the next update.

When that will happen will depend on where the current update is in terms of taking additional patches.

Comment 26

[Mats Palmgren (inactive)]

Comment on attachment 246784 [details] [diff] [review]
Patch for 1.8 branch

Maybe it's not too late for 1.8.1.1?

Comment 27

[Daniel Veditz [:dveditz]]

It really is too late, but I'll leave nominated as a potential ride-along if we have to re-spin.

Comment 28

[Jay Patel [:jay]]

Comment on attachment 246784 [details] [diff] [review]
Patch for 1.8 branch

Approved for 1.8 branch, a=jay for drivers.

Comment 29

[Mats Palmgren (inactive)]

Checked in to MOZILLA_1_8_BRANCH at 2006-12-27 19:23 PST

Comment 30

[Carsten Book [:Tomcat]]

Verified fixed for 1.8.1.2 with Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.8.1.2pre) Gecko/20070128 BonEcho/2.0.0.2pre ID:2007012803 and also on Linux Fedora FC 6

Comment 31

[Olli Pettay [:smaug][[e-mail]]]

backed out from branch because of the regression.
Will be fixed properly on 1.8.1.3, I hope.

Comment 32

[Jay Patel [:jay]]

removing fixed1.8.1.2 keyword and nominating for 1.8.1.3 to keep this on our radar for next time.

Comment 33

[Daniel Veditz [:dveditz]]

caused regressions, backed out of 1.8.1.2

Comment 34

[Jay Patel [:jay]]

Confirming that the backout of this patch reverted Firefox 2.0.0.2 rc5 to the original broken behavior.  

If/when we decide to fix this on the branches again, we need to make sure to address the regression in bug 370521 and land that patch as well.

Comment 35

[Daniel Veditz [:dveditz]]

Haven't seen any activity for nearly a month and still leary of regressions like the one that happened last time. Not going to hold the release for this.

Feel free to prove me wrong with a regression-free patch and ask for approval on it.