Moodle
  1. Moodle
  2. MDL-39187

Date picker formslib element popup doesn't work when dateselector (a group of elements) is added within another group.

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.5
    • Fix Version/s: 2.5
    • Component/s: Forms Library
    • Labels:
    • Testing Instructions:
      Hide

      Test 1
      Note: Unselecting the checkbox will not disable the calendar image in this situation, as it is not using the optional setting for the date selector element - will be addressed by MDL-39463

      1. Go to Site administration ► Users ► Accounts ► Browse list of users
      2. Click "Show more"
      3. Click on Calender icon and ensure it pops up.
      4. Select a date in the calendar and ensure the pop-up closes and the select boxes update to this date.
      5. Choose a new date in the select boxes and then click on the calendar icon and ensure the pop-up shows this date.
      6. Ensure you can filter by these dates and they work as expected.

      Test 2
      Note: Unselecting the checkbox will not disable the calendar image in this situation, as it is not using the optional setting for the date selector element - will be addressed by MDL-39463

      1. Create a feedback module.
      2. Ensure the date_selectors are working on this page (the date_time_selectors here are in a form group) and that they save appropriately.

      Test 3

      1. Repeat the steps in MDL-26649.
      Show
      Test 1 Note: Unselecting the checkbox will not disable the calendar image in this situation, as it is not using the optional setting for the date selector element - will be addressed by MDL-39463 Go to Site administration ► Users ► Accounts ► Browse list of users Click "Show more" Click on Calender icon and ensure it pops up. Select a date in the calendar and ensure the pop-up closes and the select boxes update to this date. Choose a new date in the select boxes and then click on the calendar icon and ensure the pop-up shows this date. Ensure you can filter by these dates and they work as expected. Test 2 Note: Unselecting the checkbox will not disable the calendar image in this situation, as it is not using the optional setting for the date selector element - will be addressed by MDL-39463 Create a feedback module. Ensure the date_selectors are working on this page (the date_time_selectors here are in a form group) and that they save appropriately. Test 3 Repeat the steps in MDL-26649 .
    • Affected Branches:
      MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull Master Branch:
      MDL-39187_master
    • Rank:
      49790

      Description

      The pop-up for the date picker does not occur when the date_selector (or date_time_selector) is in a form group. This happens on all Moodle 2.x versions, it is only noticeable now because there is now a calendar icon next to the element that does not behave as it should.

        Issue Links

          Activity

          Hide
          Mark Nelson added a comment -

          Ok, so after investigating it seems this is an issue in all prior Moodle versions. The only reason we are noticing this now is because the calendar icon is displaying next to the date selector and is not triggering the pop-up. In previous Moodle versions (where there is no icon) the date selector does not actually pop-up when using the select boxes on this page. The reason is because on this form the function createElement is used, rather than addElement. The reason why it differs between the two I have yet to investigate.

          We have two options now -

          1. Fix it so that the pop-up now displays on this form.
          2. Hide the icon when we are using createElement.

          I am going to try for option 1 for now.

          Show
          Mark Nelson added a comment - Ok, so after investigating it seems this is an issue in all prior Moodle versions. The only reason we are noticing this now is because the calendar icon is displaying next to the date selector and is not triggering the pop-up. In previous Moodle versions (where there is no icon) the date selector does not actually pop-up when using the select boxes on this page. The reason is because on this form the function createElement is used, rather than addElement. The reason why it differs between the two I have yet to investigate. We have two options now - Fix it so that the pop-up now displays on this form. Hide the icon when we are using createElement. I am going to try for option 1 for now.
          Hide
          Mark Nelson added a comment -

          Ok, so it seems the date initialiser JS is never executed on this form which is why the pop-up does not happen.

          Show
          Mark Nelson added a comment - Ok, so it seems the date initialiser JS is never executed on this form which is why the pop-up does not happen.
          Hide
          Mark Nelson added a comment -

          The JS loop in dateselector.js "Y.all('fieldset.fdate_time_selector').each(function(){" does not actually loop through anything in this case. The reason for this is because the dateselector (which is a group of select boxes and an image) is being added within another group, in this case by the function setupForm in user/filters/date. This means the CSS selectors are not the same, meaning the JS is not working.

          Show
          Mark Nelson added a comment - The JS loop in dateselector.js "Y.all('fieldset.fdate_time_selector').each(function(){" does not actually loop through anything in this case. The reason for this is because the dateselector (which is a group of select boxes and an image) is being added within another group, in this case by the function setupForm in user/filters/date. This means the CSS selectors are not the same, meaning the JS is not working.
          Hide
          Mark Nelson added a comment -

          s/not working/not being executed/g.

          Show
          Mark Nelson added a comment - s/not working/not being executed/g.
          Hide
          Mark Nelson added a comment -

          Wow, that was a difficult issue. My first aim was to change the forms library so that when a date_selector (or date_time_selector) was generated within a group I would wrap a fieldset with the class 'fdate_selector' around each date_selector so that the JS was triggered, the only issue with this was it did not display the group as expected, which would have meant CSS changes etc, which was not ideal as it would break any custom themes. I then created another CSS class to attach to a group fieldset that contained any date_selectors which triggered some new JS that looped through the DOM elements to determine how many unique date_selectors there were, then got the necessary elements associated with each (ie. day, month, year, checkbox and calendar image) and put JS events on them.

          Show
          Mark Nelson added a comment - Wow, that was a difficult issue. My first aim was to change the forms library so that when a date_selector (or date_time_selector) was generated within a group I would wrap a fieldset with the class 'fdate_selector' around each date_selector so that the JS was triggered, the only issue with this was it did not display the group as expected, which would have meant CSS changes etc, which was not ideal as it would break any custom themes. I then created another CSS class to attach to a group fieldset that contained any date_selectors which triggered some new JS that looped through the DOM elements to determine how many unique date_selectors there were, then got the necessary elements associated with each (ie. day, month, year, checkbox and calendar image) and put JS events on them.
          Hide
          Mark Nelson added a comment -

          Ok, ended up coming up with a much easier solution after I had a think about it tonight. It's now past 1am and I think I am going to read, enough coding for today.

          Show
          Mark Nelson added a comment - Ok, ended up coming up with a much easier solution after I had a think about it tonight. It's now past 1am and I think I am going to read, enough coding for today.
          Show
          Mark Nelson added a comment - Alt method: https://github.com/markn86/moodle/compare/master...MDL-39187_master_alt
          Hide
          Mark Nelson added a comment -

          I now wrapping the date selectors in a span element with either the class fdate_selector or fdate_time_selector depending on the element. They are only ever wrapped in this span if they are added to a group, so in Moodle's case this is only in user/filters/date.php and mod/feedback/mod_form.php. However, I have changed mod/feedback/mod_form.php in MDL-39463 to use addElement, so this will not be an issue. This means for 99% of the cases a date_selector or date_time_selector is used the HTML generated on the page will not differ when this patch is applied.

          Show
          Mark Nelson added a comment - I now wrapping the date selectors in a span element with either the class fdate_selector or fdate_time_selector depending on the element. They are only ever wrapped in this span if they are added to a group, so in Moodle's case this is only in user/filters/date.php and mod/feedback/mod_form.php. However, I have changed mod/feedback/mod_form.php in MDL-39463 to use addElement, so this will not be an issue. This means for 99% of the cases a date_selector or date_time_selector is used the HTML generated on the page will not differ when this patch is applied.
          Hide
          Rajesh Taneja added a comment -

          Thanks Mark,

          Patch looks good although you can reduce some code by considering "Add event" which is only called when element is added, so you don't have to add if in createElement and updatevalue element.

          1. Add flag update in default which is called at AddElement, or
            _usedcreateelement = true; by default
            ...
            ...
            case 'AddElement:
               $this->_usedcreateelement = false;
               parent::onQuickFormEvent($event, $arg, $caller);
               break;
            
          2. Not sure why you changed _wrap from protected to public. Probably best to keep it as protected.
          3. Rather then using start_tag and end_tag and using 2 if's, you might want to use html_writer::tag() to do that job for you.

          Rest all looks grt. As per JS performance, it seems it's better to use selectors without tags, so all good.

          Show
          Rajesh Taneja added a comment - Thanks Mark, Patch looks good although you can reduce some code by considering "Add event" which is only called when element is added, so you don't have to add if in createElement and updatevalue element. Add flag update in default which is called at AddElement, or _usedcreateelement = true; by default ... ... case 'AddElement: $this->_usedcreateelement = false; parent::onQuickFormEvent($event, $arg, $caller); break; Not sure why you changed _wrap from protected to public. Probably best to keep it as protected. Rather then using start_tag and end_tag and using 2 if's, you might want to use html_writer::tag() to do that job for you. Rest all looks grt. As per JS performance, it seems it's better to use selectors without tags, so all good.
          Hide
          Mark Nelson added a comment -

          Thanks Raj, very useful points. Much appreciated. Pushing to integration now.

          Show
          Mark Nelson added a comment - Thanks Raj, very useful points. Much appreciated. Pushing to integration now.
          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
          Mark Nelson added a comment -

          Your wish is my command, rebasing done.

          Show
          Mark Nelson added a comment - Your wish is my command, rebasing done.
          Hide
          Damyon Wiese added a comment -

          Thanks Mark,

          This looks like a nasty issue - thanks for working on it. The changes look reasonable and work for me. Integrated to master.

          Show
          Damyon Wiese added a comment - Thanks Mark, This looks like a nasty issue - thanks for working on it. The changes look reasonable and work for me. Integrated to master.
          Hide
          Frédéric Massart added a comment -

          Tested on Chrome, and passed. Thanks!

          Show
          Frédéric Massart added a comment - Tested on Chrome, and passed. Thanks!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Did you think this day was not going to arrive ever?

          Your patience has been rewarded, yay, sent upstream, thanks!

          Closing...ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Did you think this day was not going to arrive ever? Your patience has been rewarded, yay, sent upstream, thanks! Closing...ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: