Closed
Bug 357684
Opened 18 years ago
Closed 18 years ago
onchange on textbox not fired when onblur changes text value
Categories
(Core :: Layout: Form Controls, defect)
Core
Layout: Form Controls
Tracking
()
VERIFIED
FIXED
People
(Reporter: fedebona, Assigned: MatsPalmgren_bugz)
References
Details
(Keywords: regression, testcase)
Attachments
(4 files)
702 bytes,
text/html
|
Details | |
2.75 KB,
text/html
|
Details | |
2.75 KB,
patch
|
smaug
:
review+
bzbarsky
:
superreview+
|
Details | Diff | Splinter Review |
3.13 KB,
patch
|
bzbarsky
:
review+
bzbarsky
:
superreview+
|
Details | Diff | Splinter Review |
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
Reporter | ||
Comment 1•18 years ago
|
||
Comment 2•18 years ago
|
||
Dup of bug 357021 or bug 355367?
Reporter | ||
Comment 3•18 years ago
|
||
(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•18 years ago
|
||
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•18 years ago
|
||
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?
Flags: blocking1.9?
Keywords: regression
Comment 6•18 years ago
|
||
(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•18 years ago
|
||
> 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•18 years ago
|
||
(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•18 years ago
|
||
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?
Assignee | ||
Comment 10•18 years ago
|
||
(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•18 years ago
|
||
> That can't happen AFAICT, the order of events should be: See the IE and Opera order in comment 4.
Assignee | ||
Comment 12•18 years ago
|
||
(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...
Assignee: nobody → mats.palmgren
OS: Windows XP → All
Hardware: PC → All
Assignee | ||
Comment 13•18 years ago
|
||
Assignee | ||
Comment 14•18 years ago
|
||
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.
Assignee | ||
Comment 15•18 years ago
|
||
Move the CheckFireOnChange() call from "Blur" to "pre-Blur".
Attachment #243876 -
Flags: superreview?(bzbarsky)
Attachment #243876 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 16•18 years ago
|
||
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•18 years ago
|
||
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...
Attachment #243876 -
Flags: superreview?(bzbarsky)
Attachment #243876 -
Flags: superreview+
Attachment #243876 -
Flags: review?(bzbarsky)
Attachment #243876 -
Flags: review?(Olli.Pettay)
Updated•18 years ago
|
Flags: in-testsuite?
Comment 18•18 years ago
|
||
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.
Attachment #243876 -
Flags: review?(Olli.Pettay) → review+
Comment 19•18 years ago
|
||
We want to take this ASAP, but we will not block 1.8.1.1 for it at this point.
Flags: wanted1.8.1.x+
Flags: blocking1.8.1.1?
Flags: blocking1.8.1.1-
Comment 20•18 years ago
|
||
Mats, the patch has r+ and sr+, can it be checked in?
Assignee | ||
Comment 21•18 years ago
|
||
Checked in to trunk at 2006-11-28 05:08 PST -> FIXED
Status: NEW → RESOLVED
Closed: 18 years ago
Flags: blocking1.9?
Resolution: --- → FIXED
Assignee | ||
Comment 22•18 years ago
|
||
Attachment #246784 -
Flags: superreview?(bzbarsky)
Attachment #246784 -
Flags: review?(bzbarsky)
Comment 23•18 years ago
|
||
Based on your expertice and experience: What is the expected time for this fix to be included in an automated FF2 update?
Comment 24•18 years ago
|
||
Comment on attachment 246784 [details] [diff] [review] Patch for 1.8 branch Makes sense
Attachment #246784 -
Flags: superreview?(bzbarsky)
Attachment #246784 -
Flags: superreview+
Attachment #246784 -
Flags: review?(bzbarsky)
Attachment #246784 -
Flags: review+
Comment 25•18 years ago
|
||
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.
Assignee | ||
Updated•18 years ago
|
Attachment #246784 -
Flags: approval1.8.1.2?
Assignee | ||
Comment 26•18 years ago
|
||
Comment on attachment 246784 [details] [diff] [review] Patch for 1.8 branch Maybe it's not too late for 1.8.1.1?
Attachment #246784 -
Flags: approval1.8.1.2? → approval1.8.1.1?
Comment 27•18 years ago
|
||
It really is too late, but I'll leave nominated as a potential ride-along if we have to re-spin.
Whiteboard: ride-along candidate
Updated•18 years ago
|
Attachment #246784 -
Flags: approval1.8.1.1? → approval1.8.1.2?
Updated•18 years ago
|
Flags: blocking1.8.1.2+
Comment 28•18 years ago
|
||
Comment on attachment 246784 [details] [diff] [review] Patch for 1.8 branch Approved for 1.8 branch, a=jay for drivers.
Attachment #246784 -
Flags: approval1.8.1.2? → approval1.8.1.2+
Assignee | ||
Comment 29•18 years ago
|
||
Checked in to MOZILLA_1_8_BRANCH at 2006-12-27 19:23 PST
Keywords: fixed1.8.1.2
Whiteboard: ride-along candidate
Comment 30•18 years ago
|
||
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
Status: RESOLVED → VERIFIED
Keywords: fixed1.8.1.2 → verified1.8.1.2
Comment 31•17 years ago
|
||
backed out from branch because of the regression. Will be fixed properly on 1.8.1.3, I hope.
Comment 32•17 years ago
|
||
removing fixed1.8.1.2 keyword and nominating for 1.8.1.3 to keep this on our radar for next time.
Flags: blocking1.8.1.3?
Keywords: verified1.8.1.2
Comment 33•17 years ago
|
||
caused regressions, backed out of 1.8.1.2
Flags: blocking1.8.1.2+ → blocking1.8.1.2-
Comment 34•17 years ago
|
||
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.
Updated•17 years ago
|
Attachment #246784 -
Flags: approval1.8.1.2+
Comment 35•17 years ago
|
||
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.
Flags: blocking1.8.1.4? → blocking1.8.1.4-
You need to log in
before you can comment on or make changes to this bug.
Description
•