Last Comment Bug 640136 - onchange & input events are not fired for all form elements on restore
: onchange & input events are not fired for all form elements on restore
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Session Restore (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: Firefox 11
Assigned To: Paul O'Shannessy [:zpao] (not reading much bugmail, email directly)
:
Mentors:
Depends on: 698162
Blocks: 700144
  Show dependency treegraph
 
Reported: 2011-03-09 03:40 PST by boris
Modified: 2011-11-25 16:29 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Testcase (631 bytes, text/html)
2011-03-11 14:45 PST, Anthony Hughes (:ashughes) [GFX][QA][Mentor]
no flags Details
Patch v0.1 (9.75 KB, patch)
2011-06-14 14:04 PDT, Paul O'Shannessy [:zpao] (not reading much bugmail, email directly)
dietrich: feedback+
Details | Diff | Splinter Review
Patch v0.2 (11.87 KB, patch)
2011-10-12 16:49 PDT, Paul O'Shannessy [:zpao] (not reading much bugmail, email directly)
dietrich: review+
Details | Diff | Splinter Review
Patch v0.3 (12.10 KB, patch)
2011-11-07 16:03 PST, Paul O'Shannessy [:zpao] (not reading much bugmail, email directly)
dietrich: review+
Details | Diff | Splinter Review

Description boris 2011-03-09 03:40:12 PST
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; ru; rv:1.9.2.15) Gecko/20110303 Firefox/3.6.15 GTB7.1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.1; ru; rv:1.9.2.15) Gecko/20110303 Firefox/3.6.15 GTB7.1

I have a page, which contents change depending on user choise in select field. On session restore, onchange handler not fired, page gets corrupted.

Reproducible: Always

Steps to Reproduce:
small page that shows the problem on restore:

<html>
<head>
<script src="http://ajax.googleapis.com/ajax/libs/jquery/1.5.1/jquery.min.js" type="text/javascript"></script>
<script type="text/javascript">
    //<![CDATA[
    jQuery(document).ready(function(){
    	var test = jQuery('#test')
    	test.change(function(){
			jQuery('#test2').text(test.val())
    	});
	})
    //]]>
</script>

</head>
<body>
<form>
	<select id="test">
		<option label="1" value="1" selected="selected">1</option>
		<option label="2" value="2">2</option>
		<option label="3" value="3">3</option>
	</select>
</form>
<div id="test2"></div>
</body>
</html>

Actual Results:  
servers tells browser that option 1 is selected, browser restores to option 2 or 3 and onchange handler should be fired, but it doesn't
Comment 1 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-03-11 14:45:19 PST
Created attachment 518841 [details]
Testcase

I've attached your testcase, Boris.  In the future, please simply attach a test file instead of putting source in comment.
Comment 2 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-03-11 14:47:56 PST
I've tested your testcase in Firefox 4.0rc1.

1. Open the testcase
2. Select "1" from the option
3. Close Firefox
4. Start Firefox
5. Click "Restore Session" from about:home

Result:
Option 1 is still selected.

Is this what is expected? I'm not 100% sure what this bug actually is.
Comment 3 boris 2011-03-12 03:59:52 PST
>I've attached your testcase, Boris.  In the future, please simply attach a test
>file instead of putting source in comment.

Sorry, I'm using your issue tracker for a first time, not used to it yet :)

>Result:
>Option 1 is still selected.

>Is this what is expected? I'm not 100% sure what this bug actually is.

There is an onchange handler, it should be fired if session restore gives a select any value that differs from what server said to be selected (I mean an option having selected="selected", or a first one if none has that attribute).

What is expected: I open the testcase, change to, say, option 2. Onchange fires, and "2" appears in a div below. Close the browser, reopen. Session restores, option 2 gets selected as expected. But the div below is empty - handler not fired, page corrupted.

The real world application i'm working on is a tour search form - changing destination country makes arrival dates, hotels and many more to change, session restore break all of that. Having to workaround with forcing change event on page load and it makes an unnecessary ajax request, making user wait.
Comment 4 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-03-17 10:19:29 PDT
Ok, I'm seeing that now. It is also reproducible on Firefox 4.0rc1.  Moving to NEW.

Steps:
1. Open the attached testcase
2. Select "2" from the select box (notice a "2" appears below)
3. Restart Firefox
4. Click "Restore Session" from about:home

Result:
2 is selected in select box but the "2" in the div is missing.

Expected:
2 selected in select box and 2 displayed in the div
Comment 5 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-06-14 14:04:18 PDT
Created attachment 539318 [details] [diff] [review]
Patch v0.1

I may have gone overboard here, but I made events fire for all of the form fields we restore. There was a comment saying "NB: dispatching "change" events might have unintended side-effects" but I'm not sure if it's a problem. I added tests, but I think that test file wasn't actually the right one (I did this while offline, so didn't know what bug 476161 actually tested). I kind of want to change that file to be about testing form restore events in general...
Comment 6 Dietrich Ayala (:dietrich) 2011-06-15 10:17:55 PDT
Comment on attachment 539318 [details] [diff] [review]
Patch v0.1

Review of attachment 539318 [details] [diff] [review]:
-----------------------------------------------------------------

yeah there might be real life web usage that this could be problematic for... but i can't think of anything obvious. let's get it in nightly tester's hands and take it from there.
Comment 7 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-10-12 16:49:30 PDT
Created attachment 566683 [details] [diff] [review]
Patch v0.2

Wrapping this up to clean out my patch queue.
Comment 8 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-10-25 15:15:41 PDT
pushed to fx-team: https://hg.mozilla.org/integration/fx-team/rev/78c921e2b56b
Comment 9 :Margaret Leibovic 2011-10-27 11:38:59 PDT
https://hg.mozilla.org/mozilla-central/rev/78c921e2b56b
Comment 10 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-10-28 18:19:26 PDT
Backed out with https://hg.mozilla.org/mozilla-central/rev/dfbe9a0fbf97 on account of bug 698162
Comment 11 Jim Jeffery not reading bug-mail 1/2/11 2011-10-29 01:50:37 PDT
FWIW, when this patch was in m-c this forum:
http://forums.mozillazine.org/viewforum.php?f=23

would ask to re-send POST data when restarting the browser.  Running a build now with this patch backed out, and the forum is no longer asking to re-send POST.
Comment 12 neil@parkwaycc.co.uk 2011-10-29 02:55:58 PDT
(In reply to boris from comment #0)
> I have a page, which contents change depending on user choise in select
> field. On session restore, onchange handler not fired, page gets corrupted.
How does the page deal with history navigation?
Comment 13 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-11-07 16:03:32 PST
Created attachment 572663 [details] [diff] [review]
Patch v0.3

I looked into bug 698162 a bit and figured it out. Bugzilla listens for change events on product/component and reset assignee. We were started firing change events here, even if the default value was selected, which is probably inappropriate.

The problem is that we aren't filtering default values out for <select>s because it's a lot of work for minimal savings. So I just added a check to make sure if the <select> has the same selectedIndex as what we're about to set, we don't fire the event. This fixes the bugzilla case and is probably a better behavior on the web in general.
Comment 14 Dietrich Ayala (:dietrich) 2011-11-08 09:27:57 PST
Comment on attachment 572663 [details] [diff] [review]
Patch v0.3

Review of attachment 572663 [details] [diff] [review]:
-----------------------------------------------------------------

nice and simple, thanks paul, r=me.
Comment 15 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-11-08 15:33:10 PST
https://hg.mozilla.org/integration/fx-team/rev/47f9a8d155a1
Comment 16 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2011-11-09 02:05:08 PST
https://hg.mozilla.org/mozilla-central/rev/47f9a8d155a1
Comment 17 Jim Jeffery not reading bug-mail 1/2/11 2011-11-11 04:54:11 PST
This seems to have returned with the latest hourly build from m-c win32:

http://hg.mozilla.org/mozilla-central/rev/50c1bcb49c76
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:11.0a1) Gecko/20111111 Firefox/11.0a1

Open any bug
Close Firefox
Start Firefox
Note that the editable fields 'Assigned to' & 'QA Contact' have the checkboxes exposed and 'checked' showing 'reset to default'

Now I'm confused if its this bug, or:
https://bugzilla.mozilla.org/show_bug.cgi?id=698162
Comment 18 Jim Jeffery not reading bug-mail 1/2/11 2011-11-11 04:56:30 PST
With reference to comment above, it seems intermittant, I just tested again, and didn't repo the problem.  Maybe something is 'racy' ?
Comment 19 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-11-11 08:21:15 PST
(In reply to Jim Jeffery not reading bug-mail 1/2/11 from comment #18)
> With reference to comment above, it seems intermittant, I just tested again,
> and didn't repo the problem.  Maybe something is 'racy' ?

This could be a race condition between restoration of saved form data and setting of default state values onLoad. I wouldn't know without checking the code.
Comment 20 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-11-11 11:32:08 PST
(In reply to Jim Jeffery not reading bug-mail 1/2/11 from comment #17)
> Now I'm confused if its this bug, or:
> https://bugzilla.mozilla.org/show_bug.cgi?id=698162

That bug was caused by the initial landing of this bug, so it's fine to comment here.

(In reply to Jim Jeffery not reading bug-mail 1/2/11 from comment #18)
> With reference to comment above, it seems intermittant, I just tested again,
> and didn't repo the problem.  Maybe something is 'racy' ?

Keep an eye out. I'll do the same of course. It's possible that an additional component or product as added, which could trigger us to send the change event (and in turn tell bugzilla to reset the assignee).

(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #19)
> This could be a race condition between restoration of saved form data and
> setting of default state values onLoad. I wouldn't know without checking the
> code.

Defaults are typically set in the html (eg. <option selected>) but it is possible. I think bugzilla sets it right in the html and doesn't modify fields after load, but I could be mistaken.
Comment 21 boris 2011-11-11 12:34:04 PST
(In reply to neil@parkwaycc.co.uk from comment #12)
> (In reply to boris from comment #0)
> > I have a page, which contents change depending on user choise in select
> > field. On session restore, onchange handler not fired, page gets corrupted.
> How does the page deal with history navigation?

Sorry for delay. That page does nothing with history - there's just a search form and results below it, updated thru ajax on every change in form.
Comment 22 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-11-11 13:22:55 PST
(In reply to boris from comment #21)
> Sorry for delay. That page does nothing with history - there's just a search
> form and results below it, updated thru ajax on every change in form.

Neil meant "how does that page deal with the user using the back/forward buttons to navigate to/from it?"
Comment 23 boris 2011-11-12 03:28:58 PST
(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #22)
> Neil meant "how does that page deal with the user using the back/forward
> buttons to navigate to/from it?"
Yes, I answered exactly that question - no dealing. But what handling are you talking about? Adding hashes #state1, #state2, ... to history or some document load/unload handling or something else? Anyway can't see if it's relevant to this topic.
Comment 24 neil@parkwaycc.co.uk 2011-11-12 13:09:37 PST
(In reply to boris from comment #23)
> But what handling are you talking about?
When you navigate back/forward, session history will try to restore the contents that the form fields had when you navigated away. I wasn't aware that this restoration triggered any onchange handler that the page could use to fix itself to display what you would expect if someone had typed into the field.
Comment 25 neil@parkwaycc.co.uk 2011-11-12 13:28:11 PST
If you load the test case, navigate away from the page, arrange for the bfcache for that page to be discarded¹, and then navigate back to the page, then the select's value is still restored as expected, but nothing else is.

¹E.g. by loading a sufficiently large number of intervening pages.
Comment 26 boris 2011-11-12 14:09:20 PST
Ok, that's clear now. It looks the same problem to me. I don't know if page's state (while navigating back and forth) is maintained by the same mechanism that used for session restore - if it's not and other part of code is responsible of that, than it should be considered buggy too.

The truth is other browsers are doing bad too (even worse) and I working around this inconsistencies by checking fields are having default values, if not checking if onchange fired for the last state, if not - forcing onchange handler. But I really shouldn't. I think it's a valid point - if a browser imitates user actions that will normally fire events that I might want to handle - that events should be fired. No matter if it's session restore or navigating back/forward or something else.

P.S.: going to open tickets for other browsers having same problems now and link them here
Comment 27 neil@parkwaycc.co.uk 2011-11-25 16:29:33 PST
(In reply to boris from comment #26)
> Ok, that's clear now. It looks the same problem to me. I don't know if
> page's state (while navigating back and forth) is maintained by the same
> mechanism that used for session restore - if it's not and other part of code
> is responsible of that, than it should be considered buggy too.
Page navigation is normally handled by session history, and as far as I know it too does not fire events for the form elements that it restores. (Session restore doesn't restore that part of session history, it uses its own code.)

However in some cases page navigation is handled by the bfcache, in which case the page is not discarded and regenerated but simply temporarily suspended. In this case your code does not need to be executed because its effects have been frozen along with the rest of the page.

Note You need to log in before you can comment on or make changes to this bug.