Ticket Show property changes preference has no effect

[Ryan J Ollos]

The checkbox does not seem to be working on t.e.o (Trac 1.2dev), but it functions correctly on the other Edgewall Trac sites, which run Trac 1.0.9.

I haven't tested whether the problem is reproducible with a clean install of 1.2dev.

[Ryan J Ollos]

Also, I don't see any errors in the browser console.

[Peter Suter]

Works for me in Firefox, but not in Chrome.

showPropertyChanges.attr('checked')​ seems to always be "checked" in Chrome. Maybe showPropertyChanges.prop('checked') should be used instead?

(Strange that it works with Trac 1.0.9 though.)

[Ryan J Ollos]

I see the issue with Chrome 47 and Firefox 42 on Mac OSX. I can reproduce on the trunk, but not on 1.0-stable. The difference in behavior is probably due to the upgrade of the jQuery libraries in #11019.

The proposed fix works well and makes sense to me. I'll push the change in a day or so if there is no other feedback. Thanks!

[Peter Suter]

(Ah, probably it only "worked" for me because I still had the old jQuery cached in Firefox.)

​.attr() versus .prop() is also mentioned in the jQuery upgrade guide linked in #11019.

[Ryan J Ollos]

Fixed in [14372].

[Jun Omae]

It seems that line 34 and 96 have the same issue.

$ grep -n '\.attr(.checked' trac/htdocs/js/*.js
trac/htdocs/js/threaded_comments.js:34:    var showPropertyChangesChecked = showPropertyChanges.attr('checked');
trac/htdocs/js/threaded_comments.js:36:      showPropertyChanges.attr("checked", false);
trac/htdocs/js/threaded_comments.js:56:      showPropertyChanges.attr("checked", true);
trac/htdocs/js/threaded_comments.js:78:    .attr('checked', 'checked');
trac/htdocs/js/threaded_comments.js:90:  showPropertyChanges.attr('checked', comments_prefs.comments_only == 'false');
trac/htdocs/js/threaded_comments.js:96:      ticket_comments_only: !showPropertyChanges.attr('checked'),

[Ryan J Ollos]

Thanks for catching that. There is still an issue that the Show property changes state is not preserved when switching between Oldest first and Newest first. For handling checked, we should just change all attr to prop?

trac/htdocs/js/threaded_comments.js

@@ -31 +31 @@
   };
 
   var applyOrder = function() {
-    var showPropertyChangesChecked = showPropertyChanges.attr('checked');
+    var showPropertyChangesChecked = showPropertyChanges.prop('checked');
     if (showPropertyChangesChecked) {
-      showPropertyChanges.attr("checked", false);
+      showPropertyChanges.prop("checked", false);
       applyShowPropertyChanges();
     }
     order = $("input[name='trac-comments-order']:checked").val();
@@ -53 +53 @@
       });
     }
     if (showPropertyChangesChecked) {
-      showPropertyChanges.attr("checked", true);
+      showPropertyChanges.prop("checked", true);
       applyShowPropertyChanges();
     }
   };
@@ -75 +75 @@
 
   $("input[name='trac-comments-order']")
     .filter("[value=" + comments_prefs.comments_order + "]")
-    .attr('checked', 'checked');
+    .prop('checked', true);
   applyOrder();
   $("input[name='trac-comments-order']").change(function() {
     unapplyOrder();
@@ -87 +87 @@
     }, dataType: 'text' });
   });
 
-  showPropertyChanges.attr('checked', comments_prefs.comments_only == 'false');
+  showPropertyChanges.prop('checked', comments_prefs.comments_only == 'false');
   applyShowPropertyChanges();
   showPropertyChanges.click(function() {
     applyShowPropertyChanges();
     $.ajax({ url: form.attr('action'), type: 'POST', data: {
       save_prefs: true,
-      ticket_comments_only: !showPropertyChanges.attr('checked'),
+      ticket_comments_only: !showPropertyChanges.prop('checked'),
       __FORM_TOKEN: form_token
     }, dataType: 'text' });
   });

[Ryan J Ollos]

I noticed that similar changes were staged in #11835.

comment:8 changes committed to trunk in [14375].