Edgewall Software
Modify

Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#12260 closed defect (fixed)

Ticket Show property changes preference has no effect

Reported by: Ryan J Ollos Owned by: Peter Suter
Priority: normal Milestone: 1.2
Component: general Version:
Severity: normal Keywords:
Cc: Branch:
Release Notes:

Fixed regression in Show property changes preference on ticket page, after upgrade to jQuery 1.11.3 (#11019).

API Changes:
Internal Changes:

Description

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.

Attachments (0)

Change History (10)

comment:1 by Ryan J Ollos, 8 years ago

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

comment:2 by Peter Suter, 8 years ago

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

Last edited 8 years ago by Peter Suter (previous) (diff)

comment:3 by Ryan J Ollos, 8 years ago

Milestone: 1.2
Owner: set to Ryan J Ollos
Status: newassigned

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!

comment:4 by Peter Suter, 8 years ago

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

comment:5 by Ryan J Ollos, 8 years ago

Release Notes: modified (diff)
Resolution: fixed
Status: assignedclosed

Fixed in [14372].

comment:6 by Ryan J Ollos, 8 years ago

Owner: changed from Ryan J Ollos to Peter Suter

comment:7 by Jun Omae, 8 years ago

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:24:    if (commentsOnly.attr('checked')) {
trac/htdocs/js/threaded_comments.js:34:    var commentsOnlyChecked = commentsOnly.attr('checked');
trac/htdocs/js/threaded_comments.js:36:      commentsOnly.attr("checked", false);
trac/htdocs/js/threaded_comments.js:56:      commentsOnly.attr("checked", true);
trac/htdocs/js/threaded_comments.js:78:    .attr('checked', 'checked');
trac/htdocs/js/threaded_comments.js:90:  commentsOnly.attr('checked', comments_prefs.comments_only != 'false');
trac/htdocs/js/threaded_comments.js:96:      ticket_comments_only: !!commentsOnly.attr('checked'),
Version 0, edited 8 years ago by Jun Omae (next)

comment:8 by Ryan J Ollos, 8 years ago

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

    diff --git a/trac/htdocs/js/threaded_comments.js b/trac/htdocs/js/threaded_comments.js
    index 03bd1db..f5fa774 100644
    a b jQuery(document).ready(function($){  
    3131  };
    3232
    3333  var applyOrder = function() {
    34     var showPropertyChangesChecked = showPropertyChanges.attr('checked');
     34    var showPropertyChangesChecked = showPropertyChanges.prop('checked');
    3535    if (showPropertyChangesChecked) {
    36       showPropertyChanges.attr("checked", false);
     36      showPropertyChanges.prop("checked", false);
    3737      applyShowPropertyChanges();
    3838    }
    3939    order = $("input[name='trac-comments-order']:checked").val();
    jQuery(document).ready(function($){  
    5353      });
    5454    }
    5555    if (showPropertyChangesChecked) {
    56       showPropertyChanges.attr("checked", true);
     56      showPropertyChanges.prop("checked", true);
    5757      applyShowPropertyChanges();
    5858    }
    5959  };
    jQuery(document).ready(function($){  
    7575
    7676  $("input[name='trac-comments-order']")
    7777    .filter("[value=" + comments_prefs.comments_order + "]")
    78     .attr('checked', 'checked');
     78    .prop('checked', true);
    7979  applyOrder();
    8080  $("input[name='trac-comments-order']").change(function() {
    8181    unapplyOrder();
    jQuery(document).ready(function($){  
    8787    }, dataType: 'text' });
    8888  });
    8989
    90   showPropertyChanges.attr('checked', comments_prefs.comments_only == 'false');
     90  showPropertyChanges.prop('checked', comments_prefs.comments_only == 'false');
    9191  applyShowPropertyChanges();
    9292  showPropertyChanges.click(function() {
    9393    applyShowPropertyChanges();
    9494    $.ajax({ url: form.attr('action'), type: 'POST', data: {
    9595      save_prefs: true,
    96       ticket_comments_only: !showPropertyChanges.attr('checked'),
     96      ticket_comments_only: !showPropertyChanges.prop('checked'),
    9797      __FORM_TOKEN: form_token
    9898    }, dataType: 'text' });
    9999  });

comment:9 by Ryan J Ollos, 8 years ago

I noticed that similar changes were staged in #11835.

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

comment:10 by Ryan J Ollos, 8 years ago

Summary: The Show Property Changes checkbox has no effect on t.e.oTicket Show property changes preference has no effect

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Peter Suter.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from Peter Suter to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.