Code

Opened 4 years ago

Last modified 6 days ago

#13614 new Bug

selectfilter2 many to many widget data loss with browser issues

Reported by: minarets Owned by: julien
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: yes
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 (5)

13614.filterselect-webkit-bug.diff (4.7 KB) - added by julien 3 years ago.
13614.filterselect-webkit-bug.2.diff (7.8 KB) - added by julien 3 years ago.
13614.filterselect-bug.3.diff (16.6 KB) - added by julien 2 years ago.
13614.filterselect-bug.4.diff (41.0 KB) - added by julien 2 years ago.
13614.filterselect-bug.5.diff (41.8 KB) - added by ramiro 2 years ago.
Patch updated to ignore ordering of options when checking contents of vertical/horizontal select widgets

Download all attachments as: .zip

Change History (43)

comment:1 Changed 4 years ago by ubernostrum

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

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 3 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 3 years ago by julien

  • Severity set to Normal
  • Type set to Bug

comment:4 Changed 3 years ago by julien

  • Easy pickings unset
  • Resolution set to worksforme
  • Status changed from new to closed

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 3 years ago by CollinAnderson

  • Cc cmawebsite@… added
  • Resolution worksforme deleted
  • Status changed from closed to reopened
  • 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 3 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 3 years ago by julien

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 3 years ago by julien

comment:8 Changed 3 years ago by julien

  • 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:9 Changed 3 years ago by anonymous

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

comment:10 Changed 3 years ago by CollinAnderson

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 3 years ago by ptone

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

Changed 3 years ago by julien

comment:12 Changed 3 years ago by julien

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 3 years ago by bpeschier

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 3 years ago by ramiro

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 3 years ago by julien

@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 3 years ago by julien

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 3 years ago by julien

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 2 years ago by anonymous

  • Version changed from 1.2 to 1.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 2 years ago by CollinAnderson

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 2 years ago by julien

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 2 years ago by julien

  • Severity changed from Normal to Release 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 2 years ago by julien

comment:22 Changed 2 years ago by julien

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 2 years ago by aaugustin

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 2 years ago by aaugustin

  • Owner changed from nobody to julien
  • Status changed from reopened to new

Changed 2 years ago by julien

comment:25 follow-up: Changed 2 years ago by julien

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 2 years ago by aaugustin

  • Triage Stage changed from Accepted to Ready 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 2 years ago by ramiro

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 2 years ago by aaugustin

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

Changed 2 years ago by ramiro

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

comment:29 Changed 2 years ago by aaugustin

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 2 years ago by julien

  • Patch needs improvement set
  • Severity changed from Release blocker to Normal
  • Triage Stage changed from Ready for checkin to Accepted

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 21 months 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 20 months ago by julien

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 14 months ago by timo

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 14 months ago by julien

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 14 months ago by CollinAnderson

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 5 months ago by adrian

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 months ago by CollinAnderson

  • Summary changed from Back button breaks many to many widget to selectfilter2 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 6 days ago by CollinAnderson

The reload issue is still there in Firefox 28.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as new
The owner will be changed from julien to anonymous. Next status will be 'assigned'
as The resolution will be set. Next status will be 'closed'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.