Moodle
  1. Moodle
  2. MDL-30660

M.util.show_confirm_dialog should include value of the button clicked, and should work on form submits

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.0.6, 2.1.3, 2.2, 2.3
    • Fix Version/s: 2.0.7, 2.1.4, 2.2.1
    • Component/s: Libraries
    • Labels:
      None
    • Testing Instructions:
      Hide

      The MDL-27314 testing instructions can be used to test this.

      To test the onsubmit bit, change line 735 of attemptsreport.php from

      $PAGE->requires->event_handler('#deleteattemptsbutton', 'click', 'M.util.show_confirm_dialog',

      to

      $PAGE->requires->event_handler('#attemptsform', 'submit', 'M.util.show_confirm_dialog',

      Show
      The MDL-27314 testing instructions can be used to test this. To test the onsubmit bit, change line 735 of attemptsreport.php from $PAGE->requires->event_handler('#deleteattemptsbutton', 'click', 'M.util.show_confirm_dialog', to $PAGE->requires->event_handler('#attemptsform', 'submit', 'M.util.show_confirm_dialog',
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE, MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE
    • Pull from Repository:
    • Pull Master Branch:
    • Rank:
      33477

      Description

      This was found while working on MDL-27314.

      Normally, when you click a submit button in a form, the name and value of the submit button that were clicked are submitted as a post variable. M.util.show_confirm_dialog is not sending that information, which sometimes is critical.

      Also, it is not possible to use M.util.show_confirm_dialog as a form event handler.

        Issue Links

          Activity

          Hide
          Eloy Lafuente (stronk7) added a comment -

          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

          Show
          Eloy Lafuente (stronk7) added a comment - 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
          Hide
          Sam Hemelryk added a comment -

          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

          Show
          Sam Hemelryk added a comment - 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
          Hide
          Sam Hemelryk added a comment -

          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!

          Show
          Sam Hemelryk added a comment - 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!
          Hide
          Tim Hunt added a comment -

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

          Show
          Tim Hunt added a comment - Following Jabber chat, I think if you can fix 2, then this can go in.
          Hide
          Sam Hemelryk added a comment -

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

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Ok after discussing in Dev chat I've fixed up the alert message and this has now been integrated! Cheers Sam
          Hide
          Aparup Banerjee added a comment -

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

          Show
          Aparup Banerjee added a comment - MDL-27314 has failed and the same instructions apply here. failing this ("by proxy")
          Hide
          Tim Hunt added a comment -

          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.

          Show
          Tim Hunt added a comment - 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.
          Hide
          Tim Hunt added a comment -

          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.

          Show
          Tim Hunt added a comment - 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.
          Hide
          Tim Hunt added a comment -

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

          Show
          Tim Hunt added a comment - I should add, this fix will also fix the problem with MDL-27314 .
          Hide
          Eloy Lafuente (stronk7) added a comment -

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

          Show
          Eloy Lafuente (stronk7) added a comment - New commit adding tagNmae workaround picked to all 2.X branches & master, thanks!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          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)

          Show
          Eloy Lafuente (stronk7) added a comment - 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 )
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Yes, you did it!

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

          Closing, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Yes, you did it! Now your code is part of the best weeklies released ever, many thanks! Closing, ciao

            People

            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: