Moodle
  1. Moodle
  2. MDL-26649

Date picker formslib element usability improvements

    Details

    • Testing Instructions:
      Hide

      This must be tested on all supported browsers.

      Enable JS

      1. Create a new quiz on your site.
      2. On the editing page click to expand the 'Timing' section.
      3. Hover over the calendar icon next to 'Open the quiz' and 'Close the quiz' and ensure the image is not clickable and that the mouse icon does not change to a hand (this is the default behaviour (in Firefox anyway) when hovering over an input type image (even if it is disabled), this may confuse users who think they can interact with it even though the enable checkbox is not checked).
      4. Enable the 'Open the quiz' element by clicking on the 'Enable' checkbox.
      5. Click on the calendar icon and ensure the calendar pop-up displays and you can choose a date.
      6. Select a date and ensure the select box values populate with this date and the calendar closes.
      7. Click on the calendar icon again to display the pop-up and then click on it again to ensure it closes.
      8. Click on the calendar icon again to display the pop-up and then click anywhere else on the screen (besides the select boxes) to ensure it closes.
      9. Select a day, month and year using the select boxes, then click on the calendar icon and ensure the pop-up is set to this date.
      10. Change the date in the select boxes while the calendar pop-up is open and ensure the date changes.
      11. Uncheck the 'Enable' checkbox and repeat step 3.
      12. Create a new course.
      13. On the editing page for the 'Course start date' field repeat steps 3-11 as it is a dateselector element, not a datetimeselector.
      14. Create a new database activity and ensure you do not get the JS error 'TypeError: this.calendarimage is null' preventing the rest of JS on the form from loading.
      15. Try and break the calendar on Firefox with the firebug console open to spot any JS warnings/errors.

      Disable JS

      1. Create a new quiz on your site.
      2. On the editing page ensure there is no calendar icon next to the 'Open the quiz' and 'Close the quiz' fields.
      3. Create a new course.
      4. On the editing page ensure there is no calendar icon next to the 'Course start date' field.
      Show
      This must be tested on all supported browsers. Enable JS Create a new quiz on your site. On the editing page click to expand the 'Timing' section. Hover over the calendar icon next to 'Open the quiz' and 'Close the quiz' and ensure the image is not clickable and that the mouse icon does not change to a hand (this is the default behaviour (in Firefox anyway) when hovering over an input type image (even if it is disabled), this may confuse users who think they can interact with it even though the enable checkbox is not checked). Enable the 'Open the quiz' element by clicking on the 'Enable' checkbox. Click on the calendar icon and ensure the calendar pop-up displays and you can choose a date. Select a date and ensure the select box values populate with this date and the calendar closes. Click on the calendar icon again to display the pop-up and then click on it again to ensure it closes. Click on the calendar icon again to display the pop-up and then click anywhere else on the screen (besides the select boxes) to ensure it closes. Select a day, month and year using the select boxes, then click on the calendar icon and ensure the pop-up is set to this date. Change the date in the select boxes while the calendar pop-up is open and ensure the date changes. Uncheck the 'Enable' checkbox and repeat step 3. Create a new course. On the editing page for the 'Course start date' field repeat steps 3-11 as it is a dateselector element, not a datetimeselector. Create a new database activity and ensure you do not get the JS error 'TypeError: this.calendarimage is null' preventing the rest of JS on the form from loading. Try and break the calendar on Firefox with the firebug console open to spot any JS warnings/errors. Disable JS Create a new quiz on your site. On the editing page ensure there is no calendar icon next to the 'Open the quiz' and 'Close the quiz' fields. Create a new course. On the editing page ensure there is no calendar icon next to the 'Course start date' field.
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_22_STABLE, MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull Master Branch:
      MDL-26649_master
    • Rank:
      16227

      Description

      This was caused by the code in MDL-16592. The date picker used to be basic, but usable and fast to operate. Now it is hard to operate and looks broken. Our technical testing staff reported it as a bug as follows:

      <<
      For each of the date fields for the above labels i.e. 'allow editing from' and 'prevent editing from' the drop-down and the calender get displayed at the same time. This looks untidy and only one of the 2 options is necessary to make the selection and there is no need to have them both displayed at the same time.
      >>

      My suggestion would be:

      1) As the easiest fix to leave it in a professional, working state with reasonable UI, we could just remove the popup.

      2) Alternatively I suggest adding a separate button/icon (only added in JavaScript) which appears just to the right of the dropdowns, and opens the popup. So it doesn't interfere with you using the dropdowns at all. When clicking that button, the popup appears and the dropdown is disabled until you close the popup (which updates the dropdown dates).

      If anybody wants us to implement #1 we certainly can. I think we might have time to implement #2 as well. Alternatively of course we're happy to leave this to somebody else This isn't on our critical list, we can leave it sucking if we have to. But it does suck really hard, I'm surprised nobody else has complained.

      On which note - I did try to use Jira search to find if anyone had already reported this, I searched several times but didn't spot it? Very odd? Probably this is a duplicate and I just couldn't find it?

        Issue Links

          Activity

          Hide
          Teresa Gibbison added a comment -

          Hi Sam - I've noticed this too and have had a few of our users comment that it looks broken. I have searched too and only found this issue so as you can see voted on it! I think the calendar selection popup is nice but agree that it should be launched from a separate icon beside the date field as I've seen on other sites. I'm no developer so will have to just vote on this and tweet the tracker number - I see from the qa site that this remains an issue in the 2.2 code as at today
          Cheers
          Teresa

          Show
          Teresa Gibbison added a comment - Hi Sam - I've noticed this too and have had a few of our users comment that it looks broken. I have searched too and only found this issue so as you can see voted on it! I think the calendar selection popup is nice but agree that it should be launched from a separate icon beside the date field as I've seen on other sites. I'm no developer so will have to just vote on this and tweet the tracker number - I see from the qa site that this remains an issue in the 2.2 code as at today Cheers Teresa
          Hide
          Gordon Alexander added a comment -

          Hi

          I can't believe only three people (four including me) have voted for this.

          Anyway I've attached my solution which appears to work. Once I've tested it properly I'll tell you. The zip file needs to be extracted into lib/form/yui/

          Any comments, bugs, improvements please shout.

          Cheers

          Gordon

          Show
          Gordon Alexander added a comment - Hi I can't believe only three people (four including me) have voted for this. Anyway I've attached my solution which appears to work. Once I've tested it properly I'll tell you. The zip file needs to be extracted into lib/form/yui/ Any comments, bugs, improvements please shout. Cheers Gordon
          Hide
          Gordon Alexander added a comment -

          I need to add a prevent.default to stop the page jumping around.

          Show
          Gordon Alexander added a comment - I need to add a prevent.default to stop the page jumping around.
          Hide
          Stephen Bourget added a comment -

          Two questions...

          1. What version of Moodle is your fix written for? (Moodle 2.0, 2.1 or 2.2)

          2. Is there a chance that you can post it as a patch (http://docs.moodle.org/dev/How_to_create_a_patch) So we can see which lines were actually changed?

          Thanks,
          -Steve

          Show
          Stephen Bourget added a comment - Two questions... 1. What version of Moodle is your fix written for? (Moodle 2.0, 2.1 or 2.2) 2. Is there a chance that you can post it as a patch ( http://docs.moodle.org/dev/How_to_create_a_patch ) So we can see which lines were actually changed? Thanks, -Steve
          Hide
          Gordon Alexander added a comment -

          Steve,

          I'm currently testing it against 2.1, I've found and fixed a couple of cross browser issues. Once I've done that I'll check it against the other versions.

          When I've finished testing I'll create a patch as you suggest.

          Cheers

          Gordon

          Show
          Gordon Alexander added a comment - Steve, I'm currently testing it against 2.1, I've found and fixed a couple of cross browser issues. Once I've done that I'll check it against the other versions. When I've finished testing I'll create a patch as you suggest. Cheers Gordon
          Hide
          Michael de Raadt added a comment -

          Triaging and bumping this issue.

          There are screenshots in the linked duplicate.

          Show
          Michael de Raadt added a comment - Triaging and bumping this issue. There are screenshots in the linked duplicate.
          Hide
          Michael de Raadt added a comment -

          It would be good to know if this applies across browsers.

          Show
          Michael de Raadt added a comment - It would be good to know if this applies across browsers.
          Hide
          Howard Miller added a comment -

          We've now got a client with this problem (2.2). Anything we can do to help resolve this?

          Show
          Howard Miller added a comment - We've now got a client with this problem (2.2). Anything we can do to help resolve this?
          Hide
          Marek Wójcik added a comment -

          Gordon
          Did you finish your tests? How long it takes?

          Show
          Marek Wójcik added a comment - Gordon Did you finish your tests? How long it takes?
          Hide
          Mark Nelson added a comment -

          I am closing this as it is no longer an issue. You can use the select boxes as per normal, or the pop-up which now displays above the selector.

          Show
          Mark Nelson added a comment - I am closing this as it is no longer an issue. You can use the select boxes as per normal, or the pop-up which now displays above the selector.
          Hide
          Marek Wójcik added a comment -

          Pop-up doesn't display above the selector. If you display pop-up calendar, and later click into dropdown selector, selector is still above calendar. Problem is still not resolved.

          Show
          Marek Wójcik added a comment - Pop-up doesn't display above the selector. If you display pop-up calendar, and later click into dropdown selector, selector is still above calendar. Problem is still not resolved.
          Hide
          Marek Wójcik added a comment -

          Icon added in zip, doesn't show properly. CSS class are not defined properly.

          Show
          Marek Wójcik added a comment - Icon added in zip, doesn't show properly. CSS class are not defined properly.
          Hide
          Mark Nelson added a comment -

          Hi Marek, can you let me know what version of Moodle as well as the browser you are using? If you could provide a screenshot that would also be great. Thanks.

          Show
          Mark Nelson added a comment - Hi Marek, can you let me know what version of Moodle as well as the browser you are using? If you could provide a screenshot that would also be great. Thanks.
          Hide
          Marek Wójcik added a comment -

          Hi Mark
          I'm using Moodle 2.2+ (Build: 20111209)
          I have change dateselector to attached in zip file.
          Screenshot is attached.

          Show
          Marek Wójcik added a comment - Hi Mark I'm using Moodle 2.2+ (Build: 20111209) I have change dateselector to attached in zip file. Screenshot is attached.
          Hide
          Mark Nelson added a comment -

          Thanks Marek for the screenshot. It was my mistake. This does still happen when the date selector is near the bottom of the page causing the select box to render it's contents above itself, not below.

          Show
          Mark Nelson added a comment - Thanks Marek for the screenshot. It was my mistake. This does still happen when the date selector is near the bottom of the page causing the select box to render it's contents above itself, not below.
          Hide
          Dan Poltawski added a comment -

          I've noticed recently that quite a lot of sites use put the picker below rather than above, maybe we should copy it.

          Show
          Dan Poltawski added a comment - I've noticed recently that quite a lot of sites use put the picker below rather than above, maybe we should copy it.
          Hide
          Mark Nelson added a comment -

          Hi all, I am leaving on a holiday to Whistler and will not be able to look at this until I get back. If anyone would like to provide a patch, or take this issue for themselves, that would be great.

          Show
          Mark Nelson added a comment - Hi all, I am leaving on a holiday to Whistler and will not be able to look at this until I get back. If anyone would like to provide a patch, or take this issue for themselves, that would be great.
          Hide
          Ankit Agarwal added a comment -

          Hi Mark,
          Here are a few things you might want to consider:-

          1. A title on the icon would be good.
          2. I dont see a way of closing the calendar. It is very annoying. You have to click multiple times outside to get it closed. Ideally it should be closed on selecting a date.
          3. When there are six weeks displayed in a calendar month it normally overlaps with the select dropdowns.
          4. in claim_calendar you have
                        if (M.form.dateselector.currentowner) {
                            return;
                        }
                        if (M.form.dateselector.currentowner != this) {
                            this.connect_handlers();
                            this.set_date_from_selects();
                        }
            

            Am I missing something or the second if will never get executed?
            Rest looks good.
            Thanks

          Show
          Ankit Agarwal added a comment - Hi Mark, Here are a few things you might want to consider:- A title on the icon would be good. I dont see a way of closing the calendar. It is very annoying. You have to click multiple times outside to get it closed. Ideally it should be closed on selecting a date. When there are six weeks displayed in a calendar month it normally overlaps with the select dropdowns. in claim_calendar you have if (M.form.dateselector.currentowner) { return ; } if (M.form.dateselector.currentowner != this ) { this .connect_handlers(); this .set_date_from_selects(); } Am I missing something or the second if will never get executed? Rest looks good. Thanks
          Hide
          Mark Nelson added a comment -

          Hi Ankit,

          Thanks for the review.

          1) Good spotting.
          2) Done.
          3) This can not be done so easily. The code

          this.panel.set('align', {
              node:this.currentowner.get('node').one('select'),
              points:[Y.WidgetPositionAlign.BL, Y.WidgetPositionAlign.TL]
          });
          

          is responsible for positioning the calendar where Y.WidgetPositionAlign.BL and Y.WidgetPositionAlign.TL are variables which mean 'Bottom Left' and 'Top Left' of the node we are aligning with. YUI sets the style of the calendar (eg. left: 439px; top: 316.417px; z-index: 0) depending on these values. I could get the current top position and then use YUI to change it to move it up but this seems hacky. There is also the issue with moving it up when the calendar displays with 6 weeks initially, meaning we would be moving it up unnecessarily to begin with. I think we can ignore this issue for now as it is the current behaviour, so is not a regression being introduced by this commit. If it is really a concern a separate issue can be created.
          4) The first if statement is not actually needed as we only call claim_calendar on one action now (the calendar icon being clicked), rather than when different select boxes are used (day/month/year) so I removed it. The second if statement will always get executed, so I removed the if surrounding it.

          I also made it so that the image was disabled if the 'enabled' checkbox was not checked and when hovering over the image the cursor would remain the default image, rather than becoming a hand, so it is clear it does nothing.

          Show
          Mark Nelson added a comment - Hi Ankit, Thanks for the review. 1) Good spotting. 2) Done. 3) This can not be done so easily. The code this.panel.set('align', { node:this.currentowner.get('node').one('select'), points:[Y.WidgetPositionAlign.BL, Y.WidgetPositionAlign.TL] }); is responsible for positioning the calendar where Y.WidgetPositionAlign.BL and Y.WidgetPositionAlign.TL are variables which mean 'Bottom Left' and 'Top Left' of the node we are aligning with. YUI sets the style of the calendar (eg. left: 439px; top: 316.417px; z-index: 0) depending on these values. I could get the current top position and then use YUI to change it to move it up but this seems hacky. There is also the issue with moving it up when the calendar displays with 6 weeks initially, meaning we would be moving it up unnecessarily to begin with. I think we can ignore this issue for now as it is the current behaviour, so is not a regression being introduced by this commit. If it is really a concern a separate issue can be created. 4) The first if statement is not actually needed as we only call claim_calendar on one action now (the calendar icon being clicked), rather than when different select boxes are used (day/month/year) so I removed it. The second if statement will always get executed, so I removed the if surrounding it. I also made it so that the image was disabled if the 'enabled' checkbox was not checked and when hovering over the image the cursor would remain the default image, rather than becoming a hand, so it is clear it does nothing.
          Hide
          Ankit Agarwal added a comment -

          Hi Mark,
          Thanks for the changes, Patch looks good, although I didnt test it again.
          Feel free to push for integration.
          Thanks

          Show
          Ankit Agarwal added a comment - Hi Mark, Thanks for the changes, Patch looks good, although I didnt test it again. Feel free to push for integration. Thanks
          Hide
          Mark Nelson added a comment -

          I explained the logic to Ankit in person, but will also put it here for the integrator.

          handle_select_change : function(e) {
              this.closepopup = false;
              this.set_date_from_selects();
              this.closepopup = true;
          },
          

          It may appear looking at the code that the closepopup variable is not actually used, as it is not used in the handle_select_change or the set_date_from_selects function. However, in the function set_date_from_selects there is the following line of code:

          M.form.dateselector.calendar.select(new Date(year, month, day));
          

          This code calls the YUI library and changes the date on the calendar. Now, whenever the calendar is displayed via the function claim_calendar the function connect_handlers is called which contains

          M.form.dateselector.calendar.selectEvent.subscribe(this.set_selects_from_date, this, true);
          

          which triggers the call of the set_selects_from_date function whenever the date is changed. So, any call to set_date_from_selects will trigger set_selects_from_date. The closepopup variable ensures that the popup is not closed (assuming it is currently open) when a date is chosen via the select box, but that it will close if a date is chosen by clicking on a day in the calendar.

          Show
          Mark Nelson added a comment - I explained the logic to Ankit in person, but will also put it here for the integrator. handle_select_change : function(e) { this.closepopup = false; this.set_date_from_selects(); this.closepopup = true; }, It may appear looking at the code that the closepopup variable is not actually used, as it is not used in the handle_select_change or the set_date_from_selects function. However, in the function set_date_from_selects there is the following line of code: M.form.dateselector.calendar.select(new Date(year, month, day)); This code calls the YUI library and changes the date on the calendar. Now, whenever the calendar is displayed via the function claim_calendar the function connect_handlers is called which contains M.form.dateselector.calendar.selectEvent.subscribe(this.set_selects_from_date, this, true); which triggers the call of the set_selects_from_date function whenever the date is changed. So, any call to set_date_from_selects will trigger set_selects_from_date. The closepopup variable ensures that the popup is not closed (assuming it is currently open) when a date is chosen via the select box, but that it will close if a date is chosen by clicking on a day in the calendar.
          Hide
          Mark Nelson added a comment -

          I added a comment in the code so that any future developers will know what is going on.

          Show
          Mark Nelson added a comment - I added a comment in the code so that any future developers will know what is going on.
          Hide
          Martin Dougiamas added a comment -

          My +10 for this in 2.5, seems solid and useful.

          Show
          Martin Dougiamas added a comment - My +10 for this in 2.5, seems solid and useful.
          Hide
          Damyon Wiese added a comment -

          Thanks Mark,

          One small thing I spotted right away - you don't add the jsenabled class to elements you want to hide from non-JS users (or everyone will see it always). jsenabled gets automatically added to the body only if javascript is enabled - so you only need to add visibleifjs to the elements you want to hide from non-js users.

          Show
          Damyon Wiese added a comment - Thanks Mark, One small thing I spotted right away - you don't add the jsenabled class to elements you want to hide from non-JS users (or everyone will see it always). jsenabled gets automatically added to the body only if javascript is enabled - so you only need to add visibleifjs to the elements you want to hide from non-js users.
          Hide
          Mark Nelson added a comment -

          Thanks Damyon, made that adjustment.

          Show
          Mark Nelson added a comment - Thanks Damyon, made that adjustment.
          Hide
          Damyon Wiese added a comment -

          I found the linked issue while testing this change.

          Show
          Damyon Wiese added a comment - I found the linked issue while testing this change.
          Hide
          Damyon Wiese added a comment -

          Thanks Mark,

          This has been integrated to master now. I think this is much easier to use than the old version.

          Show
          Damyon Wiese added a comment - Thanks Mark, This has been integrated to master now. I think this is much easier to use than the old version.
          Hide
          Andrew Nicols added a comment -

          I know this is a formslib change, but for those changes which also affect JS, if you could add me as a watcher that would be grand and give me an opportunity to pass my eye over the JS changes too.

          Cheers,

          Andrew

          Show
          Andrew Nicols added a comment - I know this is a formslib change, but for those changes which also affect JS, if you could add me as a watcher that would be grand and give me an opportunity to pass my eye over the JS changes too. Cheers, Andrew
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          grrr, toggle_calendar_image() if failing badly with:

          TypeError: 'null' is not an object (evaluating 'this.calendarimage.set'), line #156
          

          When adding any activity to a course (tried database, chat, label...). It causes JS to stop badly (detected when running some behat tests).

          Show
          Eloy Lafuente (stronk7) added a comment - - edited grrr, toggle_calendar_image() if failing badly with: TypeError: ' null ' is not an object (evaluating ' this .calendarimage.set'), line #156 When adding any activity to a course (tried database, chat, label...). It causes JS to stop badly (detected when running some behat tests).
          Hide
          Eloy Lafuente (stronk7) added a comment -

          For your info, I'm commenting out a line in the dateselector.js to keep JS working and allow me to continue my behat integration and testing session:

          --- a/lib/form/yui/dateselector/dateselector.js
          +++ b/lib/form/yui/dateselector/dateselector.js
          @@ -87,7 +87,8 @@ YUI.add('moodle-form-dateselector', function(Y) {
                               // Set the node to the enablecheckbox variable.
                               this.enablecheckbox = node;
                               // Set the calendar icon status depending on the value of the checkbox.
          -                    this.toggle_calendar_image();
          +                    // QUICK HACK to keep JS working. MDL-26649. FIXME!
          +                    // this.toggle_calendar_image();
                           }
                       }, this);
                   },
          

          I don't know anything but that call being the apparent culprit, so don't ask.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - For your info, I'm commenting out a line in the dateselector.js to keep JS working and allow me to continue my behat integration and testing session: --- a/lib/form/yui/dateselector/dateselector.js +++ b/lib/form/yui/dateselector/dateselector.js @@ -87,7 +87,8 @@ YUI.add('moodle-form-dateselector', function(Y) { // Set the node to the enablecheckbox variable. this .enablecheckbox = node; // Set the calendar icon status depending on the value of the checkbox. - this .toggle_calendar_image(); + // QUICK HACK to keep JS working. MDL-26649. FIXME! + // this .toggle_calendar_image(); } }, this ); }, I don't know anything but that call being the apparent culprit, so don't ask. Ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          just test-failing this in advance

          Show
          Eloy Lafuente (stronk7) added a comment - just test-failing this in advance
          Hide
          Mark Nelson added a comment -

          Doh, Martin said if this caused any regressions I would have to cut off my beard. However, this doesn't count as we are still in integration and not in stable.

          Eloy, I created a fix: https://github.com/markn86/moodle/commit/76b47b173b4ed679fd4e02e5f26fff67af2f56f7 if this could be pulled into int that would be great.

          Show
          Mark Nelson added a comment - Doh, Martin said if this caused any regressions I would have to cut off my beard. However, this doesn't count as we are still in integration and not in stable. Eloy, I created a fix: https://github.com/markn86/moodle/commit/76b47b173b4ed679fd4e02e5f26fff67af2f56f7 if this could be pulled into int that would be great.
          Hide
          Dan Poltawski added a comment -

          Mark: you were struggling to reproduce this - seemingly you have done now - could you add the instructions which trigger it to these testing instructions?

          Show
          Dan Poltawski added a comment - Mark: you were struggling to reproduce this - seemingly you have done now - could you add the instructions which trigger it to these testing instructions?
          Hide
          Mark Nelson added a comment -

          I couldn't replicate the error when adding a label, but was able to when I added a database activity. I added an extra step to the instructions.

          Show
          Mark Nelson added a comment - I couldn't replicate the error when adding a label, but was able to when I added a database activity. I added an extra step to the instructions.
          Hide
          Damyon Wiese added a comment -

          Just checking against behat (should have done this earlier).

          Show
          Damyon Wiese added a comment - Just checking against behat (should have done this earlier).
          Hide
          Damyon Wiese added a comment -

          Confirming that behat is passing (only tested mod_choice which has date fields in the test).

          Show
          Damyon Wiese added a comment - Confirming that behat is passing (only tested mod_choice which has date fields in the test).
          Hide
          Damyon Wiese added a comment -

          Pulled in this fix - sending this back for re-testing.

          Show
          Damyon Wiese added a comment - Pulled in this fix - sending this back for re-testing.
          Hide
          Adrian Greeve added a comment -

          Tested on the master integration branch.
          Besides problems already reported, this works fine.
          Test passed.

          Show
          Adrian Greeve added a comment - Tested on the master integration branch. Besides problems already reported, this works fine. Test passed.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Your awesome contributions are now part of Moodle, your fav LMS out there.

          Closing this as fixed.

          Many thanks for all the hard work, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Your awesome contributions are now part of Moodle, your fav LMS out there. Closing this as fixed. Many thanks for all the hard work, ciao

            People

            • Votes:
              20 Vote for this issue
              Watchers:
              18 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: