Edgewall Software
Modify

Opened 8 years ago

Closed 5 years ago

Last modified 4 years ago

#10138 closed defect (fixed)

Doubleclick creates 2 tickets

Reported by: anonymous Owned by: Ryan J Ollos
Priority: normal Milestone: 1.0.2
Component: general Version: 0.12.1
Severity: minor Keywords: javascript
Cc:
Release Notes:

Multiple form submits are prevented for cases that the operation is not idempotent, or an error would result.

API Changes:

The trac-disable-on-submit class prevents multiple submissions of a form due to repeated clicks of a submit button.

Description

When you doublelick on a submit button, you get 2 tickets.

Is this a bug or, maybe, a feature?

Attachments (0)

Change History (29)

comment:1 Changed 8 years ago by Remy Blank

Certainly not a feature, but there's a logical explanation. When you double-click on the submit button, your browser starts a POST request, aborts it, then starts another one. The server doesn't necessarily see the abort until too late (typically, when it tries to send the reply back), so it processes both POST requests completely, and hence creates two tickets.

Maybe we could use some JavaScript trickery to work around the issue, like disabling the submit button after the first click.

comment:2 Changed 8 years ago by Christian Boos

Keywords: javascript added
Milestone: next-major-0.1X
Severity: normalminor

I'd imagine the browsers should try to avoid such problems with submit buttons in the first place… but OK, we could help with some javascript.

comment:3 Changed 7 years ago by Carsten Klein <carsten.klein@…>

Considering that with each form, a hidden token is being set back to the server, I wonder why the token was not expired by the first POST hitting the server and why the second time the same token hits the server, that token is still being accepted?

comment:4 in reply to:  3 Changed 7 years ago by Remy Blank

Replying to Carsten Klein <carsten.klein@…>:

Considering that with each form, a hidden token is being set back to the server, I wonder why the token was not expired by the first POST hitting the server and why the second time the same token hits the server, that token is still being accepted?

The token, which is used to avoid CSRF, isn't stored server-side, so there's nothing to expire. It's stored as a cookie, and in every form, and both are compared on submit.

comment:5 Changed 7 years ago by contato@…

I'm implementing Trac 0.12.3 at the company I work. I'm new to the system and yet, I'm teaching the developers and analysts to use it. One of the analysts double-clicks this freaking submit button every single time, creating duplicated tickets.

So, I've managed to create a small hack to solve this while there is no official "bug-fix". Keep in mind that my version is 0.12.3.

From the Trac source-code copy the file trac/ticket/templates/ticket.html (http://trac.edgewall.org/browser/trunk/trac/ticket/templates/ticket.html) to your trac_env/templates

Now, open the ticket.html that you just copied to the trac_env/templates

Go to line 376 http://trac.edgewall.org/browser/trunk/trac/ticket/templates/ticket.html#L376

And, inside the input (submit) tag, write this

onclick="javascript: this.disabled='disabled'"

So, your line will look like this

<input type="submit" name="submit" value="${ticket.exists and _('Submit changes') or _('Create ticket')}" onclick="javascript: this.disabled='disabled'" />

This nobish hack is working for me and probably will work for everybody.

comment:6 in reply to:  5 Changed 7 years ago by contato@…

Replying to contato@…:

I'm implementing Trac 0.12.3 at the company I work. I'm new to the system and yet, I'm teaching the developers and analysts to use it. One of the analysts double-clicks this freaking submit button every single time, creating duplicated tickets.

So, I've managed to create a small hack to solve this while there is no official "bug-fix". Keep in mind that my version is 0.12.3.

From the Trac source-code copy the file trac/ticket/templates/ticket.html (http://trac.edgewall.org/browser/trunk/trac/ticket/templates/ticket.html) to your trac_env/templates

Now, open the ticket.html that you just copied to the trac_env/templates

Go to line 376 http://trac.edgewall.org/browser/trunk/trac/ticket/templates/ticket.html#L376

And, inside the input (submit) tag, write this

onclick="javascript: this.disabled='disabled'"

So, your line will look like this

<input type="submit" name="submit" value="${ticket.exists and _('Submit changes') or _('Create ticket')}" onclick="javascript: this.disabled='disabled'" />

This nobish hack is working for me and probably will work for everybody.

Fogrt this hack. The web developer biggest enemy, the doomed Internet Explorer, doesnt work with this hack…

comment:7 Changed 7 years ago by contato@…

A NEW HACK HAS APPEARED

Again, keep in mind I'm working with Trac 0.12.3

The first step is the same, copy the ticket.html from trac/ticket/templates/ticket.html to your trac_env/template folder.

Go to line 398 http://trac.edgewall.org/browser/tags/trac-0.12.3/trac/ticket/templates/ticket.html#L398 and give an HTML ID to the input tag. Mine I called "freaking_submit_button", as you can see:

<input type="submit" name="submit" value="${ticket.exists and _('Submit changes') or _('Create ticket')}" id='freaking_submit_button' />

Now, you just have to make the form disable the button when it's submitted.

The tag FORM which creates the ticket form is located at line 190 http://trac.edgewall.org/browser/tags/trac-0.12.3/trac/ticket/templates/ticket.html#L190

Just create a onsubmit event to disable the button

onsubmit="document.getElementById('freaking_submit_button').disabled = 1;"

So, your FORM tag may look like this

<form py:if="has_property_editor" method="post" id="propertyform"
    action="${ticket.exists and href.ticket(ticket.id) + '#trac-add-comment' or href.newticket()}"
    onsubmit="document.getElementById('freaking_submit_button').disabled = 1;">

And this is it. Works on Iexplorer 9, Fire(awesome)Fox 12, the browsers people are using at my job.

I hope this one works fine for everybody.

comment:8 Changed 7 years ago by Jun Omae

Simple patch to avoid the issue.

  • trac/htdocs/js/trac.js

     
    131131  }
    132132
    133133})(jQuery);
     134
     135// Disable the submit buttons on post each form
     136jQuery(document).ready(function($) {
     137    $('form[method=post]').submit(function() {
     138        $(':submit', this).attr('disabled', true);
     139    }).find(':submit').attr('disabled', false);
     140});

comment:9 Changed 5 years ago by Ryan J Ollos

Milestone: next-major-releases1.0.2
Owner: set to Ryan J Ollos
Status: newassigned

The issue is also reported in th:#10297.

The patch from comment:8 works well for the ticket form, but on /admin/ticket/milestones it is no longer possible to Add, Apply changes or Remove selected items (at least with Firefox 26 on Ubuntu). I did some reading and found a code snippet that seems to work well,

  • trac/templates/layout.html

    diff --git a/trac/templates/layout.html b/trac/templates/layout.html
    index bc3eb76..777585f 100644
    a b  
    4848        $(".trac-autofocus").focus();
    4949        $(".trac-target-new").attr("target", "_blank");
    5050        setTimeout(function() { $(".trac-scroll").scrollToTop() }, 1);
     51        // Disable the submit buttons on post each form
     52        $("form[method=post]").submit(function() {
     53          $(this).submit(function() { return false; });
     54        });
    5155      });
    5256    </script>
    5357    ${select("*[local-name() != 'title']|text()|comment()")}

However, it sounds like this won't work for AJAX forms which rely on multiple submits between page loads. One suggested way to handle this is to add a special class to AJAX forms such as trac-multisubmit, and use a :not(trac-multisubmit) selector for the form when disabling the submit buttons.

  • trac/templates/layout.html

    diff --git a/trac/templates/layout.html b/trac/templates/layout.html
    index bc3eb76..777585f 100644
    a b  
    4848        $(".trac-autofocus").focus();
    4949        $(".trac-target-new").attr("target", "_blank");
    5050        setTimeout(function() { $(".trac-scroll").scrollToTop() }, 1);
     51        // Disable the submit buttons on post each form
     52        $("form[method=post]:not(.trac-multisubmit)").submit(function() {
     53          $(this).submit(function() { return false; });
     54        });
    5155      });
    5256    </script>
    5357    ${select("*[local-name() != 'title']|text()|comment()")}
Last edited 4 years ago by Jun Omae (previous) (diff)

comment:10 Changed 5 years ago by Remy Blank

Instead of a trac-multisubmit class, I would add a trac-disable-on-submit class to only those buttons where clicking isn't idempotent. For example, on the ticket page, you would set it on the "Submit changes" button but not on the "Preview" button. I expect that only relatively few buttons need to be disabled that way.

comment:11 Changed 5 years ago by Ryan J Ollos

That sounds good. log:rjollos.git:t10138 uses the trac-disable-on-submit class to avoid multiple submits in the ticket form. It should handle the case in which the user clicks the back button in the browser, allowing the form to be submitted after back is clicked. I think this is desirable for our use cases, but I'll investigate further. Next I'll also test in some additional web browsers and add the trac-disable-on-submit class where necessary in Trac.

comment:12 Changed 5 years ago by Remy Blank

This disables the whole form when clicking any button in the form. This is more than necessary. Would it be possible to tag only specific buttons (e.g. on the ticket page only the "Submit changes" button), so that it's still possible to double-click others (e.g. "Preview")?

comment:6 mentions that simply disabling the button doesn't work with IE, but maybe there are other solutions?

  • Instead of setting the disabled attribute on the button, have its click() handler stop propagation of the event on the second click?
  • Or add a click() handler on the button that sets a class on the form, and have the form submit() handler disable the form if the class is present?

comment:13 Changed 5 years ago by anonymous

Possibly the Post/Redirect/Get pattern could be used to solve this?

comment:14 in reply to:  12 ; Changed 5 years ago by Ryan J Ollos

Replying to rblank:

This disables the whole form when clicking any button in the form.

[be27039c/rjollos.git] aims to be more specific and only disable the submit action on buttons to which the class is applied. It is implemented for buttons on the New Ticket and Admin Milestone pages for which clicking is not idempotent. I've testing in Firefox 26 on Ubuntu 13.04 and IE 11 on Windows 7 so far.

  • Instead of setting the disabled attribute on the button, have its click() handler stop propagation of the event on the second click?
  • Or add a click() handler on the button that sets a class on the form, and have the form submit() handler disable the form if the class is present?

The second suggestion is what I've attempted to implement in the revised changes.

comment:15 in reply to:  13 Changed 5 years ago by Ryan J Ollos

Replying to anonymous:

Possibly the Post/Redirect/Get pattern could be used to solve this?

I'm not sure, but that looks like quite bit of extra complexity for our case in which a duplicate submit is not all that harmful in any cases that I'm aware of.

comment:16 in reply to:  13 Changed 5 years ago by Remy Blank

Replying to anonymous:

Possibly the Post/Redirect/Get pattern could be used to solve this?

We already do that for all mutating operations, but it's the solution to a different problem (avoiding a second POST on reload). In this case, double-clicking does the second POST before the response to the first one (the redirect) is received, so it doesn't help.

comment:17 in reply to:  14 ; Changed 5 years ago by Remy Blank

Replying to rjollos:

[be27039c/rjollos.git] aims to be more specific and only disable the submit action on buttons to which the class is applied. It is implemented for buttons on the New Ticket and Admin Milestone pages for which clicking is not idempotent. I've testing in Firefox 26 on Ubuntu 13.04 and IE 11 on Windows 7 so far.

Looks nice.

  • It would be nice to add comments about the operations performed on unload (basically, what you mention in comment:11).
  • Nit: format the else as } else {.
  • Technically, you wouldn't need to protect the milestone "Save" and "Remove" buttons, as they should be idempotent (save definitely is, remove may return an error on the second click).

comment:18 Changed 5 years ago by Jun Omae

I think we should also prevent doubleclick in attach-file-form.

comment:19 in reply to:  17 Changed 5 years ago by Ryan J Ollos

Replying to rblank:

  • Technically, you wouldn't need to protect the milestone "Save" and "Remove" buttons, as they should be idempotent (save definitely is, remove may return an error on the second click).

With the Save button and a rename of the resource (component6 - > component7) or with the Remove button, I found that a double-click can result in:

Trac Error

Component component6 does not exist.

TracGuide — The Trac User and Administration Guide


This is with TracStandalone running locally, and I have to double or triple click the buttons pretty fast.

comment:20 Changed 5 years ago by Remy Blank

Ok, the operations are idempotent but an error isn't nice, so let's protect these, too.

comment:21 Changed 5 years ago by Ryan J Ollos

In [0d7c6f7db/rjollos.git] I've handled additional buttons for which the action is not idempotent, or an error results.

On the batch modification form, the comment operation is not idempotent (however, I didn't find any errors due to multi-submit of the other batch modification operations). Since there is another submit handler for the batch modification form, there is a problem on 1.0-stable - the button will be disabled until the page is refreshed if submit is pressed with no items in the table selected: branches/1.0-stable/trac/htdocs/js/query.js@12332:411,422-423#L410. I've put a fix in place [cd20d667/rjollos.git]. This fix isn't ideal, but also isn't necessary on the trunk after [12374#file2], since the submit button is disabled until an item from the table is selected.

comment:22 in reply to:  21 Changed 5 years ago by Ryan J Ollos

Replying to rjollos:

This fix isn't ideal, but also isn't necessary on the trunk after [12374#file2], since the submit button is disabled until an item from the table is selected.

Actually that is not entirely true yet, but will be after a follow-up change that I'll propose in #11056 - the remaining validation will be moved out of the submit handler and the code for handling radio buttons will be made consistent with the code for handling the checkboxes.

comment:23 Changed 5 years ago by Jun Omae

I've tried log:rjollos.git:t10138 with IE 6, IE 8, Firefox 26 and Chrome 33 beta. That works well except one thing.

I think the Add milestone button should be protected. The button will be shonw if visiting not-existent milestone page, e.g. milestone/notfound.

  • trac/ticket/templates/milestone_edit.html

    diff --git a/trac/ticket/templates/milestone_edit.html b/trac/ticket/templates/milestone_edit.html
    index 3a56aac..de378fa 100644
    a b ${milestone.description}</textarea></p>  
    113113          <input py:when="True" type="submit" name="save"
    114114                 value="${_('Submit changes')}" class="trac-disable-on-submit" />
    115115          <input py:otherwise="" type="submit" name="add"
    116                  value="${_('Add milestone')}" />
     116                 value="${_('Add milestone')}" class="trac-disable-on-submit" />
    117117          <input type="submit" name="cancel" value="${_('Cancel')}" />
    118118        </div>
    119119      </form>

comment:24 in reply to:  23 ; Changed 5 years ago by Ryan J Ollos

Replying to jomae:

I think the Add milestone button should be protected. The button will be shonw if visiting not-existent milestone page, e.g. milestone/notfound.

I haven't been able to reproduce any issues due to multi-submit of this form. What issue were you seeing?

comment:25 in reply to:  24 Changed 5 years ago by Jun Omae

Replying to rjollos:

Replying to jomae:

I think the Add milestone button should be protected. The button will be shonw if visiting not-existent milestone page, e.g. milestone/notfound.

I haven't been able to reproduce any issues due to multi-submit of this form. What issue were you seeing?

An IntegrityError rarely happens if the same milestone is concurrently added. But that is another issue, sorry. The button doesn't need the protection for mulit-submit.

2014-01-19 09:45:55,099 Trac[main] ERROR: Internal Server Error:
Traceback (most recent call last):
  File "/home/jun66j5/src/trac/edgewall/git/trac/web/main.py", line 504, in _dispatch_request
    dispatcher.dispatch(req)
  File "/home/jun66j5/src/trac/edgewall/git/trac/web/main.py", line 221, in dispatch
    resp = chosen_handler.process_request(req)
  File "/home/jun66j5/src/trac/edgewall/git/trac/ticket/roadmap.py", line 681, in process_request
    return self._do_save(req, milestone)
  File "/home/jun66j5/src/trac/edgewall/git/trac/ticket/roadmap.py", line 815, in _do_save
    milestone.insert()
  File "/home/jun66j5/src/trac/edgewall/git/trac/ticket/model.py", line 1042, in insert
    to_utimestamp(self.completed), self.description))
  File "/home/jun66j5/src/trac/edgewall/git/trac/db/util.py", line 121, in execute
    cursor.execute(query, params)
  File "/home/jun66j5/src/trac/edgewall/git/trac/db/util.py", line 54, in execute
    r = self.cursor.execute(sql_escape_percent(sql), args)
IntegrityError: duplicate key violates unique constraint "milestone_pkey"

comment:26 Changed 5 years ago by Ryan J Ollos

Okay, thanks for testing!

comment:27 Changed 5 years ago by Ryan J Ollos

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

Committed to 1.0-stable in [12458] and merged to trunk in [12459].

comment:28 Changed 4 years ago by Ryan J Ollos

API Changes: modified (diff)

comment:29 Changed 4 years ago by Ryan J Ollos

DynamicFieldsPlugin prevents form submission on validation failure using a handler for the submit event. After form submission is prevented by DynamicFieldsPlugin, it won't be possible to submit the form again due to the presence of the trac-submit-is-disabled class on the form. The issue was reported in th:#12258, and was fixed in [th 14516]. However, I'll continue to consider whether there might be a better way solution.

Last edited 4 years ago by Jun Omae (previous) (diff)

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Ryan J Ollos.
The resolution will be deleted.
to The owner will be changed from Ryan J Ollos 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.