Moodle
  1. Moodle
  2. MDL-40470

url_select triggers on click not on change.

    Details

    • Testing Instructions:
      Hide

      In as many browsers as possible:

      • IE8
      • IE9
      • IE10
      • Firefox
      • Chrome
      • iOS Safari
      • iOS Chrome
      • Android
      • Chrome Android
      • Whatever else you can find

      Testing instructions:

      • Open a course
      • Turn editing on
      • Click the "Add block..." dropdown
      • Close it (by clicking on it) it again without making a selection
        • Confirm no change
      • Hit enter (keyboard) to open it again
      • Hit enter (keyboard) to close it again
        • Confirm no change
      • Use the mouse to select an item from the "Add block..." dropdown
        • Confirm that it was added
      • Use the keyboard to select an item from the "Add block..." dropdown
        • Confirm that it was added
      • Add a new assignment
        • Set the feedback type to the "Offline grading worksheet"
      • Open the activity
      • View/grade all submissions
      • From the dropdown choose "Download grading worksheet"
        • Confirm that you were prompted to download the file
        • Click cancel
      • Click the dropdown again and change it to the top option
        • Confirm that there was no change, and you weren't promped to download the file again

      Note: Opera used to not handle some items properly - keyboard IIRC. This may be fixed now they've moved to Blink.

      Show
      In as many browsers as possible: IE8 IE9 IE10 Firefox Chrome iOS Safari iOS Chrome Android Chrome Android Whatever else you can find Testing instructions: Open a course Turn editing on Click the "Add block..." dropdown Close it (by clicking on it) it again without making a selection Confirm no change Hit enter (keyboard) to open it again Hit enter (keyboard) to close it again Confirm no change Use the mouse to select an item from the "Add block..." dropdown Confirm that it was added Use the keyboard to select an item from the "Add block..." dropdown Confirm that it was added Add a new assignment Set the feedback type to the "Offline grading worksheet" Open the activity View/grade all submissions From the dropdown choose "Download grading worksheet" Confirm that you were prompted to download the file Click cancel Click the dropdown again and change it to the top option Confirm that there was no change, and you weren't promped to download the file again Note: Opera used to not handle some items properly - keyboard IIRC. This may be fixed now they've moved to Blink.
    • Difficulty:
      Easy
    • Affected Branches:
      MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE
    • Pull 2.4 Branch:
    • Pull 2.5 Branch:
    • Pull Master Branch:
      MDL-40470-master
    • Rank:
      51272

      Description

      I fixed this issue a while ago (sorry, can't find tracker issue), but it seems the code has been changed in 2.5 onwards so the issue is present again.

      1. Visit a course.
      2. Create an assignment.
      3. Check 'Offline grading worksheet' under 'Feedback types'.
      4. Click on the assignment link.
      5. Click 'View/grade all submissions'.
      6. In the select box labelled 'Grading action' choose 'Download grading worksheet'.
      7. When the pop-up appears to download the file, click 'cancel' and remain on the page.
      8. Attempt to change this to 'Upload grading worksheet' and notice that the download grading worksheet action is triggered.

        Issue Links

          Activity

          Hide
          Andrew Nicols added a comment -

          Hi Mark,

          Which browsers? Sadly, there's a blinking complicated set of logic. Some browsers don't bubbe change events on chnage, others bubble click events on change. There's a set of browser logic to try and work it out, but it could be that the affected browsers (firefox + opera IIRC) now do the right thing and we need to change this logic.

          Show
          Andrew Nicols added a comment - Hi Mark, Which browsers? Sadly, there's a blinking complicated set of logic. Some browsers don't bubbe change events on chnage, others bubble click events on change. There's a set of browser logic to try and work it out, but it could be that the affected browsers (firefox + opera IIRC) now do the right thing and we need to change this logic.
          Hide
          Mark Nelson added a comment -

          Hi Andrew, this was done in FF and still an issue.

          Show
          Mark Nelson added a comment - Hi Andrew, this was done in FF and still an issue.
          Hide
          Mark Nelson added a comment -

          Found the old issue - MDL-35954.

          I am guessing it has something to do with the function check_changed in yui/src/formautosubmit/js/formautosubmit.js ..

          Show
          Mark Nelson added a comment - Found the old issue - MDL-35954 . I am guessing it has something to do with the function check_changed in yui/src/formautosubmit/js/formautosubmit.js ..
          Hide
          Andrew Nicols added a comment -

          I've just tried to replicate this in Firefox 22.0 on Mac. What version of FF are you using?

          Show
          Andrew Nicols added a comment - I've just tried to replicate this in Firefox 22.0 on Mac. What version of FF are you using?
          Hide
          Mark Nelson added a comment -

          Still happens. Just confirmed then on Stable 2.5. I am using FF 24.

          Show
          Mark Nelson added a comment - Still happens. Just confirmed then on Stable 2.5. I am using FF 24.
          Hide
          Andrew Nicols added a comment -

          I'm still unable to replicate this on Mac OS 10.8.4 running:

          • Firefox 22.0
          • Firefox 24.0
          • Chrome
          • Safari

          What OS are you seeing this on? I wonder if it's a Linux + Firefox issue or something?

          Show
          Andrew Nicols added a comment - I'm still unable to replicate this on Mac OS 10.8.4 running: Firefox 22.0 Firefox 24.0 Chrome Safari What OS are you seeing this on? I wonder if it's a Linux + Firefox issue or something?
          Hide
          Mark Nelson added a comment -

          Ubuntu. I am going to ask some of the other devs to see if they can replicate this.

          Show
          Mark Nelson added a comment - Ubuntu. I am going to ask some of the other devs to see if they can replicate this.
          Hide
          Andrew Nicols added a comment -

          You were going to check this - can you show me on Monday morning?

          Cheers

          Show
          Andrew Nicols added a comment - You were going to check this - can you show me on Monday morning? Cheers
          Hide
          Mark Nelson added a comment -

          After showing Andrew in person he now understands what is going on. Now he can fly back to England.

          Show
          Mark Nelson added a comment - After showing Andrew in person he now understands what is going on. Now he can fly back to England.
          Hide
          Andrew Nicols added a comment -

          Apologies for the painful testing instructions. We could do with some behat tests for this if possible, and I'd like to write some JS Unit tests too.

          Show
          Andrew Nicols added a comment - Apologies for the painful testing instructions. We could do with some behat tests for this if possible, and I'd like to write some JS Unit tests too.
          Hide
          Andrew Nicols added a comment -

          Ping...

          Show
          Andrew Nicols added a comment - Ping...
          Hide
          Mark Nelson added a comment -

          Looking good Andrew, and so does the code.

          Submitting to integration.

          Show
          Mark Nelson added a comment - Looking good Andrew, and so does the code. Submitting to integration.
          Hide
          Eloy Lafuente (stronk7) 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
          Eloy Lafuente (stronk7) 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
          Damyon Wiese added a comment -

          Thanks Andrew - I added the linked issue to deal with keyboard navigation on chrome - but that is the same before and after your patch. The other thing I noticed about this change is that it is reporting 'yui: NOT loaded: node-data' to the console (lots of times) on the course page on all branches (chrome + firefox). Can you add a fix for this second issue?

          Show
          Damyon Wiese added a comment - Thanks Andrew - I added the linked issue to deal with keyboard navigation on chrome - but that is the same before and after your patch. The other thing I noticed about this change is that it is reporting 'yui: NOT loaded: node-data' to the console (lots of times) on the course page on all branches (chrome + firefox). Can you add a fix for this second issue?
          Hide
          Andrew Nicols added a comment -

          Added a patch to each branch for the issue described.
          node-data is automatically loaded by the YUI loader if the browser doesn't support data-attribute and isn't a first class submodule in it's own right.

          Show
          Andrew Nicols added a comment - Added a patch to each branch for the issue described. node-data is automatically loaded by the YUI loader if the browser doesn't support data-attribute and isn't a first class submodule in it's own right.
          Hide
          Damyon Wiese added a comment -

          Thanks Andrew - I would prefer if you could squash those commits:

          Quoting because this was clarified recently:

          "Git commits should not:

          Include changes from bugs found and fixed before integration"

          http://docs.moodle.org/dev/Coding_style#Git_commits

          Show
          Damyon Wiese added a comment - Thanks Andrew - I would prefer if you could squash those commits: Quoting because this was clarified recently: "Git commits should not: Include changes from bugs found and fixed before integration" http://docs.moodle.org/dev/Coding_style#Git_commits
          Hide
          Andrew Nicols added a comment -

          Squashed as per your request. Sorry - I was being dense!

          Show
          Andrew Nicols added a comment - Squashed as per your request. Sorry - I was being dense!
          Hide
          Damyon Wiese added a comment -

          Thanks Andrew,

          This has been integrated to 24, 25 and master now. I tested this in integration - and apart from the chrome issue (which was there before) did not find any errors.

          Show
          Damyon Wiese added a comment - Thanks Andrew, This has been integrated to 24, 25 and master now. I tested this in integration - and apart from the chrome issue (which was there before) did not find any errors.
          Hide
          Damyon Wiese added a comment -

          Passing test.

          Show
          Damyon Wiese added a comment - Passing test.
          Hide
          Dan Poltawski added a comment -

          Congratulations - this issue has been included in Moodle and is now available on our git mirrors and shortly will become available on the download servers shortly.

          Show
          Dan Poltawski added a comment - Congratulations - this issue has been included in Moodle and is now available on our git mirrors and shortly will become available on the download servers shortly.

            People

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

              Dates

              • Created:
                Updated:
                Resolved: