selectfilter2 many to many widget data loss when using the back button

[Maura Chace]

Here's the setup:

1. You're on an admin page with a many to many javascript widget.
   
2. You leave this page, say, with "view on site."
   
3. You hit the back button to go back to the admin page.

In Chrome (5.0.375.55 beta), your many to many widget is now clear (all of the choices are unselected). This happens whether you have edited the selection or not. I see this with Django 1.1 and 1.2. It also happens to me sometimes in Safari (4.0.5).

[James Bennett]

I've been able to reproduce this in Chrome and Safari, but not in Opera or Firefox. Thinking it might be an issue with Webkit pulling the page out of cache after the widget's JS has replaced the original select element.

[stringfellow]

Worse still, after pressing save, if I hit the back button, the selected m2m box fills with a number of choices, seemingly as many as there were previously but selected alphabetically rather than by their ID. If (carelessly, as I did) one then adds another to the selected box and hits save, the new set of m2m objects will be the ones that were prefilled, and thus, of course, wrong (unless you happened to want the first X in the list).

[Julien Phalip]

Webkit seems to have sorted itself out. I could not reproduce this on Safari 5.0.5 (6533.21.1) and Chrome 11.0.696.57.

[Collin Anderson]

I'm having this problem when saving an object that has the filter_horizontal widget in Safari/533.21.1 and Chrome 11.0.696.68.

User permissions are a good example. Go to the change page of a user that has permissions selected. Don't modify anything, press save, then press back. The permissions will be set to the first n permissions in the list, where n is the number of permissions that they originally had.

[anonymous]

Confirming the issue still is presenting itself in django 1.2 with Chrome 13.0.782.220. As stated by CollinAnderson, both halfs of this issue are easily reproduced 100% of the time using user permissions.

1. If you use navigation other than save from a user change page and then hit your back button you will find all permissions for that user will be cleared. Refreshing the change page will restore the list to it's correct values. However, if you save without doing that you will remove all previously added permissions.

2. If you save a user change page (with or without making any changes) and then hit the back button the user permissions with have the first n permissions in the list.

This is a pretty nasty bug in the sneaky way it screws up your data. It's very easy to miss while making edits in the admin and could completely screw things up. This effect all many to many relationships (although obviously user permissions is a easy example)

[Julien Phalip]

Yes, I confirm the issue too. Not sure why I couldn't reproduce it in comment:4. This happens both with filter_vertical and filter_horizontal.

[Julien Phalip]

Here is a suggested fix. I haven't tested it extensively in all browsers yet, but it seems to work. Please give it a whirl and see how you go.

[Collin Anderson]

Yes, that patch seems to have fixed it for me on Chrome. Thanks! IE 7/8 and Firefox appear to continue work fine.

[Preston Holmes]

Just adding a datapoint, seems to be problem free for me with Safari 5.0.2 os x 10.6.8

[Julien Phalip]

I've uploaded a patch with a more robust and optimised implementation. I've successfully tested it in multiple browsers but I'd appreciate more people testing it in multiple browsers and platforms to confirm that the bug is fixed and that the behaviour remains correct in non-buggy browsers. Thanks!

[Bas Peschier]

OSX 10.6:

• Safari 5.1 ✓
• WebKit nightly (2011-09-25) ✓
• Firefox 7.0 ✓
• Aurora 8.0a2 ✓
• Chrome 14.0 ✓
• Chrome Canary 16.0 ✓
• Opera 11.51 ✓
• Camino 2.0 ✓

[Ramiro Morales]

I'd like to suggest to the contributors that see this issue to also make sure they report it to their browser vendors (Webkit-based ones as can be gleaned from the comments posted) so such a problem is triaged and possibly fixed by them ASAP considering it can cause data corruption.

I'd go even further and change the type value (Bug) of this ticket, unfortunately we don't have a value for such field that is more correct to describe the workaround Julien has implemented.

[Julien Phalip]

@ramiro: It works with all browsers, including Webkit-based ones, when using a plain <select> widget. The problem here is that the javascript modifies the widget, in particular its name attribute. It's still unclear to me why Chrome behaves differently than other browsers, but it could be argued that Django's javascript is responsible for this issue and that it shouldn't be modifying the widget in such a way.

By the way, the suggested workaround leaves the original <select> widget alone, hides it, and uses the dynamically created 'from' and 'to' widgets only to handle the UI while updating the hidden widget in the background with the selected values, so that the browser only has to deal with a plain <select> widget when submitting the form or when navigating to and away from the page.

[Julien Phalip]

I've managed to get the simplest way to illustrate the root cause of this whole issue:

<!DOCTYPE html>
<html>
<body>

<select multiple="multiple" id="original" name="choice">
    <option value="1">1</option>
    <option value="2" selected="selected">2</option>
    <option value="3" selected="selected">3</option>
</select>

<script type="text/javascript">
	var original = document.getElementById("original");

	var clone = document.createElement('select');
	clone.setAttribute('multiple', 'multiple');
	clone.setAttribute('name', original.getAttribute('name'));
	original.parentNode.appendChild(clone);

	original.setAttribute('name', original.getAttribute('name') + '_old');
</script>

<p>To reproduce the problem:</p>
<ol>
    <li>Note how some options are selected by default in the select box above.</li>
    <li>Navigate away from this page (e.g. by clicking this <a href="http://google.com">link</a>) and then go back to this page by pressing the browser's 'Back' button.
    <li>Note how the options above aren't selected any more.</li>
</ol>

</body>
</html>

I'm happy to report this issue to the vendors. However, I swear that this problem used to exist in Safari until a couple of weeks ago, and so like the reporter I suspected that the problem was with Webkit. But I can't reproduce it in Safari any more (using 5.1 / 6534.50). The problem is now only showing up in Chrome (using 15.0.874.24 beta) :-/

Can someone reproduce the problem using the code above in a Webkit browser other than Chrome or Safari?

[Julien Phalip]

I've filed a ticket for Chromium: ​http://code.google.com/p/chromium/issues/detail?id=98038

If it doesn't get fixed in time for 1.4 then I'll push the workaround.

[anonymous]

The above fix (13614.filterselect-webkit-bug.2.diff) does not work for Chrome 15.0.874.121 and Django 1.3.1.
If you navigate from the admin page, that contains SelectFilter2, to some other page and than click 'back' button - SelectFilter2 preserves values - this is good. But if you decide to change SelectFilter2 values and hit 'save' - it does not save changes. Am I the only one?

[Collin Anderson]

I'm finding that patch 2 isn't even preserving the values correctly for me when I click back. patch 1 works fine for me on Chrome 15.0.874.121 on Windows. I am using Django 1.3.1, but patching against the latest SelectFilter2.js from trunk.

[Julien Phalip]

Just a quick note: there has been some recent discussions between the chromium guys about the root cause of this issue: ​http://code.google.com/p/chromium/issues/detail?id=76739

... although I'm not sure if there's any chance for them to fix it before Django 1.4 lands.

This is a bug in Chrome/Webkit, not in Django. However we might decide to fix our own code to get around it. In any case, selenium tests should be written to ensure the patch doesn't introduce regressions.

[Julien Phalip]

I'm marking this ticket as release-blocker as it's a serious data loss issue. I think we're pretty close to a solution with the patch but I'm going to write comprehensive Selenium tests for multiple browsers to be certain.

[Julien Phalip]

The patch above gets around the Chrome bug. However, the tests also highlight another bug in... Firefox :-/

In Firefox, if you select some options, then navigate away and go back, the options get reinitialized to their original values.

This is driving me insane :-) Some manual testing on other browsers and platforms would be welcome. Thanks!

[Aymeric Augustin]

I don't feel obligated to work around known browser bugs in this area. Most people don't use the "back" button very much -- many don't even know it exists!

[Julien Phalip]

I bit the bullet and used the patch in #15220 as a basis to rewrite the widget as a jQuery plugin, which makes everything a lot easier to understand and work with. I've also added all the improvements made for that widget in trunk over the last months. I then refactored the code to fix this bug.

However, as in the previous patch, while the bug seems to be fixed by this patch in Chrome (all Selenium tests pass), there is still an issue with Firefox (all Selenium tests pass except test_back_button_bug_13614).

With this patch, in Firefox, if there are multiple filter widgets on the page, then only the first one keeps the selected values after pressing 'Back'; all other widgets receive the initial values. Run the test_back_button_bug_13614 test to get an illustration.

I've spent a lot of time trying to debug this and I'm really stumped. I haven't been able to figure out why Firefox behaves that way yet. Any insight would be helpful.

[Aymeric Augustin]

I don't have much to say about the patch: it's a rewrite, it looks clean, and it's tested.

I tested on a model with four M2M fields, two displayed horizontally and two vertically, under OS X.

Without the patch: Firefox => OK, Chrome => KO, Safari => KO. This confirms the initial analysis that failures occurred under WebKit.

With the patch: Firefox => OK, Chrome => OK, Safari => OK.

As far as manual testing goes, it seems to me that everything is OK, even with multiple widgets. Specifically, I can't reproduce the problems you encountered with Firefox. Assuming you've checked that the widgets work under IE, the patch is good to go.

[Ramiro Morales]

Replying to julien:

With this patch, in Firefox, if there are multiple filter widgets on the page, then only the first one keeps the selected values after pressing 'Back'; all other widgets receive the initial values. Run the test_back_button_bug_13614 test to get an illustration.

I've spent a lot of time trying to debug this and I'm really stumped. I haven't been able to figure out why Firefox behaves that way yet. Any insight would be helpful.

I've tested the patch on Windows 7:

• Things work ok and as expected on Chrome (test passes)

• ... and sort of work on Firefox (10.0.2)

If (always with Firefox):

• In the relevant sections I have Peter, Jenny, Lisa and Bob in the top/left -hand lists and Jason, John, Arthur and Cliff in the bottom/right -hand lists (as left by execute_basic_operations())

• Then click the 'Home' link and then the Back browser button

I get back the status preserved in both widgets ordering included.

At this point the test_back_button_bug_13614() test method seems to expect them in e.g. the Bob, Jenny, Lisa, Peter order.

Then, when I save the School and come back to edit it again... I get the order the tests expect (maybe by Student PK?).

Maybe we can change at least one of the assertions to just assert that set(...) == set(...)?

[Aymeric Augustin]

#17826 could be fixed by this patch too (to be tested).

[Ramiro Morales]

Patch updated to ignore ordering of options when checking contents of vertical/horizontal select widgets

[Aymeric Augustin]

We're about to release 1.4 RC1, but:

• the patch is a complete rewrite, which involves some risks,
• the Selenium tests break under IE,
• they also randomly break under FF.

We're running short on time and we aren't sufficiently familiar with Selenium yet to make reliable and consistent tests across browsers.

So, at this time, it appears safer to live with the current behavior and fix this properly in 1.4.1. While this bug can turn into a data loss issue, it's still an edge case. Julien agreed on this plan on IRC.

I will leave the "release blocker" status set so we don't forget this ticket for 1.4.1, but we accept to release 1.4 without a fix.

[Julien Phalip]

Removing the "release blocker" flag to avoid confusion. We are not going to ship a fix for this in 1.4. It will stay on my radar for later.

[KyleMac]

I got fed up with how slow and crash happy SelectBox was with large (1000-11000) lists and rewrote a large chunk and submitted a pull request at Github in reference to #3202. I wish I had seen this ticket and knew that you guys were willing to accept a total jQuery based rewrite because I would have done that.

The code on this page performs no faster than the original. There is no need for a refresh_state on every action and all manipulation should be done outside of the DOM. Stuff like .find('option:selected') is also very slow.

When I saw this ticket I realised that my code has an even worse version of this bug (it's still there in Chrome 20). Mine would actually select the top N options to fill the right column which I think is worse than no options at all. One of the Chromium tickets says that the problem is that Webkit remembers selections by selectedIndex rather than the value attribute which lead me to the solution.

After pressing the back button if you inspect the DOM you will notice that while option.selected is wrong the selected attribute is still actually in the HTML and correct. So what I did was call a fake replaceState so that a popstate is fired on page back and then just redisplay the selections based on the selected attribute from getAttribute. Now there's no need to waste twice the resources on a copy of the list.

​https://github.com/django/django/pull/222

[Julien Phalip]

Quick note: the Webkit bug has been fixed:
​https://bugs.webkit.org/show_bug.cgi?id=89409
​http://trac.webkit.org/changeset/120895

It might take a little while before it gets deployed in the public versions of Safari and Chrome, so some tests should still be run before we can close this ticket.

@KyleMac: Thank you for your work. I'm in favour of rewriting this widget in jQuery, so if you're interested in working on that, you may take a look at ticket #15220.

[Tim Graham]

I cannot reproduce with Chrome 24. If someone can confirm this can't be reproduced with the latest version of Safari I think we can close it.

[Julien Phalip]

Unfortunately I'm still seeing the problem in Chrome (25.0.1364.68 beta) and Safari (6.0.2). It still works well for me in Firefox (18.0.1).

[Collin Anderson]

I can't reproduce it in Chrome Stable (24.0.1312.56 m) or (24.0.1312.57 m) on Windows XP. I tried django 1.3 and django 1.4.

[Adrian Holovaty]

A related issue I'm seeing in Firefox 25 is that clicking reload clears all selectfilter2 fields, whereas a shift-click on reload works fine. A user who reloads an admin page hoping to reset their changes prior to saving can inadvertently clear several m2m fields at once.

[Collin Anderson]

I can confirm Adrian's related issue that in Firefox 26 clicking reload clears out values in the field, both in django 1.3 and django 1.6. It works fine in chrome.

[Collin Anderson]

The reload issue is still there in Firefox 28.

[Collin Anderson]

see also #18879

[Claude Paroz]

I tried to update the patch and make a pull request: ​https://github.com/django/django/pull/4701

However, the problem is still there on my Firefox 37 (that is admin_widgets.tests.HorizontalVerticalFilterSeleniumFirefoxTests.test_back_button_bug_13614 fails).
I wonder if this is something Django can really fix.

[Tim Graham]

The reload issue from comment 36 is tracked in #22955 and should be fixed in Django 1.8.8+.

As for the original issue with the back button, can anyone reproduce that? I'm attached a modified version of the selenium test from Claude's branch which I believe should reproduce the problem -- the test in that branch didn't seem correct to me. The new test passes for me on both Chrome 47 and Firefox 43.

[Collin Anderson]

I can't reproduce the issue in Chrome or Firefox.

[Tim Graham]

​PR to add the regression test.

[Tim Graham <timograham@…>]

In 7bc94b5:

Refs #13614 -- Added test for admin's many-to-many widget data loss bug.

It looks like browsers have fixed the reported issue.