Skip to content
This repository

Asynchronous call to confirm (actual and tests passed ok) #196

Closed
wants to merge 7 commits into from

10 participants

Akzhan Abdulin Steve Schwartz Nicholas Firth-McCoy Sergey Nartimov symve Matias Korhonen Neeraj Singh Zachary Scott Andrew Coleman xhh
Akzhan Abdulin

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 Abdulin

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.

Steve Schwartz
Collaborator

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 Abdulin

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

Steve Schwartz
Collaborator

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 Abdulin

Wow! I did it.

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

Akzhan Abdulin

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.
Steve Schwartz
Collaborator

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 Abdulin

Pull request slightly updated to be more accurate.

Steve Schwartz
Collaborator

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 Abdulin

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 Abdulin

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 Abdulin

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

This is less painful.

Steve Schwartz
Collaborator

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

Akzhan Abdulin

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 Abdulin

@JangoSteve, any comments?

Steve Schwartz
Collaborator

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 Abdulin

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

Steve Schwartz
Collaborator

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.

Steve Schwartz
Collaborator

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.

Steve Schwartz Rewrote portion of rails.confirm deferred calls to eliminate double-t…
…riggering

of directly bound handlers outside of jquery-ujs. See #196.
bffb813
Steve Schwartz
Collaborator

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.

Akzhan Abdulin

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

Steve Schwartz
Collaborator

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 Abdulin

Looks like OK. I vote for.

Akzhan Abdulin

Any chance to see it landed?)

Steve Schwartz
Collaborator

@akzhan, have you been using it in an app at all? Curious to see if any side-effects are seen in production environments. Perhaps I'll start using it also in a couple of my projects, and if it goes a week or so with no side-effects or IE weirdness, I'll pull it in to master.

Akzhan Abdulin

Not yet. Will be updated today.

Some sort of initial async. code was used in dev. env.

Nicholas Firth-McCoy

Thanks for this guys, it's looking great :) I'm working on integrating this into a production app at the moment.

@JangoSteve, there's a tiny bug on line 394 in the $(rails.formInputClickSelector).live('click.rails') event handler function - you call e.preventDefault() instead of event.preventDefault().

In terms of features, I think it would be great to have access to the element as well as the message in $.rails.confirm. That way, you can access other data attributes in your custom modal box (eg cancel-text, confirm-text). This probably has implications for backwards compatibility though.

Nicholas Firth-McCoy

Actually, updating $.rails.allowAction to call $.rails.confirm(message, element); seems to be working without issue for me, whether I override $.rails.confirm or not.

Steve Schwartz
Collaborator

@nfm, yeah you should be able to do that, as JS doesn't enforce the number of arguments when calling functions. If you don't include an argument when calling the function, it's just null.

Akzhan Abdulin

This code already merged into https://github.com/errbit/errbit master branch.

Akzhan Abdulin

@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 Abdulin

Pull request updated by @JangoSteve and @nfm commits.

Steve Schwartz
Collaborator

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

Sergey Nartimov

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

Steve Schwartz
Collaborator

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

Steve Schwartz
Collaborator

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

Nicholas Firth-McCoy

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

Akzhan Abdulin

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

Now it is used in Errbit application.

Akzhan Abdulin

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

Am I right?

Akzhan Abdulin

Bump :)

Nicholas Firth-McCoy

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?

Matias Korhonen

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.

Steve Schwartz
Collaborator

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

Nicholas Firth-McCoy

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

Steve Schwartz
Collaborator

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

Steve Schwartz
Collaborator

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.

Neeraj Singh
Collaborator

@JangoSteve any update on this issue ?

Steve Schwartz
Collaborator

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

Steve Schwartz
Collaborator

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.

Steve Schwartz JangoSteve closed this April 02, 2012
Zachary Scott
zzak commented August 02, 2012

@JangoSteve ping! Any update here?

Andrew Coleman

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.

Cyril Mougel shingara referenced this pull request in errbit/errbit June 09, 2013
Closed

Javascript not works with jquery 1.10 #500

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
208  src/rails.js
@@ -51,8 +51,8 @@
51 51
     // Link elements bound by jquery-ujs
52 52
     linkClickSelector: 'a[data-confirm], a[data-method], a[data-remote]',
53 53
 
54  
-		// Select elements bound by jquery-ujs
55  
-		selectChangeSelector: 'select[data-remote]',
  54
+    // Select elements bound by jquery-ujs
  55
+    selectChangeSelector: 'select[data-remote]',
56 56
 
57 57
     // Form elements bound by jquery-ujs
58 58
     formSubmitSelector: 'form',
@@ -85,9 +85,22 @@
85 85
       return event.result !== false;
86 86
     },
87 87
 
  88
+    resolveOrReject: function(deferred, resolved) {
  89
+      if (resolved) {
  90
+        deferred.resolve();
  91
+      } else {
  92
+        deferred.reject();
  93
+      }
  94
+      return deferred;
  95
+    },
  96
+
88 97
     // Default confirm dialog, may be overridden with custom confirm dialog in $.rails.confirm
89 98
     confirm: function(message) {
90  
-      return confirm(message);
  99
+      var res = confirm(message),
  100
+          answer = $.Deferred();
  101
+
  102
+      rails.resolveOrReject(answer, res);
  103
+      return answer.promise();
91 104
     },
92 105
 
93 106
     // Default ajax function, may be overridden with custom function in $.rails.ajax
@@ -97,10 +110,10 @@
97 110
 
98 111
     // Submits "remote" forms and links with ajax
99 112
     handleRemote: function(element) {
100  
-      var method, url, data,
101  
-        crossDomain = element.data('cross-domain') || null,
102  
-        dataType = element.data('type') || ($.ajaxSettings && $.ajaxSettings.dataType),
103  
-        options;
  113
+      var method, url, data, button,
  114
+          crossDomain = element.data('cross-domain') || null,
  115
+          dataType = element.data('type') || ($.ajaxSettings && $.ajaxSettings.dataType),
  116
+          options;
104 117
 
105 118
       if (rails.fire(element, 'ajax:before')) {
106 119
 
@@ -109,7 +122,7 @@
109 122
           url = element.attr('action');
110 123
           data = element.serializeArray();
111 124
           // memoized value from clicked submit button
112  
-          var button = element.data('ujs:submit-button');
  125
+          button = element.data('ujs:submit-button');
113 126
           if (button) {
114 127
             data.push(button);
115 128
             element.data('ujs:submit-button', null);
@@ -120,9 +133,9 @@
120 133
           data = element.serialize();
121 134
           if (element.data('params')) data = data + "&" + element.data('params'); 
122 135
         } else {
123  
-           method = element.data('method');
124  
-           url = element.attr('href');
125  
-           data = element.data('params') || null; 
  136
+          method = element.data('method');
  137
+          url = element.attr('href');
  138
+          data = element.data('params') || null; 
126 139
         }
127 140
 
128 141
         options = {
@@ -155,14 +168,19 @@
155 168
     // <a href="/users/5" data-method="delete" rel="nofollow" data-confirm="Are you sure?">Delete</a>
156 169
     handleMethod: function(link) {
157 170
       var href = link.attr('href'),
158  
-        method = link.data('method'),
159  
-        csrf_token = $('meta[name=csrf-token]').attr('content'),
160  
-        csrf_param = $('meta[name=csrf-param]').attr('content'),
161  
-        form = $('<form method="post" action="' + href + '"></form>'),
162  
-        metadata_input = '<input name="_method" value="' + method + '" type="hidden" />';
163  
-
164  
-      if (csrf_param !== undefined && csrf_token !== undefined) {
165  
-        metadata_input += '<input name="' + csrf_param + '" value="' + csrf_token + '" type="hidden" />';
  171
+          method = link.data('method') || 'GET',
  172
+          csrf_token = $('meta[name=csrf-token]').attr('content'),
  173
+          csrf_param = $('meta[name=csrf-param]').attr('content'),
  174
+          form = $('<form></form>', { action: href, method: method, 'data-ujs-generated': 'true' }),
  175
+          metadata_input = '';
  176
+
  177
+      if (method !== 'GET') {
  178
+        form.attr('method', 'POST');
  179
+        metadata_input += '<input name="_method" value="' + method + '" type="hidden" />';
  180
+
  181
+        if (csrf_param !== undefined && csrf_token !== undefined) {
  182
+          metadata_input += '<input name="' + csrf_param + '" value="' + csrf_token + '" type="hidden" />';
  183
+        }
166 184
       }
167 185
 
168 186
       form.hide().append(metadata_input).appendTo('body');
@@ -176,7 +194,9 @@
176 194
     */
177 195
     disableFormElements: function(form) {
178 196
       form.find(rails.disableSelector).each(function() {
179  
-        var element = $(this), method = element.is('button') ? 'html' : 'val';
  197
+        var element = $(this),
  198
+            method = element.is('button') ? 'html' : 'val';
  199
+
180 200
         element.data('ujs:enable-with', element[method]());
181 201
         element[method](element.data('disable-with'));
182 202
         element.attr('disabled', 'disabled');
@@ -189,7 +209,9 @@
189 209
     */
190 210
     enableFormElements: function(form) {
191 211
       form.find(rails.enableSelector).each(function() {
192  
-        var element = $(this), method = element.is('button') ? 'html' : 'val';
  212
+        var element = $(this),
  213
+            method = element.is('button') ? 'html' : 'val';
  214
+
193 215
         if (element.data('ujs:enable-with')) element[method](element.data('ujs:enable-with'));
194 216
         element.removeAttr('disabled');
195 217
       });
@@ -207,20 +229,36 @@
207 229
    */
208 230
     allowAction: function(element) {
209 231
       var message = element.data('confirm'),
210  
-          answer = false, callback;
211  
-      if (!message) { return true; }
  232
+          confirmAnswer,
  233
+          answer = $.Deferred();
  234
+
  235
+      if (!message) { return $.when(true); }
212 236
 
213 237
       if (rails.fire(element, 'confirm')) {
214  
-        answer = rails.confirm(message);
215  
-        callback = rails.fire(element, 'confirm:complete', [answer]);
  238
+        confirmAnswer = rails.confirm(message);
  239
+        confirmAnswer.then(
  240
+          function() {
  241
+            var callbackOk = rails.fire(element, 'confirm:complete', [ true ]);
  242
+            rails.resolveOrReject(answer, callbackOk);
  243
+          },
  244
+          function() {
  245
+            rails.fire(element, 'confirm:complete', [ false ]);
  246
+            answer.reject();
  247
+          }
  248
+        );
  249
+        return answer.promise();
  250
+      // If `confirm` event handler returned false...
  251
+      } else {
  252
+        answer.reject();
  253
+        return answer.promise();
216 254
       }
217  
-      return answer && callback;
218 255
     },
219 256
 
220 257
     // Helper function which checks for blank inputs in a form that match the specified CSS selector
221 258
     blankInputs: function(form, specifiedSelector, nonBlank) {
222 259
       var inputs = $(), input,
223  
-        selector = specifiedSelector || 'input,textarea';
  260
+          selector = specifiedSelector || 'input,textarea';
  261
+
224 262
       form.find(selector).each(function() {
225 263
         input = $(this);
226 264
         // Collect non-blank inputs if nonBlank option is true, otherwise, collect blank inputs
@@ -247,6 +285,7 @@
247 285
     // manually invoke them. If anyone returns false then stop the loop
248 286
     callFormSubmitBindings: function(form) {
249 287
       var events = form.data('events'), continuePropagation = true;
  288
+
250 289
       if (events !== undefined && events['submit'] !== undefined) {
251 290
         $.each(events['submit'], function(i, obj){
252 291
           if (typeof obj.handler === 'function') return continuePropagation = obj.handler(obj.data);
@@ -260,65 +299,99 @@
260 299
 
261 300
   $(rails.linkClickSelector).live('click.rails', function(e) {
262 301
     var link = $(this);
263  
-    if (!rails.allowAction(link)) return rails.stopEverything(e);
264 302
 
265  
-    if (link.data('remote') !== undefined) {
266  
-      rails.handleRemote(link);
267  
-      return false;
268  
-    } else if (link.data('method')) {
269  
-      rails.handleMethod(link);
270  
-      return false;
271  
-    }
  303
+    rails.allowAction(link).then(
  304
+      function() {
  305
+        if (link.data('remote') !== undefined) {
  306
+          rails.handleRemote(link);
  307
+        } else {
  308
+          rails.handleMethod(link);
  309
+        }
  310
+      },
  311
+      function() {
  312
+        rails.stopEverything(e);
  313
+      }
  314
+    );
  315
+
  316
+    e.preventDefault();
272 317
   });
273 318
 
274  
-	$(rails.selectChangeSelector).live('change.rails', function(e) {
  319
+  $(rails.selectChangeSelector).live('change.rails', function(e) {
275 320
     var link = $(this);
276  
-    if (!rails.allowAction(link)) return rails.stopEverything(e);
277 321
 
278  
-    rails.handleRemote(link);
279  
-    return false;
280  
-  });	
  322
+    rails.allowAction(link).then(
  323
+      function() {
  324
+        rails.handleRemote(link);
  325
+      },
  326
+      function() {
  327
+        rails.stopEverything(e);
  328
+      }
  329
+    );
  330
+
  331
+    e.preventDefault();
  332
+  });
281 333
 
282 334
   $(rails.formSubmitSelector).live('submit.rails', function(e) {
283 335
     var form = $(this),
284  
-      remote = form.data('remote') !== undefined,
285  
-      blankRequiredInputs = rails.blankInputs(form, rails.requiredInputSelector),
286  
-      nonBlankFileInputs = rails.nonBlankInputs(form, rails.fileInputSelector);
  336
+        remote = (form.data('remote') !== undefined),
  337
+        blankRequiredInputs = rails.blankInputs(form, rails.requiredInputSelector),
  338
+        nonBlankFileInputs = rails.nonBlankInputs(form, rails.fileInputSelector);
  339
+
  340
+    rails.allowAction(form).then(
  341
+      function() {
  342
+        // skip other logic when required values are missing or file upload is present
  343
+        if (blankRequiredInputs && form.attr("novalidate") == undefined && rails.fire(form, 'ajax:aborted:required', [blankRequiredInputs])) {
  344
+          return rails.stopEverything(e);
  345
+        }
287 346
 
288  
-    if (!rails.allowAction(form)) return rails.stopEverything(e);
  347
+        if (remote) {
  348
+          if (nonBlankFileInputs) {
  349
+            return rails.fire(form, 'ajax:aborted:file', [nonBlankFileInputs]);
  350
+          }
289 351
 
290  
-    // skip other logic when required values are missing or file upload is present
291  
-    if (blankRequiredInputs && form.attr("novalidate") == undefined && rails.fire(form, 'ajax:aborted:required', [blankRequiredInputs])) {
292  
-      return rails.stopEverything(e);
293  
-    }
  352
+          // If browser does not support submit bubbling, then this live-binding will be called before direct
  353
+          // bindings. Therefore, we should directly call any direct bindings before remotely submitting form.
  354
+          if (!$.support.submitBubbles && rails.callFormSubmitBindings(form) === false) return rails.stopEverything(e);
294 355
 
295  
-    if (remote) {
296  
-      if (nonBlankFileInputs) {
297  
-        return rails.fire(form, 'ajax:aborted:file', [nonBlankFileInputs]);
  356
+          rails.handleRemote(form);
  357
+        } else {
  358
+          // slight timeout so that the submit button gets properly serialized
  359
+          setTimeout(function() {
  360
+            rails.disableFormElements(form);
  361
+            // Submit the form from dom-level js (i.e. *not* via jquery),
  362
+            // which will skip all submit bindings (including this live-binding),
  363
+            // since they have already been called.
  364
+            form.get(0).submit();
  365
+          }, 13);
  366
+        }
  367
+      },
  368
+      function() {
  369
+        rails.stopEverything(e);
298 370
       }
  371
+    );
299 372
 
300  
-      // If browser does not support submit bubbling, then this live-binding will be called before direct
301  
-      // bindings. Therefore, we should directly call any direct bindings before remotely submitting form.
302  
-      if (!$.support.submitBubbles && rails.callFormSubmitBindings(form) === false) return rails.stopEverything(e);
303  
-
304  
-      rails.handleRemote(form);
305  
-      return false;
306  
-    } else {
307  
-      // slight timeout so that the submit button gets properly serialized
308  
-      setTimeout(function(){ rails.disableFormElements(form); }, 13);
309  
-    }
  373
+    e.preventDefault();
310 374
   });
311 375
 
312 376
   $(rails.formInputClickSelector).live('click.rails', function(event) {
313 377
     var button = $(this);
314 378
 
315  
-    if (!rails.allowAction(button)) return rails.stopEverything(event);
316  
-
317  
-    // register the pressed submit button
318  
-    var name = button.attr('name'),
319  
-      data = name ? {name:name, value:button.val()} : null;
  379
+    rails.allowAction(button).then(
  380
+      function() {
  381
+        // register the pressed submit button
  382
+        var name = button.attr('name'), form,
  383
+          data = name ? {name:name, value:button.val()} : null;
  384
+
  385
+        form = button.closest('form');
  386
+        form.data('ujs:submit-button', data);
  387
+        form.submit();
  388
+      },
  389
+      function() {
  390
+        rails.stopEverything(event);
  391
+      }
  392
+    );
320 393
 
321  
-    button.closest('form').data('ujs:submit-button', data);
  394
+    event.preventDefault();
322 395
   });
323 396
 
324 397
   $(rails.formSubmitSelector).live('ajax:beforeSend.rails', function(event) {
@@ -330,3 +403,4 @@
330 403
   });
331 404
 
332 405
 })( jQuery );
  406
+
49  test/public/test/data-confirm.js
@@ -5,12 +5,19 @@ module('data-confirm', {
5 5
       'data-remote': 'true',
6 6
       'data-confirm': 'Are you absolutely sure?',
7 7
       text: 'my social security number'
  8
+    }))
  9
+    .append($('<form />', {
  10
+      action: '/echo',
  11
+      'data-confirm': 'true',
  12
+      method: 'post'
8 13
     }));
9 14
 
10 15
     this.windowConfirm = window.confirm;
11 16
   },
12 17
   teardown: function() {
13 18
     window.confirm = this.windowConfirm;
  19
+
  20
+    $('form[data-ujs-generated]').remove();
14 21
   }
15 22
 });
16 23
 
@@ -99,3 +106,45 @@ asyncTest('binding to confirm:complete event and returning false', 2, function()
99 106
     start();
100 107
   }, 50);
101 108
 });
  109
+
  110
+asyncTest('clicking non-ajax link with data-confirm', 2, function() {
  111
+  window.confirm = function(msg) {
  112
+    ok(true, 'confirm dialog should be called');
  113
+    return true;
  114
+  };
  115
+
  116
+  $('a[data-confirm]')
  117
+    .removeAttr('data-remote')
  118
+    .bind('click', function() {
  119
+      ok(true, 'direct click binding should only be called once');
  120
+    })
  121
+    .bind('ajax:beforeSend', function() {
  122
+      ok(false, 'ajax should not be called');
  123
+    })
  124
+    .trigger('click');
  125
+
  126
+  setTimeout(function() {
  127
+    start();
  128
+  }, 10);
  129
+});
  130
+
  131
+asyncTest('submitting non-ajax form with data-confirm', 3, function() {
  132
+  window.confirm = function(msg) {
  133
+    ok(true, 'confirm dialog should be called');
  134
+    return true;
  135
+  };
  136
+
  137
+  $('form[data-confirm]')
  138
+    .bind('submit', function() {
  139
+      ok(true, 'direct submit binding should only be called once');
  140
+    })
  141
+    .bind('ajax:beforeSend', function() {
  142
+      ok(false, 'ajax should not be called');
  143
+    })
  144
+    .bind('iframe:loaded', function() {
  145
+      ok(true, 'should submit via non-ajax');
  146
+      start();
  147
+    })
  148
+    .trigger('submit');
  149
+
  150
+});
8  test/public/test/data-method.js
... ...
@@ -1,6 +1,10 @@
1 1
 (function(){
2 2
 
3  
-module('data-method');
  3
+module('data-method', {
  4
+  teardown: function() {
  5
+    $('form[data-ujs-generated]').remove();
  6
+  }
  7
+});
4 8
 
5 9
 function submit(fn) {
6 10
   $('#qunit-fixture').
@@ -31,4 +35,4 @@ asyncTest('link with "data-method" and CSRF', 1, function() {
31 35
   });
32 36
 });
33 37
 
34  
-})();
  38
+})();
51  test/public/test/settings.js
@@ -22,15 +22,42 @@ App.assert_request_path = function(request_env, path) {
22 22
 
23 23
 // hijacks normal form submit; lets it submit to an iframe to prevent
24 24
 // navigating away from the test suite
25  
-$(document).bind('submit', function(e) {
26  
-  if (!e.isDefaultPrevented()) {
27  
-    var form = $(e.target), action = form.attr('action'),
28  
-        name = 'form-frame' + jQuery.guid++,
29  
-        iframe = $('<iframe name="' + name + '" />');
30  
-
31  
-    if (action.indexOf('iframe') < 0) form.attr('action', action + '?iframe=true')
32  
-    form.attr('target', name);
33  
-    $('#qunit-fixture').append(iframe);
34  
-    $.event.trigger('iframe:loading', form);
35  
-  }
36  
-});
  25
+//$(document).bind('submit', function(e) {
  26
+//  if (!e.isDefaultPrevented()) {
  27
+//    var form = $(e.target), action = form.attr('action'),
  28
+//        name = 'form-frame' + jQuery.guid++,
  29
+//        iframe = $('<iframe name="' + name + '" />');
  30
+//
  31
+//    if (action.indexOf('iframe') < 0) form.attr('action', action + '?iframe=true')
  32
+//    form.attr('target', name);
  33
+//    $('#qunit-fixture').append(iframe);
  34
+//    $.event.trigger('iframe:loading', form);
  35
+//  }
  36
+//});
  37
+
  38
+// The above doesn't work, since rails.js now calls the dom-level form.submit() function
  39
+// for normal forms after all bindings have been run. The below more directly accomplishes
  40
+// its intentions anyway, for the purposes of making sure the test suite never navigates
  41
+// to a new page for non-ajax submitted forms.
  42
+// See for explanation: http://diveintogreasemonkey.org/patterns/override-method.html
  43
+var iframeSubmit = function(event) {
  44
+  var target = event ? event.target : this,
  45
+      form = $(target), action = form.attr('action'),
  46
+      name = 'form-frame' + jQuery.guid++,
  47
+      iframe = $('<iframe name="' + name + '" />');
  48
+
  49
+  if (action.indexOf('iframe') < 0) form.attr('action', action + '?iframe=true')
  50
+  form.attr('target', name);
  51
+  $('#qunit-fixture').append(iframe);
  52
+  $.event.trigger('iframe:loading', form);
  53
+
  54
+  this._submit();
  55
+}
  56
+
  57
+// capture the onsubmit event on all forms
  58
+window.addEventListener('submit', iframeSubmit, true);
  59
+
  60
+// If a script calls someForm.submit(), the onsubmit event does not fire,
  61
+// so we need to redefine the submit method of the HTMLFormElement class.
  62
+HTMLFormElement.prototype._submit = HTMLFormElement.prototype.submit;
  63
+HTMLFormElement.prototype.submit = iframeSubmit;
Commit_comment_tip

Tip: You can add notes to lines in a file. Hover to the left of a line to make a note

Something went wrong with that request. Please try again.