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

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Mike Walsh]

Attachment: 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

[Alice0775 White]

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

Comment 2

[Mounir Lamouri (:mounir)]

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.

Comment 4

[Mike Moening]

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. :-)

Comment 5

[Mounir Lamouri (:mounir)]

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

Comment 6

[Mounir Lamouri (:mounir)]

Attachment: Opportunistic patch

Comment 7

[Mike Moening]

Mounir,
Why is Firefox 15 "wontfix" ?
Thanks.

Comment 8

[Mounir Lamouri (:mounir)]

(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

[Alex Keybl [:akeybl]]

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

Comment 10

[Mike Moening]

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

Comment 11

[Alex Keybl [:akeybl]]

(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?

Comment 12

[Andrew Quartey [:drexler]]

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

Comment 13

[Mounir Lamouri (:mounir)]

Attachment: Patch

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

Andrew, could you review it?

Comment 14

[Mounir Lamouri (:mounir)]

Comment on attachment 657508 [details] [diff] [review]
Patch

Asking a review from bz to because I need a DOM peer review.

Comment 15

[Mounir Lamouri (:mounir)]

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

Comment 16

[Mounir Lamouri (:mounir)]

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

Comment 17

[Mounir Lamouri (:mounir)]

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 18

[Boris Zbarsky [:bzbarsky]]

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. :(

Comment 19

[Andrew Quartey [:drexler]]

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.

Comment 20

[Mounir Lamouri (:mounir)]

Attachment: Part 2 - Same thing for <textarea>

Comment 21

[Boris Zbarsky [: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.  :(

Comment 22

[Mounir Lamouri (:mounir)]

I will try to work on making this better. Fixing the bug seems a higher priority I believe.

Comment 23

[Mounir Lamouri (:mounir)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1181826ded9c
https://hg.mozilla.org/integration/mozilla-inbound/rev/b0203e3c174c

Comment 24

[Mounir Lamouri (:mounir)]

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.

Comment 25

[Mounir Lamouri (:mounir)]

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

[Approval Request Comment]
See previous comment.

Comment 27

[Mike Moening]

Shouldn't the target milestone be mozilla16?

Comment 28

[Boris Zbarsky [:bzbarsky]]

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.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1181826ded9c
https://hg.mozilla.org/mozilla-central/rev/b0203e3c174c

Comment 30

[Lukas Blakk [:lsblakk] use ?needinfo]

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.

Comment 31

[Mounir Lamouri (:mounir)]

Pushed to Beta:
https://hg.mozilla.org/releases/mozilla-beta/rev/3a7a2c2c6ea7
https://hg.mozilla.org/releases/mozilla-beta/rev/399c829bc942

Comment 32

[Mounir Lamouri (:mounir)]

Pushed to Aurora:
https://hg.mozilla.org/releases/mozilla-aurora/rev/bc2b6a313317
https://hg.mozilla.org/releases/mozilla-aurora/rev/b1018e2ac8a1

Comment 35

[Mounir Lamouri (:mounir)]

Anthony, there is no need to verify that bug: we have automatic tests for that.

Comment 36

[Mihaela Velimiroviciu (:mihaelav)]

Removing verifyme keyword based on comment #35.

Comment 37

[u279076]

Thanks Mounir, flagging [qa-].