Moodle
  1. Moodle
  2. MDL-36619

A user cannot reorder the courses in the block 'Course overview' on 'My home' when javascript is turned off

    Details

    • Testing Instructions:
      Hide

      Test 1

      1. Log in as admin and disable ajax
      2. Log in as student and go to "My Home" page and click "Customise this page"
      3. Make sure you see move icon
      4. Click on move should show drop option icons (move here icon)
      5. click on anyone of them and make sure course you were moving is placed in correct order.
      6. Turn JS off in the browser and reload page, it should still show same move icon. Try re-order courses and it should work fine.

      Test 1

      1. Enrol a student in some courses
      2. Enable AJAX
      3. Login as that student
      4. Go to my home and click "Customise this page"
      5. Make sure you can drag-n-drop course to re-order
      6. Turn JS off in the browser
      7. Reload the page and you should different move icon.
      8. Try re-order courses and it should work fine.
      Show
      Test 1 Log in as admin and disable ajax Log in as student and go to "My Home" page and click "Customise this page" Make sure you see move icon Click on move should show drop option icons (move here icon) click on anyone of them and make sure course you were moving is placed in correct order. Turn JS off in the browser and reload page, it should still show same move icon. Try re-order courses and it should work fine. Test 1 Enrol a student in some courses Enable AJAX Login as that student Go to my home and click "Customise this page" Make sure you can drag-n-drop course to re-order Turn JS off in the browser Reload the page and you should different move icon. Try re-order courses and it should work fine.
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull Master Branch:
      wip-mdl-36619
    • Rank:
      46111

      Description

      A user cannot reorder the courses in the block 'Course overview' on 'My home' when javascript is turned off.

      • Enrol a student in some courses
      • Login as that student
      • Go to my home
      • Turn JS off

      Excepted:

      • You can reorder the courses

      Actual:

      • You can't

        Issue Links

          Activity

          Hide
          Rajesh Taneja added a comment -

          ajaxenabled() is returning true, when JS is turned off in browser

          Show
          Rajesh Taneja added a comment - ajaxenabled() is returning true, when JS is turned off in browser
          Hide
          Rajesh Taneja added a comment -

          Thanks for reporting this.

          I've put that on the backlog.

          In the meantime feel free to help us work on this issue. If you are able to provide a patch or links to your Git repository branch, please add a patch label so we will spot it.

          Show
          Rajesh Taneja added a comment - Thanks for reporting this. I've put that on the backlog. In the meantime feel free to help us work on this issue. If you are able to provide a patch or links to your Git repository branch, please add a patch label so we will spot it.
          Hide
          Andrew Davis added a comment -

          "Turn JS off"

          Update the testing instructions to make this clear that you mean to turn off JS in the browser, assuming that is what you mean.

          [Y] Syntax
          [Y] Output
          [Y] Whitespace
          [NA] Language
          [NA] Databases
          [Y] Testing
          [Y] Security
          [Y] Documentation
          [Y] Git
          [Y] Sanity check

          Looks good.

          Show
          Andrew Davis added a comment - "Turn JS off" Update the testing instructions to make this clear that you mean to turn off JS in the browser, assuming that is what you mean. [Y] Syntax [Y] Output [Y] Whitespace [NA] Language [NA] Databases [Y] Testing [Y] Security [Y] Documentation [Y] Git [Y] Sanity check Looks good.
          Hide
          Rajesh Taneja added a comment -

          Thanks Andrew,

          I have updated testing instructions.

          Show
          Rajesh Taneja added a comment - Thanks Andrew, I have updated testing instructions.
          Hide
          Dan Poltawski added a comment -

          Hi Raj,

          The point is that we can't work out serverside whether JS is enabled. We should just fallback gracefully.

          So rather than using noscript, the modern approach is to use the jsenabled class which is added to the document body by JS at the top of the body.

          git grep for .jsenabled to see many examples of this.

          Show
          Dan Poltawski added a comment - Hi Raj, The point is that we can't work out serverside whether JS is enabled. We should just fallback gracefully. So rather than using noscript, the modern approach is to use the jsenabled class which is added to the document body by JS at the top of the body. git grep for .jsenabled to see many examples of this.
          Hide
          Rajesh Taneja added a comment -

          Hello Dan,

          I am using visibleifjs and it works fine. Only case if client doesn't support JS, then there is a good fallback by noscript.

          Not sure why this is wrong. Can you please check this again.

          Show
          Rajesh Taneja added a comment - Hello Dan, I am using visibleifjs and it works fine. Only case if client doesn't support JS, then there is a good fallback by noscript. Not sure why this is wrong. Can you please check this again.
          Hide
          Dan Poltawski added a comment -

          Added Sam here.

          Sam, I seem to remember you and I discussing the use of noscript in html. But now that I search, I can't justify to Raj why this is a bad idea. At all.

          Am I misremembering, or is there a reason for us to avoid noscript tag?

          Show
          Dan Poltawski added a comment - Added Sam here. Sam, I seem to remember you and I discussing the use of noscript in html. But now that I search, I can't justify to Raj why this is a bad idea. At all. Am I misremembering, or is there a reason for us to avoid noscript tag?
          Hide
          Sam Hemelryk added a comment -

          Hi guys,

          Noscript is tolerable if there is good reason to use it.

          When thinking about it the main reason it is not so great is because it usually gets used when there is a greater problem in the implementation.
          More specifically that JavaScript has not been used in a complementary way.

          As per our JS guidelines http://docs.moodle.org/dev/JavaScript_guidelines, Moodle should work with without JS and JS should be used for progressive enhancement.
          If we need a noscript tag in order to present a different interface then we can be 99% sure that JS is not being used for progressive enhancement and really noscript is just facilitating making things work without JS without fixing the design.

          No doubt there are uses of noscript that make sense, perhaps for example if you had a plugin that required JS and would not function without it you could use a noscript element to point the user to a help page that explains what is wrong and advises them on how to fix it.
          I should add I've not read your patch sorry Raj, I've just dumped my thoughts on noscript.
          I'm assuming this is about introducing an alternative interface. If so then really the correct solution for this would be to redesign the interface so that JS was used correctly (progressive enhancement).
          Any other solution would be a band-aid hack until such a change would be made.
          Not that a band aid is a bad thing, really it all comes down to time available vs worth of the change. I'll leave that up to you and Dan to decide.
          Personally I'd rather not see noscript tags land but thats only a preference, if its the best solution anyway lets get it in

          Many thanks
          Sam

          Show
          Sam Hemelryk added a comment - Hi guys, Noscript is tolerable if there is good reason to use it. When thinking about it the main reason it is not so great is because it usually gets used when there is a greater problem in the implementation. More specifically that JavaScript has not been used in a complementary way. As per our JS guidelines http://docs.moodle.org/dev/JavaScript_guidelines , Moodle should work with without JS and JS should be used for progressive enhancement. If we need a noscript tag in order to present a different interface then we can be 99% sure that JS is not being used for progressive enhancement and really noscript is just facilitating making things work without JS without fixing the design. No doubt there are uses of noscript that make sense, perhaps for example if you had a plugin that required JS and would not function without it you could use a noscript element to point the user to a help page that explains what is wrong and advises them on how to fix it. I should add I've not read your patch sorry Raj, I've just dumped my thoughts on noscript. I'm assuming this is about introducing an alternative interface. If so then really the correct solution for this would be to redesign the interface so that JS was used correctly (progressive enhancement). Any other solution would be a band-aid hack until such a change would be made. Not that a band aid is a bad thing, really it all comes down to time available vs worth of the change. I'll leave that up to you and Dan to decide. Personally I'd rather not see noscript tags land but thats only a preference, if its the best solution anyway lets get it in Many thanks Sam
          Hide
          Rajesh Taneja added a comment -

          Thanks Sam,

          If by "progressive enhancement" you mean JS should alter DOM for UI, then probably we need to re-consider it. DOM changes by JS is expensive and surely it will add to browser responsive issues.

          IMHO, we should try show/hide alternative UI by CSS or <noscrpit>. I will change the logic if you feel otherwise.
          IMO, current logic of showing move icons if more responsive and simpler.

          Show
          Rajesh Taneja added a comment - Thanks Sam, If by "progressive enhancement" you mean JS should alter DOM for UI, then probably we need to re-consider it. DOM changes by JS is expensive and surely it will add to browser responsive issues. IMHO, we should try show/hide alternative UI by CSS or <noscrpit>. I will change the logic if you feel otherwise. IMO, current logic of showing move icons if more responsive and simpler.
          Hide
          Sam Hemelryk added a comment -

          Hi Raj,

          The idea behind progressive enhancement is that we don't have an alternative UI and that the JavaScript makes minimal changes to the DOM, really in most cases it should be manipulating the DOM on when an action occurs.
          The following all form a good read:

          Having looked at your patch I see you are working with the order functionality for the course.
          Its worth pointing out that we already handle this sort of situation in several other places, off the top of my head:

          1. Moving sections with a course.
            Uses two icon, one to move up and one to move down. When JS is enabled both icons are removed and a new icon is added in a different location to allow the user to drag and drop.
          2. Moving activities with sections/course.
            A two step system using a single icon, the user clicks the icon and the page reloads with areas marked where the user can move the activity to. The user clicks the area they want and the activity is moved. This uses a single icon, when JS loads it replaces the icon with another icon and sets up drag and drop. There is also a link to cancel the action.
          3. Moving blocks.
            A two step system using a single icon, the user clicks the icon and the page reloads with areas highlighted in red where the user can move the block to. The user clicks the area they want and the block moved. This uses a single icon, when JS loads it replaces the icon with another icon and sets up drag and drop.

          In summary none of them use noscript and JS is used only to enhance the action through the use of drag and drop.
          Really the first, moving sections, is the worst in that it causes a visual change which if you are watching closely you will notice.
          The second and third (activities and blocks) I would consider ideal. Most people have JS enabled so two reload instead of one is tolerable, there is no visual jump other than changing the icon and you have to watch bloody close to notice that as the icon is the same style/color/size.

          I think this approach should be rethought to allow for an action similar to that of blocks/activities. That I think would be the best solution.
          I still feel using noscript perhaps is tolerable as a band-aid solution here, although truly I don't feel it should land.
          Either way I was only asked to comment on noscript, I will leave it to you and Dan to decide.

          Hope that helps clear things up.
          Sam

          Show
          Sam Hemelryk added a comment - Hi Raj, The idea behind progressive enhancement is that we don't have an alternative UI and that the JavaScript makes minimal changes to the DOM, really in most cases it should be manipulating the DOM on when an action occurs. The following all form a good read: http://docs.moodle.org/dev/Unobtrusive_Javascript http://docs.moodle.org/dev/Progressive_enhancement http://en.wikipedia.org/wiki/Progressive_enhancement Having looked at your patch I see you are working with the order functionality for the course. Its worth pointing out that we already handle this sort of situation in several other places, off the top of my head: Moving sections with a course. Uses two icon, one to move up and one to move down. When JS is enabled both icons are removed and a new icon is added in a different location to allow the user to drag and drop. Moving activities with sections/course. A two step system using a single icon, the user clicks the icon and the page reloads with areas marked where the user can move the activity to. The user clicks the area they want and the activity is moved. This uses a single icon, when JS loads it replaces the icon with another icon and sets up drag and drop. There is also a link to cancel the action. Moving blocks. A two step system using a single icon, the user clicks the icon and the page reloads with areas highlighted in red where the user can move the block to. The user clicks the area they want and the block moved. This uses a single icon, when JS loads it replaces the icon with another icon and sets up drag and drop. In summary none of them use noscript and JS is used only to enhance the action through the use of drag and drop. Really the first, moving sections, is the worst in that it causes a visual change which if you are watching closely you will notice. The second and third (activities and blocks) I would consider ideal. Most people have JS enabled so two reload instead of one is tolerable, there is no visual jump other than changing the icon and you have to watch bloody close to notice that as the icon is the same style/color/size. I think this approach should be rethought to allow for an action similar to that of blocks/activities. That I think would be the best solution. I still feel using noscript perhaps is tolerable as a band-aid solution here, although truly I don't feel it should land. Either way I was only asked to comment on noscript, I will leave it to you and Dan to decide. Hope that helps clear things up. Sam
          Hide
          Rajesh Taneja added a comment -

          Thanks Sam,

          I will revisit this and try get similar behaviour as of blocks.

          Show
          Rajesh Taneja added a comment - Thanks Sam, I will revisit this and try get similar behaviour as of blocks.
          Hide
          Rajesh Taneja added a comment -

          Hi! Fred,

          I am assigning you as peer-reviewer for this as it involves icons and UI change. Hope it's fine with Andrew.

          I have done changes as suggested by Sam. Now user can order course, similar to blocks and course activities. I have also renamed some variables as they were not so clear, hope that is fine as this was introduced in 2.4 and it will be nice to correct them at this point then leaving it for later.

          Cheers.

          Show
          Rajesh Taneja added a comment - Hi! Fred, I am assigning you as peer-reviewer for this as it involves icons and UI change. Hope it's fine with Andrew. I have done changes as suggested by Sam. Now user can order course, similar to blocks and course activities. I have also renamed some variables as they were not so clear, hope that is fine as this was introduced in 2.4 and it will be nice to correct them at this point then leaving it for later. Cheers.
          Hide
          Frédéric Massart added a comment -

          Hi Raj, nice one! Thanks for your work on this, here are a few comments:

          1. On a general note I'd use $ismovingcourse as a boolean and set it to false at the beginning of the renderer. I felt like it was tricky to understand if (empty($ismovingcourse)), but somehow if (!$ismovingcourse) is easier.
          2. A few lines are too long.
          3. lang:44 I don't think it's required to have two white spaces there, one (non-nbsp) is probably enough.
          4. js:6 i/move_2d is the right icon, but FYI the icon should have previously been renamed to t/dragdrop (which does not exist). So all good!
          5. renderer: I think you can make use of $this->output instead of $OUTPUT; I didn't thouroughly check though.
          6. renderer:100 Should not define cursor anymore as it would set the wrong pointer on the non-js link.
          7. renderer:103 The link cannot encapsulate a block element <div>. I'd suggest to add the link in the <div> and remove its class 'coursemovelink' because it can be selected using .course-title .move a.
          8. renderer: 151 Do you need this ID? (see next comment)
          9. css:133 Do not define the ID within a selector as it's supposed to be unique. I'd suggest you not to use the ID at all in selectors as it's considered bad practice. I think just removing the #course_list would be enough for a proper selector.
          10. css: As your removed certain classes, can you make sure they're still used or remove them from styles.css?

          Should we backport this? I mean backporting the fix, not the improvement.

          Cheers,
          Fred

          Show
          Frédéric Massart added a comment - Hi Raj, nice one! Thanks for your work on this, here are a few comments: On a general note I'd use $ismovingcourse as a boolean and set it to false at the beginning of the renderer. I felt like it was tricky to understand if (empty($ismovingcourse)) , but somehow if (!$ismovingcourse) is easier. A few lines are too long. lang:44 I don't think it's required to have two white spaces there, one (non-nbsp) is probably enough. js:6 i/move_2d is the right icon, but FYI the icon should have previously been renamed to t/dragdrop (which does not exist). So all good! renderer: I think you can make use of $this->output instead of $OUTPUT; I didn't thouroughly check though. renderer:100 Should not define cursor anymore as it would set the wrong pointer on the non-js link. renderer:103 The link cannot encapsulate a block element <div>. I'd suggest to add the link in the <div> and remove its class 'coursemovelink' because it can be selected using .course-title .move a . renderer: 151 Do you need this ID? (see next comment) css:133 Do not define the ID within a selector as it's supposed to be unique. I'd suggest you not to use the ID at all in selectors as it's considered bad practice. I think just removing the #course_list would be enough for a proper selector. css: As your removed certain classes, can you make sure they're still used or remove them from styles.css? Should we backport this? I mean backporting the fix, not the improvement. Cheers, Fred
          Hide
          Rajesh Taneja added a comment -

          Thanks for the review Fred,

          1. Changed to $ismovingcourse
          2. Lines are not exceeding limit (130-180), hope it's fine.
          3. Two white spaces are in sync with moving activity, so keeping it for now.
          4. -
          5. You are right, removed $OUTPUT usage
          6. Cursor is now set by JS, after image is replaced.
          7. Good catch, changed.
          8. Aha, don't know how I missed it. Changed to class
          9. -
          10. Removed unused classes from css

          As suggested by Sam, moving course is same as moving activity/block.
          Had a chat with Sam, before starting this re-work and he said it can be backported later.

          Again up for peer-review.

          Show
          Rajesh Taneja added a comment - Thanks for the review Fred, Changed to $ismovingcourse Lines are not exceeding limit (130-180), hope it's fine. Two white spaces are in sync with moving activity, so keeping it for now. - You are right, removed $OUTPUT usage Cursor is now set by JS, after image is replaced. Good catch, changed. Aha, don't know how I missed it. Changed to class - Removed unused classes from css As suggested by Sam, moving course is same as moving activity/block. Had a chat with Sam, before starting this re-work and he said it can be backported later. Again up for peer-review.
          Hide
          Frédéric Massart added a comment -

          Thanks Raj, I'm pushing it to integration. I noticed that we could have used the class 'iconsmall' on icons, but that's going to add up plenty of other CSS rules to revert the changes implemented by this class. Even if I think we should add it to every icon, this could probably wait for the CSS refactoring. Cheers!

          Show
          Frédéric Massart added a comment - Thanks Raj, I'm pushing it to integration. I noticed that we could have used the class 'iconsmall' on icons, but that's going to add up plenty of other CSS rules to revert the changes implemented by this class. Even if I think we should add it to every icon, this could probably wait for the CSS refactoring. Cheers!
          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
          Sam Hemelryk added a comment -

          Thanks Raj, all looks spot on and has been integrated now.

          Show
          Sam Hemelryk added a comment - Thanks Raj, all looks spot on and has been integrated now.
          Hide
          Jason Fowler added a comment -

          All works fine, thanks Raj

          Show
          Jason Fowler added a comment - All works fine, thanks Raj
          Hide
          Eloy Lafuente (stronk7) added a comment -

          A brilliant future is awaiting us out there, better with your code. Let's look towards the future together, this is now closed.

          (and won't be revisiting it unless some regression is found)

          Thanks and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - A brilliant future is awaiting us out there, better with your code. Let's look towards the future together, this is now closed. (and won't be revisiting it unless some regression is found) Thanks and ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: