Change event is not fired when the input element's value is modified while the element is focused

RESOLVED FIXED in Firefox 16

Status

()

Core
DOM: Core & HTML
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Mike Walsh, Assigned: mounir)

Tracking

({regression, testcase})

15 Branch
mozilla18
regression, testcase
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox15- wontfix, firefox16+ fixed, firefox17+ fixed, firefox18+ fixed, firefox-esr10 unaffected)

Details

(Whiteboard: [qa-])

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
Created attachment 656903 [details]
Test Case

User Agent: Mozilla/5.0 (Windows NT 5.1; rv:14.0) Gecko/20100101 Firefox/14.0.1
Build ID: 20120713134347

Steps to reproduce:

Setting the value of a text input element programmatically from within an input event handler results in no change event being fired when that element loses focus.

This behavior started with version 15.0.

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/20100101 Firefox/15.0


Actual results:

No change event was fired after the text input element lost focus.


Expected results:

A change event should have been fired when the element loses focus.

Comment 1

5 years ago
egression window(m-c)
Good:
http://hg.mozilla.org/mozilla-central/rev/333626f688a4
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/15.0 Firefox/15.0a1 ID:20120507160328
Bad:
http://hg.mozilla.org/mozilla-central/rev/fafd873d469c
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/15.0 Firefox/15.0a1 ID:20120507174628
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=333626f688a4&tochange=fafd873d469c

Regression window(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/97bef33bf606
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/15.0 Firefox/15.0a1 ID:20120507095528
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/14aa9de6287f
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/15.0 Firefox/15.0a1 ID:20120507100429
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=97bef33bf606&tochange=14aa9de6287f

Triggered by:
ba79daeeb874	Andrew Quartey — Bug 722599 - Move change event firing to content from layout r=mounir
Blocks: 722599
Status: UNCONFIRMED → NEW
tracking-firefox16: --- → ?
tracking-firefox17: --- → ?
tracking-firefox18: --- → ?
Component: Untriaged → Layout: Form Controls
Ever confirmed: true
Keywords: regression
OS: Windows XP → All
Product: Firefox → Core
(Assignee)

Updated

5 years ago
Attachment #656903 - Attachment mime type: text/plain → text/html
(Assignee)

Updated

5 years ago
Keywords: testcase
(Assignee)

Comment 2

5 years ago
That's because we set |mFocusedValue| everytime input.value is set. We should not do that when the element is focused.

Andrew, I'm assigning to the bug but let me know if you don't want/have time to work on this.
Assignee: nobody → andrew.quartey
(Assignee)

Updated

5 years ago
Duplicate of this bug: 786799
(Assignee)

Updated

5 years ago
Summary: A change event is not fired when an input element's value is modified from within an input event handler. → Change event is not fired when the input element's value is modified while the element is focused

Comment 4

5 years ago
This bug is affecting production software.
Can anybody give me a date when this fix will be released?
Firefox 15.0.1?
I'm hoping for days-weeks instead of months. :-)
(Assignee)

Comment 5

5 years ago
I don't remember why we are re-setting mFocusedValue in ::SetValue() and I can't find any reason.
I've removed that and pushed this to try. Andrew, do you happen to remember?

https://tbpl.mozilla.org/?tree=Try&rev=ff62ea0cf795
(Assignee)

Comment 6

5 years ago
Created attachment 657381 [details] [diff] [review]
Opportunistic patch
Assignee: andrew.quartey → mounir
Status: NEW → ASSIGNED
Attachment #657381 - Flags: review?(andrew.quartey)
(Assignee)

Updated

5 years ago
status-firefox-esr10: --- → unaffected
status-firefox15: --- → wontfix
status-firefox16: --- → affected
status-firefox17: --- → affected
status-firefox18: --- → affected

Comment 7

5 years ago
Mounir,
Why is Firefox 15 "wontfix" ?
Thanks.
(Assignee)

Comment 8

5 years ago
(In reply to Mike Moening from comment #7)
> Mounir,
> Why is Firefox 15 "wontfix" ?
> Thanks.

AFAIK, we only do respins (Firefox 15.0.1) for serious crashers and security issues. Not for that kind of issues. Firefox 16 could probably have that patch though.

Alex, am I correct?

Comment 9

5 years ago
(In reply to Mounir Lamouri (:mounir) from comment #8)
> Alex, am I correct?

That's correct - the only other instances we respin for are web compatibility issues affecting a large portion of users, or critical user experience bugs. I don't believe this qualifies, but we will try to fix this regression for our next release in <6 weeks.
tracking-firefox15: --- → -
tracking-firefox16: ? → +
tracking-firefox17: ? → +
tracking-firefox18: ? → +

Comment 10

5 years ago
This 2 line change broke one of our production web apps pretty badly.
That may not seem serious or critical to you, but it is to us.
It means changes to input boxes don't get saved. Forms are broken.

Please consider putting this in Firefox 15.0.1
2 different users reported this bug in the 1st day after the 15.0.0 release.

Breaking stuff and then not fixing till the next major release gives people more reasons to switch to Chrome.  We love FF, but when something like this is broke it needs to be fixed ASAP. 

It's a real pain to tell our users to disable FF auto-updates and to re-install old FF 14.0.1  That costs real money.  It has to be done by hand...
(In reply to Mike Moening from comment #10)
> This 2 line change broke one of our production web apps pretty badly.
> That may not seem serious or critical to you, but it is to us.
> It means changes to input boxes don't get saved. Forms are broken.

Mike - all software releases introduce regressions, and the criticality of those regressions must of course be weighed before considering a re-spin. Given the criteria mentioned above, and the information provided, this situation sadly doesn't appear to qualify.

Mounir - can you suggest a change that could be made on server side to work around this issue?
(In reply to Mounir Lamouri (:mounir) from comment #5)
> I don't remember why we are re-setting mFocusedValue in ::SetValue() and I
> can't find any reason.
> I've removed that and pushed this to try. Andrew, do you happen to remember?
> 
> https://tbpl.mozilla.org/?tree=Try&rev=ff62ea0cf795

Oh dear. I remember it clearly and should have detailed what i meant by 'correct behavior' in bug 722599 comment 14. The only reason for resetting mFocusedValue in ::SetValue was to prevent the change event being fired per the requirements of this covering test (by being the same when checked) - http://mxr.mozilla.org/mozilla-central/source/content/html/content/test/test_bug388558.html?force=1#38.The same logic applies to a similar change i made for nsHTMLTextAreaElement::SetValue. So that's another hidden problem too. 

Then again with this patch we have different tests expecting different behaviors. Are those tests from test_bug388558.html incorrect? I'm inclined to believe so.
(Assignee)

Comment 13

5 years ago
Created attachment 657508 [details] [diff] [review]
Patch

I think I found the logic behind or previous behavior. This patch should match it.

Andrew, could you review it?
Attachment #657381 - Attachment is obsolete: true
Attachment #657381 - Flags: review?(andrew.quartey)
Attachment #657508 - Flags: review?(andrew.quartey)
(Assignee)

Updated

5 years ago
Component: Layout: Form Controls → DOM: Core & HTML
(Assignee)

Comment 14

5 years ago
Comment on attachment 657508 [details] [diff] [review]
Patch

Asking a review from bz to because I need a DOM peer review.
Attachment #657508 - Flags: review?(bzbarsky)
(Assignee)

Comment 15

5 years ago
(In reply to Alex Keybl [:akeybl] from comment #11)
> Mounir - can you suggest a change that could be made on server side to work
> around this issue?

It really depends on what is done by the page. The change event is mostly a sugar to make developers life easier so it should be doable to work around that bug.
(Assignee)

Comment 16

5 years ago
(In reply to Mike Moening from comment #10)
> This 2 line change broke one of our production web apps pretty badly.
> That may not seem serious or critical to you, but it is to us.
> It means changes to input boxes don't get saved. Forms are broken.
> 
> Please consider putting this in Firefox 15.0.1
> 2 different users reported this bug in the 1st day after the 15.0.0 release.
> 
> Breaking stuff and then not fixing till the next major release gives people
> more reasons to switch to Chrome.  We love FF, but when something like this
> is broke it needs to be fixed ASAP. 
> 
> It's a real pain to tell our users to disable FF auto-updates and to
> re-install old FF 14.0.1  That costs real money.  It has to be done by
> hand...

Mike, we all understand how hard it is for you but you should also understand that we have nearly half billion users and pushing an update isn't a trivial procedure. We have a new release every six weeks so if everything goes as expected, we should be able to get this fixed in the next release which is going to happen in a month.
Also, working around this bug is doable. However, depending on how you use the event, it might be some serious amount of work.

By the way, to prevent those problems to happen, we would recommend you to use Firefox Beta or even Aurora and try regularly your website with those versions. Firefox changes are pushed to the Aurora version then Firefox Beta and finally the regular Firefox version. Which means a bug found in Aurora or Firefox Beta could be fixed before being pushed to Firefox and thus prevent your website to be broken in Firefox,
Using those versions would help you to make sure your website isn't broken when there is an update of Firefox and us to get that kind of useful bug reports before the official release.
(Assignee)

Comment 17

5 years ago
New try push: https://tbpl.mozilla.org/?tree=Try&rev=fc76df4deed7

Tests that were failing with the previous patch are now passing locally but we never now if that patch isn't regressing something else.
Comment on attachment 657508 [details] [diff] [review]
Patch

>+      // If the value has been set to a script

You mean "by script"?

>+      // NOTE: this is currently quite expensive work (too many string

s/many/much/

>+++ b/content/html/content/test/forms/test_change_event.html
>+    is(textInputChange[0], 3, "text input element should not have dispatched change event (3).");

s/should not/should/

r=me.  I'm not sure how to get rid of all the string stuff. :(
Attachment #657508 - Flags: review?(bzbarsky) → review+
Comment on attachment 657508 [details] [diff] [review]
Patch

Clearing review request since Boris already reviewed this. Nice work! I will file a similar follow-up bug to correct the textarea behavior too.
Attachment #657508 - Flags: review?(andrew.quartey)
(Assignee)

Comment 20

5 years ago
Created attachment 658111 [details] [diff] [review]
Part 2 - Same thing for <textarea>
Attachment #658111 - Flags: review?(bzbarsky)
Comment on attachment 658111 [details] [diff] [review]
Part 2 - Same thing for <textarea>

r=me.  For textarea the perf worries me a lot more.  :(
Attachment #658111 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 22

5 years ago
I will try to work on making this better. Fixing the bug seems a higher priority I believe.
(Assignee)

Comment 23

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/1181826ded9c
https://hg.mozilla.org/integration/mozilla-inbound/rev/b0203e3c174c
status-firefox18: affected → fixed
Flags: in-testsuite+
Hardware: x86 → All
Target Milestone: --- → mozilla18
(Assignee)

Comment 24

5 years ago
Comment on attachment 657508 [details] [diff] [review]
Patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 722599
User impact if declined: Website are broken because of this bug.
Risk to taking this patch (and alternatives if risky): risks on having this patch are small: tests have been written regarding the behaviour of the change event when .value is used. However, introducing other regressions exists even if that seem pretty low risk.
String or UUID changes made by this patch: none

IMO, the regression seems important enough to make this patch go to Beta and Aurora.
Attachment #657508 - Flags: checkin+
Attachment #657508 - Flags: approval-mozilla-beta?
Attachment #657508 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 25

5 years ago
Comment on attachment 658111 [details] [diff] [review]
Part 2 - Same thing for <textarea>

[Approval Request Comment]
See previous comment.
Attachment #658111 - Flags: checkin+
Attachment #658111 - Flags: approval-mozilla-beta?
Attachment #658111 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

5 years ago
Duplicate of this bug: 788121

Comment 27

5 years ago
Shouldn't the target milestone be mozilla16?
Mike, the target milestone is set to the first release where the bug is fixed via an in-channel patch.  That would be 18 in this ase.

There are approval requests on the patch for backports to the 16 and 17 branches; if those are approved, the "status-firefox16" and "status-firefox17" flags will be used to track the state on branches.
https://hg.mozilla.org/mozilla-central/rev/1181826ded9c
https://hg.mozilla.org/mozilla-central/rev/b0203e3c174c
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Attachment #657508 - Flags: approval-mozilla-beta?
Attachment #657508 - Flags: approval-mozilla-beta+
Attachment #657508 - Flags: approval-mozilla-aurora?
Attachment #657508 - Flags: approval-mozilla-aurora+
Comment on attachment 658111 [details] [diff] [review]
Part 2 - Same thing for <textarea>

has tests and it's early enough in Beta that if this lands before Monday it can go into Beta 3 and get a decent amount of testing in the Beta audience before release.
Attachment #658111 - Flags: approval-mozilla-beta?
Attachment #658111 - Flags: approval-mozilla-beta+
Attachment #658111 - Flags: approval-mozilla-aurora?
Attachment #658111 - Flags: approval-mozilla-aurora+
(Assignee)

Comment 31

5 years ago
Pushed to Beta:
https://hg.mozilla.org/releases/mozilla-beta/rev/3a7a2c2c6ea7
https://hg.mozilla.org/releases/mozilla-beta/rev/399c829bc942
status-firefox16: affected → fixed
(Assignee)

Comment 32

5 years ago
Pushed to Aurora:
https://hg.mozilla.org/releases/mozilla-aurora/rev/bc2b6a313317
https://hg.mozilla.org/releases/mozilla-aurora/rev/b1018e2ac8a1
status-firefox17: affected → fixed

Updated

5 years ago
Duplicate of this bug: 791847
Duplicate of this bug: 791203
Keywords: verifyme
(Assignee)

Comment 35

5 years ago
Anthony, there is no need to verify that bug: we have automatic tests for that.
Removing verifyme keyword based on comment #35.
Keywords: verifyme
Thanks Mounir, flagging [qa-].
Whiteboard: [qa-]
Duplicate of this bug: 798465
You need to log in before you can comment on or make changes to this bug.