Moodle
  1. Moodle
  2. MDL-34221

disabledIf doesn't work on drag-and-drop filepicker form elements

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3.1
    • Fix Version/s: 2.3.2
    • Component/s: Filepicker
    • Labels:
    • Testing Instructions:
      Hide

      1. Write a Moodle form which contains a form element of type 'filepicker' AND the form element of type 'filemanager'
      2. Add a form element of type 'select' to the form, with options 'yes' and 'no'.
      3. Use $mform->disabledIf() to make it so that when the 'select' element is set to 'no', the filepicker should be disabled.
      4. Print the form in a standard Moodle page.
      5. Set the 'select' element to 'no'

      Expected outcome: The filepicker/filemanager will become disabled, user is unable to add files either by drag-and-drop or by using filepicker. User also can not see the list of files inside the elements (if there were any)

      Show
      1. Write a Moodle form which contains a form element of type 'filepicker' AND the form element of type 'filemanager' 2. Add a form element of type 'select' to the form, with options 'yes' and 'no'. 3. Use $mform->disabledIf() to make it so that when the 'select' element is set to 'no', the filepicker should be disabled. 4. Print the form in a standard Moodle page. 5. Set the 'select' element to 'no' Expected outcome: The filepicker/filemanager will become disabled, user is unable to add files either by drag-and-drop or by using filepicker. User also can not see the list of files inside the elements (if there were any)
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull Master Branch:
      wip-MDL-34221-master

      Description

      The new drag-and-drop filepickers don't get disabled by the Javascript generated by $mform->disabledIf(). The "Choose a file..." button gets disabled, but the drag-and-drop box with the arrow and the text "You can drag and drop files here to add them" does not disable.

        Gliffy Diagrams

          Activity

          Hide
          Aaron Wells added a comment -

          This is quite similar to MDL-26167, which popped up when the Moodle 2 filepickers were first added. Here's a test script to demonstrate the issue:

          <?php
           
          require_once('config.php');
          require_once($CFG->dirroot.'/lib/formslib.php');
           
          class testform extends moodleform {
              function definition() {
                  global $CFG;
           
                  $mform =& $this->_form;
           
                  $mform->addElement('select','enabler','Enable other form elements?', array(0=>'No',1=>'Yes'));
           
                  // A text field will enable/disable properly
                  $mform->addElement('text','foo','This is a text field');
                  $mform->disabledIf('foo','enabler','neq',1);
           
                  // A filepicker's drag-and-drop area will remain enabled no matter what
                  $filemanager_options = array();
                  $filemanager_options['return_types'] = 3;
                  $filemanager_options['accepted_types'] = 'web_image';
                  $filemanager_options['maxbytes'] = get_max_upload_file_size($CFG->maxbytes);
                  $mform->addElement('filepicker', 'bar', 'This is a filepicker', null, $filemanager_options);
                  $mform->disabledIf('bar','enabler','neq',1);
              }
          }
          require_login();
          echo $OUTPUT->header();
          echo $OUTPUT->heading('Test');
          $form = new testform();
           
          $form->display();
           
          echo $OUTPUT->footer();

          Show
          Aaron Wells added a comment - This is quite similar to MDL-26167 , which popped up when the Moodle 2 filepickers were first added. Here's a test script to demonstrate the issue: <?php   require_once('config.php'); require_once($CFG->dirroot.'/lib/formslib.php');   class testform extends moodleform { function definition() { global $CFG;   $mform =& $this->_form;   $mform->addElement('select','enabler','Enable other form elements?', array(0=>'No',1=>'Yes'));   // A text field will enable/disable properly $mform->addElement('text','foo','This is a text field'); $mform->disabledIf('foo','enabler','neq',1);   // A filepicker's drag-and-drop area will remain enabled no matter what $filemanager_options = array(); $filemanager_options['return_types'] = 3; $filemanager_options['accepted_types'] = 'web_image'; $filemanager_options['maxbytes'] = get_max_upload_file_size($CFG->maxbytes); $mform->addElement('filepicker', 'bar', 'This is a filepicker', null, $filemanager_options); $mform->disabledIf('bar','enabler','neq',1); } } require_login(); echo $OUTPUT->header(); echo $OUTPUT->heading('Test'); $form = new testform();   $form->display();   echo $OUTPUT->footer();
          Hide
          Dan Marsden added a comment -

          you can probably also test this on the add scorm page - when other scorm types are enabled in admin settings.

          Show
          Dan Marsden added a comment - you can probably also test this on the add scorm page - when other scorm types are enabled in admin settings.
          Hide
          Marina Glancy added a comment -

          Davo, can you please look at this issue

          Show
          Marina Glancy added a comment - Davo, can you please look at this issue
          Hide
          Davo Smith added a comment -

          I'll take a look this evening (UK time)

          Show
          Davo Smith added a comment - I'll take a look this evening (UK time)
          Hide
          Davo Smith added a comment -

          The styling of the disabled filepicker could possibly do with a bit of tweaking, but the code works (in the tests I've completed).

          Show
          Davo Smith added a comment - The styling of the disabled filepicker could possibly do with a bit of tweaking, but the code works (in the tests I've completed).
          Hide
          Dan Poltawski added a comment -

          Marina are you able to review this?

          Show
          Dan Poltawski added a comment - Marina are you able to review this?
          Hide
          Marina Glancy added a comment -

          this looks pretty good. But I would go further and do the same for filemanager form element as well.
          This is my additional changes:
          https://github.com/marinaglancy/moodle/compare/master...wip-MDL-34221-master

          Show
          Marina Glancy added a comment - this looks pretty good. But I would go further and do the same for filemanager form element as well. This is my additional changes: https://github.com/marinaglancy/moodle/compare/master...wip-MDL-34221-master
          Hide
          Davo Smith added a comment -

          Those additional changes look good to me (from reading the code, I haven't pulled and tested) - do you want to update the repo / branch in the issue and then submit for integration?

          Show
          Davo Smith added a comment - Those additional changes look good to me (from reading the code, I haven't pulled and tested) - do you want to update the repo / branch in the issue and then submit for integration?
          Hide
          Dan Poltawski added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Dan Poltawski added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Sam Hemelryk added a comment -

          Thanks guys, this has been integrated now

          Show
          Sam Hemelryk added a comment - Thanks guys, this has been integrated now
          Hide
          Frédéric Massart added a comment -

          Test successful on 2.3 and master. Cheers!

          Show
          Frédéric Massart added a comment - Test successful on 2.3 and master. Cheers!
          Hide
          Aparup Banerjee added a comment -

          yay, it works!

          This issue has been put through rigorous processes and finally swam upstream along with some 65 others this week.

          Thank you all for taking the time to get us here.

          cheers!

          Show
          Aparup Banerjee added a comment - yay, it works! This issue has been put through rigorous processes and finally swam upstream along with some 65 others this week. Thank you all for taking the time to get us here. cheers!

            People

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

              Dates

              • Created:
                Updated:
                Resolved: