Hi Sam, I'm sending this to you for review & integrate.

I think it's ok and the new form handler shouldn't cause any problem... but I've seen other uses of M.util.show_confirm_dialog out there (comments, rubrics, filemanager, ...) and perhaps it needs to be checked that no regression is going to happen with them. And I trust you much more than me on that.

Once done, plz ping me and I'll continue with the new commit to add @ MDL-27314 to re-integrate it.

TIA and ciao

Hi guys,

I've just been looking at this now, by all means the changes look sensible.
A couple of things that I've noted:

1/ When checking input.name and input.value before creating the hidden input it may be wise to check type, I imagine only button and submit types won't provide there value unless triggered so perhaps we check for those. Otherwise if someone triggers on any other kind of input it will be duplicated. Not really a problem just perhaps cleaner.

2/ The alert message when element type isn't supported needs to be updated to include form in the list of supported tags.

3/ Just as an interesting note (and not at all a problem) I theorise, there is one disadvantage to including form in the list of supported tags, if JS developers attach a confirm_dialogue to a form that has elements that do not have default actions onto which the developer has attached actions AND the developer hasn't properly handled propagation this change will cause the dialogue to be shown when event propagation occurs. Same sort of problem we have when attaching events to the body tag, although of course much more narrow scope.

Personally I think we just put this in, I think #2 is thing to change, and I can do that on integration. #1 shouldn't affect any core uses and could be addressed separately.
Everyone happy with that?

Cheers
Sam

Just as another interesting note that issue about problems with form's containing inputs named id is a reoccurring problem with YUI.
Certainly an annoyance and worth reporting!

Following Jabber chat, I think if you can fix 2, then this can go in.

Ok after discussing in Dev chat I've fixed up the alert message and this has now been integrated!

Cheers
Sam

MDL-27314 has failed and the same instructions apply here. failing this ("by proxy")

Bum! Lots of different Node methods (including Node.ancestor) use a helper _wrapFn = function(fn) { (defined on line 83 of node-core.js), and this uses the broken Y.Selector.test. Once I get to work, I will do a new commit to work-around that bug in more places in our code.

Eloy, please cherry-pick https://github.com/timhunt/moodle/commit/c6873d185e6a6576f9b7e8f1943ca5704358345a onto all branches 2.0 -> master.

Note that I reviewed the code (I searched for \bY\..*\bform) and could not find any other place that would be broken. Y.all does not seem to be affected.

I should add, this fix will also fix the problem with MDL-27314.

New commit adding tagNmae workaround picked to all 2.X branches & master, thanks!

Done, passing. Tested under 20, 21, 22 & master, the dialog and the cance/ok buttons are working as expected both in the overview and reponses reports. (tested by MDL-27314)

Yes, you did it!

Now your code is part of the best weeklies released ever, many thanks!

Closing, ciao