Asynchronous call to confirm (actual and tests passed ok)

[akzhan]

Although it is possible to override the default confirm method, it doesn't make much sense, because the built-in window.confirm blocks the execution of code and waits for the user input. If we want to replace the default confirm with something else, for example jquery ui dialog or apprise, it would not work, because the modal box method returns immediately, and the result is returned via callback. And there is no way as far as I know to block the execution of script until the user chooses for "yes" or "no". I've made a proof-of-concept for asynchronous confirm, which uses Deferrable (jQuery 1.5 or above).

All tests passed. Based on proof-of-concept - #178.

[akzhan]

Take a note that tests were ran and passed on these browsers:

• Chrome 13 / mac.
• Firefox 6 / mac.
• Safari 5 / mac.

Another note: call-remote: allow empty form "action" have been marked as failed but is OK actually ('?' character added to URL), if tests runned using URL without ?. Fixed by #197.

[JangoSteve]

Woo that's a lot of changes :-) On first glance though, this looks pretty good. I'll have to run the tests also through IE when I get a chance.

Any chance I can get you to squash these commits down into one?

[akzhan]

Ok, will renew as separated pull requiest with one commit.

[JangoSteve]

No, don't do that. Just do a rebase -i and squash all the commits into the first. Rewrite the commit message however you'd like, and then force push git push -f akzhan asynchronous-call-to-confirm. It will automatically update this pull request.

[akzhan]

Wow! I did it.

@JangoSteve, thanks for your advice, I learned new git magic 🆒

[akzhan]

Just ran test suite under:

• Chrome 13/ Windows. All tests passed.
• Internet Explorer 9. All tests passed except call-remote: does not send CSRF token in custom header if crossDomain (1, 0, 1). But this test failed in master branch too.

[JangoSteve]

Yes, that sounds about right. The cross-domain stuff doesn't work well in IE, not much we can do about it. It's basically just there for those who wanna use it for whatever it's worth.

[akzhan]

Pull request slightly updated to be more accurate.

[JangoSteve]

I was going through this code a little more thoroughly and there is one major problem which is preventing me from pulling this in at the moment. Unfortunately, I'm not sure off-hand what a good solution would be.

The problem is with this, this, and this.

I see what is happening is that we are manually stopping all link clicks and form submissions to which we're bound, and then manually re-triggering the events for the elements that should not have been stopped in the first place (being sure to skip our own event handlers the second time around). I understand that this is necessary in order to invoke and "wait" for the asynchronous confirm deferred object. However, this is pretty obtrusive and could break people's applications.

Because all of our functionality is bound to the document node via live-binding (necessary in order to work for elements not yet loaded in the DOM), our bound functions don't fire until the event has propagated all the way up the DOM tree (see the differeince between live, bind, and delegate). By stopping the event unilaterally, and the re-triggering it directly on the element, we'd cause the event to propagate up the DOM tree twice. For example, consider if I had the following code in my application:

// My script
$('#really-important').bind('click', function() { alert("This is a really serious link."); } );
...
// HTML
<a href="/some/path" data-confirm="Do you wanna?" id="really-important">Click me</a>

With the code in this pull request, jquery-ujs would cause the following to happen:

1. User clicks link
2. My alert shows.
3. The "Do you wanna?" confirm dialog shows, and user clicks OK.
4. My alerts shows again. <= OH NOES!
5. Link page loads.

We can easily skip our own event handlers the second time the event is triggered (manually by us). But to require everyone else to do the same in all of their own scripts is too obtrusive.

Like I said before, I'm really not sure what a good solution is. This is precisely the difficulty with using an asynchronous function for functionality that is supposed to be synchronous; this is why this still hasn't been implemented despite multiple requests for it (like this and this).

One possible solution could be to cache the event object (with the clicked/submitted element in the target attribute, but propagated up to the document node), stop the event, and then within the deferred object, manually re-trigger the cached event on the document node with the target element intact, instead of re-triggering it on the original element, so that it doesn't propagate up again. However, this seems kind of hack-ish. There would also most certainly be issues with this solution in IE, considering that IE does some weird things with propagating the form submit event (i.e. triggering the event on the document node would not actually cause the form to submit as it would in firefox/webkit; see this patch for details).

If you have any other ideas, please let me know (it's entirely possible I'm missing something obvious here). This is certainly the most thorough and promising attempt at this so far, and the code looks really good otherwise. I'll leave the pull request open for now.

[akzhan]

You are absolutely right.

So we can add something like "data-async-behavior" attribute and realize asynchronous behaviour as an option.

A lot of applications does nothing except UJS and require custom confirmation dialogs.

[akzhan]

confirm and allowAction can stay Promise'd. We need to add both sync/async variations to event handlers only.

Is it good trade-off?

[akzhan]

Hm. I thinking hard and prefer to maintain both sync. and async. versions of rails.js as separate scripts.

This is less painful.

[JangoSteve]

How would you suggest having them as separate scripts? I.e. how would you structure or maintain such a thing?

[akzhan]

1. update tests to have third selector: [sync | async].
2. create rails-async.js and rails-confirm-ui.js.
3. update readme and reflect limitations of async. version there.
4. follow updates of sync. version.

profit.

[akzhan]

@JangoSteve, any comments?

[JangoSteve]

I like the idea of having this functionality in some way available for people who need it. However, I don't think putting this code in a separate file that has to be separately maintained and updated is the way to go.

I think a better approach may be to keep the synchronous confirm method that pops up for data-confirm, and have a separate asyncComfirm method, using the deferred object, etc in this pull-request, that fires for e.g. data-async-confirm. Then, we can just mention the caveat of data-async-confirm in the wiki.

[akzhan]

Ok, will do until 9 oct. We need this functionality in errbit.

[JangoSteve]

So, a couple things.

1. I was looking at this again, and I'm not sure it would even work as intended.

As described above, we're unilaterally stopping the click event for all links to which we're bound (those being 'a[data-confirm], a[data-method], a[data-remote]'). Then, upon deferred confirm function being resolved (either when the user clicks "ok" or immediately if there was no data-confirm on the link), we either fire the remote handler, the non-remote method handler, or re-trigger the click event if it was neither a data-remote or data-method link (i.e. it only had data-confirm and was otherwise a normal link).

However, firing the click event in jQuery does not actually trigger the native default click event in browsers. This is apparently a security limitation of JS that it doesn't allow "fake clicking" on links (see this stackoverflow answer).

Therefore, re-triggering the click event via JS on a link does not actually cause the browser window to follow the link. This can be illustrated with this:

$('a').live('click', function(e) {
  var link = $(this);
  console.log('called!');
  if (link.data('stop')) { return; }

  link.data('stop', true);
  link.click();
  link.data('stop', false);
  e.preventDefault();
});

Run that on any page and click any link. Notice that in the console, the click event properly gets triggered twice. However, the browser window will never follow the link.

2. All this being said, while I was thinking through this, I may have actually figured out a slightly different way to do this that would avoid both this and the previously-mentioned limitation of double-firing the click event. I'm working on it now and will report back soon.

[JangoSteve]

I was able to get rid of re-triggering of link clicks and form submits for non-remote links and forms.

Links:

Instead of re-triggering the link click for non-remote links (which doesn't work anyway since JS won't allow the window to follow the link when the click event is triggered via JS), I re-used the handleMethod function to create a hidden form, setting method='GET', and submit it. This will make the browser window follow the link's href and won't cause click bindings to be re-triggered.

I think this will be fine, because all of the click bindings have already been triggered and executed by this point, and we don't have to worry about any ajax callbacks, because this is only happening for non-remote links with data-confirm.

Forms:

Instead of re-triggering the form's submit event via jquery (and causing all submit bindings to be re-triggered), I trigger the form's dom-level submit event (i.e. using standard JS and not jquery), which bypasses all the jquery submit bindings the second time around and causes the window to actually submit the form.

This can be illustrated with the following:

$(document).delegate('form', 'submit', function(e) {
  var f = $(e.target)
  console.log('submitted');

  // f.submit();             // will cause both 'submitted' and alert to show twice
  // $(this).trigger(e);     // will cause 'submitted' to show once, but alert will still show twice
  // $(document).trigger(e); // will cause 'submitted' and alert to show once, but form is not submitted
  f.get(0).submit();         // both 'submitted' and alert show only once, form gets submitted
  e.preventDefault();
});

f = $('<form></form>', {action: '', method: 'GET'});
f.hide().bind('submit', function(e) { alert('hi'); }).appendTo('body');
f.submit();

I'm now working through the test suite to see if any side-effects arise from this approach, but I wanted to at least document my progress so far.

[JangoSteve]

OK, so I think I have something working. However, it takes a somewhat creative approach to solving the issues outlined above, so I want to get people's feedback in the form of a sanity check before pulling this into master.

• My branch
• Complete diff between master and the async-confirm branch (including changes from this pull)
• Diff of the changes I made on top of this pull request

[akzhan]

We need additional tests to cover edge cases described by your comment above.

[JangoSteve]

I just created two test cases for making sure direct bindings are only triggered once in the scenarios described above, one for links and one for forms.

[akzhan]

Looks like OK. I vote for.

[akzhan]

Any chance to see it landed?)

[akzhan]

@nfm, thanks for your update. @lest catched that error yesterday: http://screencast.com/t/XrBcu9u8Ak

just sent PRq to errbit (errbit/errbit#126). And it's landed.

[akzhan]

Pull request updated by @JangoSteve and @nfm commits.

[JangoSteve]

So, you guys have been using this branch with some success then?

[lest]

@JangoSteve Yes, it works good.
There is jquery.alerts used in errbit and according to this feature it's integration is possible.

[JangoSteve]

@nfm, can you take the typo commit out of this branch and put it in a separate pull request, I want to go ahead and pull that in with the next release.

[JangoSteve]

@nfm, oops nevermind, I'm dumb. I thought that was a typo in the master branch of jquery-ujs, forgot that that line was added only in this branch.

[nfm]

@JangoSteve all good. I've been using this branch too, working without issue for me.

[akzhan]

@JangoSteve, one issue was found by users and fixed by @nfm. No more issues.

Now it is used in Errbit application.

[akzhan]

@JangoSteve, I think that we can rebase this pull request to meet current master and merge into.

Am I right?

[akzhan]

Bump :)

[nfm]

I've also been using this in production for the last ~3 weeks without issue.

[symve]

Great to see the progress made on this! Looking forward to it being integrated into master.

I'd like to use the async version on the branch: https://github.com/JangoSteve/jquery-ujs/tree/async-confirm
but I need the fix for #222 which the branch version does not contain yet.

Would it be possible to update the branch with the latest code on master so I can test with an "official" version rather than a hand-merged version?

[matiaskorhonen]

Any chance if this making it into master sometime soon?

Currently we're using a horrible, horrible hack to get an async confirmation dialog to work and I'm be quite keen to get rid of it.

[JangoSteve]

@k33l0r Please use the branch mentioned in this thread. I don't don't yet have plans to merge this in the near future due to the lack of people using it, and the fact that it's a large rewrite that depends on some low-level undocumented stuff in jquery. The only thing that would really make it better, would be to have a lot of people using it in production with not problems for a while.

[nfm]

Does anyone have a recent rebase of this branch onto master in their own fork?

[JangoSteve]

Sorry, I'll update my branch shortly and push it up.

[JangoSteve]

Sorry for the delay, that was much more involved than expected. The async-confirm branch of jangosteve/jquery-ujs has now been updated to be on-par with the latest master.

[neerajsingh0101]

@JangoSteve any update on this issue ?

[JangoSteve]

@neerajdotname Yes, if a lot of people start using the fork (which I'll continue to maintain), then there's a chance I'll merge this in. Until then, I'd like to keep this open.

[JangoSteve]

On second thought, the code for this has come a long way since this pull request was created. I'm going to close this pull request and add a page to the wiki for the async-confirm branch. Everything else I said above stands.

[zzak]

@JangoSteve ping! Any update here?

[penguincoder]

Hi guys, i know this issue is closed, but i have a few questions about the code.

First off, this new asynchronous way of handling confirmation messages is pretty nice. I see how you have easily used the $.Deferred() to handle the confirm callbacks and stuff, which fits in nicely with the whole tri-state Promise paradigm.

Today i set out to write my own $.rails.confirm method so i can use the handy-dandy bootstrap modal plugin to confirm if users are sure about the action they were going to perform. It fits better on mobile screens, popup blockers don't hide the box, all that good stuff. Okay.

So, after writing it, adding in my own $.Deferred() stack for handling the clicks on the okay/cancel buttons, i realized there was something missing. The actual bits of the asynchronous stack were not being called to actually do the click when i resolve()d the promise. I discovered that if instead of passing the direct message to $.rails.confirm you passed in the element that originally triggered the event, you could use $.rails.handleMethod(element) to make the thing work after you confirmed.

My question is two part:

1. is this to be expected? (am i doing it right) if it is, you can safely ignore my next question.
2. do you want the changes for this? you have to change one line in allowAction and modify the beginning var statement in confirm

Thanks again. I definitely get how this is a difficult problem to solve. In my specific situation, however, i intend to banish the usage of window.confirm in my application, so async is for the win!

[xhh]

Finally found the 'correct' place to this specific question, but I'm still confused how to implement myself. It is possible that there is a built in option (e.g. merge this pull request)?
Thanks.

[philipgiuliani]

Im also looking forward for this feature in the future! Any chance that someone implements it again? :)