Opened 7 years ago

Closed 11 months ago

#13614 closed Bug (worksforme)

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

Reported by: Maura Chace Owned by: Julien Phalip
Component: contrib.admin Version: 1.3
Severity: Normal Keywords:
Cc: cmawebsite@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: yes

Description

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

Attachments (6)

13614.filterselect-webkit-bug.diff (4.7 KB) - added by Julien Phalip 5 years ago.
13614.filterselect-webkit-bug.2.diff (7.8 KB) - added by Julien Phalip 5 years ago.
13614.filterselect-bug.3.diff (16.6 KB) - added by Julien Phalip 5 years ago.
13614.filterselect-bug.4.diff (41.0 KB) - added by Julien Phalip 5 years ago.
13614.filterselect-bug.5.diff (41.8 KB) - added by Ramiro Morales 5 years ago.
Patch updated to ignore ordering of options when checking contents of vertical/horizontal select widgets
13614-test.diff (2.1 KB) - added by Tim Graham 12 months ago.

Download all attachments as: .zip

Change History (50)

comment:1 Changed 7 years ago by James Bennett

Triage Stage: UnreviewedAccepted

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.

comment:2 Changed 6 years ago by 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).

comment:3 Changed 6 years ago by Julien Phalip

Severity: Normal
Type: Bug

comment:4 Changed 6 years ago by Julien Phalip

Easy pickings: unset
Resolution: worksforme
Status: newclosed

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.

comment:5 Changed 5 years ago by Collin Anderson

Cc: cmawebsite@… added
Resolution: worksforme
Status: closedreopened
UI/UX: unset

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.

comment:6 Changed 5 years ago by anonymous

UI/UX: set

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

comment:7 Changed 5 years ago by 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.

Changed 5 years ago by Julien Phalip

comment:8 Changed 5 years ago by Julien Phalip

Has patch: set

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.

comment:10 Changed 5 years ago by 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.

comment:11 Changed 5 years ago by Preston Holmes

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

Changed 5 years ago by Julien Phalip

comment:12 Changed 5 years ago by 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!

comment:13 Changed 5 years ago by 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 ✓

comment:14 Changed 5 years ago by 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.

comment:15 Changed 5 years ago by 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.

comment:16 Changed 5 years ago by 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?

comment:17 Changed 5 years ago by 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.

comment:18 Changed 5 years ago by anonymous

Version: 1.21.3

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?

comment:19 Changed 5 years ago by 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.

comment:20 Changed 5 years ago by 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.

comment:21 Changed 5 years ago by Julien Phalip

Severity: NormalRelease blocker

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.

Changed 5 years ago by Julien Phalip

comment:22 Changed 5 years ago by 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!

comment:23 Changed 5 years ago by 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!

comment:24 Changed 5 years ago by Aymeric Augustin

Owner: changed from nobody to Julien Phalip
Status: reopenednew

Changed 5 years ago by Julien Phalip

comment:25 Changed 5 years ago by 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.

comment:26 Changed 5 years ago by Aymeric Augustin

Triage Stage: AcceptedReady for checkin

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.

comment:27 in reply to:  25 Changed 5 years ago by 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(...)?

comment:28 Changed 5 years ago by Aymeric Augustin

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

Changed 5 years ago by Ramiro Morales

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

comment:29 Changed 5 years ago by 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.

comment:30 Changed 5 years ago by Julien Phalip

Patch needs improvement: set
Severity: Release blockerNormal
Triage Stage: Ready for checkinAccepted

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.

comment:31 Changed 4 years ago by 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

comment:32 Changed 4 years ago by 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.

comment:33 Changed 4 years ago by 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.

comment:34 Changed 4 years ago by 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).

comment:35 Changed 4 years ago by 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.

comment:36 Changed 3 years ago by 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.

comment:37 Changed 3 years ago by Collin Anderson

Summary: Back button breaks many to many widgetselectfilter2 many to many widget data loss with browser issues

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.

comment:38 Changed 3 years ago by Collin Anderson

The reload issue is still there in Firefox 28.

comment:39 Changed 2 years ago by Collin Anderson

see also #18879

comment:40 Changed 19 months ago by 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.

comment:41 Changed 12 months ago by Tim Graham

Summary: selectfilter2 many to many widget data loss with browser issuesselectfilter2 many to many widget data loss when using the back button

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.

Changed 12 months ago by Tim Graham

Attachment: 13614-test.diff added

comment:42 Changed 11 months ago by Collin Anderson

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

comment:43 Changed 11 months ago by Tim Graham

Patch needs improvement: unset

PR to add the regression test.

comment:44 Changed 11 months ago by 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.

comment:45 Changed 11 months ago by Tim Graham

Resolution: worksforme
Status: newclosed
Note: See TracTickets for help on using tickets.
Back to Top