Uploaded image for project: '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
    • Status: Closed
    • Priority: 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

      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.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            markn 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
            markn 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
            markn 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
            markn 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
            markn 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
            markn 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
            markn Mark Nelson added a comment -

            s/not working/not being executed/g.

            Show
            markn Mark Nelson added a comment - s/not working/not being executed/g.
            Hide
            markn 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
            markn 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
            markn 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
            markn 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
            markn Mark Nelson added a comment - Alt method: https://github.com/markn86/moodle/compare/master...MDL-39187_master_alt
            Hide
            markn 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
            markn 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
            rajeshtaneja 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
            rajeshtaneja 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
            markn Mark Nelson added a comment -

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

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

            Your wish is my command, rebasing done.

            Show
            markn Mark Nelson added a comment - Your wish is my command, rebasing done.
            Hide
            damyon 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 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
            fred Frédéric Massart added a comment -

            Tested on Chrome, and passed. Thanks!

            Show
            fred Frédéric Massart added a comment - Tested on Chrome, and passed. Thanks!
            Hide
            stronk7 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
            stronk7 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:
                  Fix Release Date:
                  14/May/13