Moodle
  1. Moodle
  2. MDL-36092

Impossible to add resource/activities when JS turned off

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.3.2
    • Fix Version/s: 2.3.3
    • Component/s: Course
    • Testing Instructions:
      Hide
      • [-] Open a course
      • [-] Turn editing mode on
      • [-] Disable the Activity chooser from the 'Course settings' menu in the 'Settings' block
      • [ ] Confirm that the 'Add an activity or resource' link is not shown and that the dropdown pickers are shown
      • [-] Disable JavaScript in your browser and refresh the page
      • [ ] Confirm that the 'Add an activity or resource' link is not shown and that the dropdown pickers are shown
      • [-] Re-enable JavaScript in your browser and refresh the page
      • [-] Re-enable the Activity chooser
      • [ ] Confirm that the 'Add an activity or resource' link is shown
      • [-] Disable JavaScript in your browser and refresh the page
      • [ ] Confirm that the 'Add an activity or resource' link is not shown and that the dropdown pickers are shown
      • [-] Re-enable JavaScript in your browser and refresh the page
      • [ ] Confirm that the 'Add an activity or resource' link is shown
      Show
      [-] Open a course [-] Turn editing mode on [-] Disable the Activity chooser from the 'Course settings' menu in the 'Settings' block [ ] Confirm that the 'Add an activity or resource' link is not shown and that the dropdown pickers are shown [-] Disable JavaScript in your browser and refresh the page [ ] Confirm that the 'Add an activity or resource' link is not shown and that the dropdown pickers are shown [-] Re-enable JavaScript in your browser and refresh the page [-] Re-enable the Activity chooser [ ] Confirm that the 'Add an activity or resource' link is shown [-] Disable JavaScript in your browser and refresh the page [ ] Confirm that the 'Add an activity or resource' link is not shown and that the dropdown pickers are shown [-] Re-enable JavaScript in your browser and refresh the page [ ] Confirm that the 'Add an activity or resource' link is shown
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-36092-master
    • Rank:
      44853

      Description

      When the activity chooser is disabled and then Javascript is disabled, then the dropdown menus to add activities/resources to a course are not displayed.

      See Marina's comment on MDL-35339: http://tracker.moodle.org/browse/MDL-35339?focusedCommentId=183784&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-183784

      Replication steps:

      1. Log in as teacher/admin
      2. Navigate to a course
      3. In the Settings block under Course admin, click "Activity Chooser off"
      4. Disable JS in your browser
      5. Refresh the page

      Expected result: The "Add a resource..." and "Add an activity..." drop-down lists should appear

      Actual results: The link to the Activity chooser appears, but is inactive.

        Issue Links

          Activity

          Hide
          Michael de Raadt added a comment -

          Thankfully the drop-down lists appear with JS off when the Activity chooser setting is untouched.

          Show
          Michael de Raadt added a comment - Thankfully the drop-down lists appear with JS off when the Activity chooser setting is untouched.
          Hide
          Andrew Nicols added a comment -

          Intriguing - it works for me
          If JS is disabled, then the .jsenabled class isn't added to the body tag and the module picker isn't shown. Which browser was this with?

          That said, I've done some work to improve things anyway for JS brokenness fallback. I've changed the existing span into a hyperlink. The default href of the hyperlink will turn off the Module Chooser. If the module chooser works, then the link action is suppressed.
          As a result of creating the hyperlink when the page is created, we no longer need to perform a per-section DOM change which improves performance too.

          My only concern related to the speed at which the YUI is loaded (in general). Because of the nature in which we load YUI (e.g. only initialising the YUI module in the footer), on a large course it may take a long time for a page to fully load, and thus the YUI code isn't included for a while. If a user wants to add an activity and clicks the link while the page is still loading, it will reload the page and disable the activity chooser. If the user didn't know how (or why) the activity chooser was disabled, then they may not know how to turn it back on. This may be improved by MDL-36100.

          Show
          Andrew Nicols added a comment - Intriguing - it works for me If JS is disabled, then the .jsenabled class isn't added to the body tag and the module picker isn't shown. Which browser was this with? That said, I've done some work to improve things anyway for JS brokenness fallback. I've changed the existing span into a hyperlink. The default href of the hyperlink will turn off the Module Chooser. If the module chooser works, then the link action is suppressed. As a result of creating the hyperlink when the page is created, we no longer need to perform a per-section DOM change which improves performance too. My only concern related to the speed at which the YUI is loaded (in general). Because of the nature in which we load YUI (e.g. only initialising the YUI module in the footer), on a large course it may take a long time for a page to fully load, and thus the YUI code isn't included for a while. If a user wants to add an activity and clicks the link while the page is still loading, it will reload the page and disable the activity chooser. If the user didn't know how (or why) the activity chooser was disabled, then they may not know how to turn it back on. This may be improved by MDL-36100 .
          Hide
          Frédéric Massart added a comment -

          Hi Andrew, not sure we understood each other here. When I disable both JavaScript AND the activity chooser, the dropdown menu to add resource or activity is not displayed. Instead only the link to open the activity chooser is there (but not as a link). (See screenshot)

          Does not really matter as you are apparently fixing this in your patch, but just so you know.

          Show
          Frédéric Massart added a comment - Hi Andrew, not sure we understood each other here. When I disable both JavaScript AND the activity chooser, the dropdown menu to add resource or activity is not displayed. Instead only the link to open the activity chooser is there (but not as a link). (See screenshot) Does not really matter as you are apparently fixing this in your patch, but just so you know.
          Hide
          Andrew Nicols added a comment -

          On second thoughts, I think that my original idea of making the 'Add an activity or resource' text into a link which disables the activity checker is a bad idea in terms of usability. If a page is slow in loading then the link will not be converted for a while and when the user goes to use it, they will disable the chooser. There won't then be an obvious way to re-enable the checker.

          I've updated the patch with a CSS-only fix and have updated the testing instructions accordingly.

          Show
          Andrew Nicols added a comment - On second thoughts, I think that my original idea of making the 'Add an activity or resource' text into a link which disables the activity checker is a bad idea in terms of usability. If a page is slow in loading then the link will not be converted for a while and when the user goes to use it, they will disable the chooser. There won't then be an obvious way to re-enable the checker. I've updated the patch with a CSS-only fix and have updated the testing instructions accordingly.
          Hide
          Andrew Nicols added a comment -

          On second thoughts, I think that my original idea of making the 'Add an activity or resource' text into a link which disables the activity checker is a bad idea in terms of usability. If a page is slow in loading then the link will not be converted for a while and when the user goes to use it, they will disable the chooser. There won't then be an obvious way to re-enable the checker.

          I've updated the patch with a CSS-only fix and have updated the testing instructions accordingly.

          Show
          Andrew Nicols added a comment - On second thoughts, I think that my original idea of making the 'Add an activity or resource' text into a link which disables the activity checker is a bad idea in terms of usability. If a page is slow in loading then the link will not be converted for a while and when the user goes to use it, they will disable the chooser. There won't then be an obvious way to re-enable the checker. I've updated the patch with a CSS-only fix and have updated the testing instructions accordingly.
          Hide
          Andrew Nicols added a comment -

          Added a branch for stable too

          Show
          Andrew Nicols added a comment - Added a branch for stable too
          Hide
          Frédéric Massart added a comment -

          Hi Andrew,

          could you explain the use of contradictory class names? I find it a bit confusing as it basically does:

          • Hide if true OR false
          • Show if true OR false

          Shouldn't we get rid of the class name at all then? Just saying, I guess you had your reasons but I don't get them .

          Cheers!

          Show
          Frédéric Massart added a comment - Hi Andrew, could you explain the use of contradictory class names? I find it a bit confusing as it basically does: Hide if true OR false Show if true OR false Shouldn't we get rid of the class name at all then? Just saying, I guess you had your reasons but I don't get them . Cheers!
          Hide
          Andrew Nicols added a comment -

          Hi Fred,

          Sorry - it's not the most obvious of logic. It too me quite a while to work it out in the first place so I should have made it clearer.

          The problem at present is that we have two classes:

          • hiddenifjs
          • visibleifjs

          These will perform one thing when the jsenabled class is present (added by an early JS call), but the opposite if the jsenabled class is absent. So we get the following states:

            jsenabled jsdisabled
          hiddenifjs node is hidden node is visible
          visibleifjs node is visible node is hidden

          This is where out breakage occurs. When the activity chooser is disabled, we hide by giving it the hiddenifjs class, and the dropdowns the visibleifjs class. As a result, when JS is disabled, their roles are reversed.

          The change I've suggested is to add some additional classes to force the activity chooser to remain hidden even if JS is disabled and to force the dropdowns to show. So the possible states are then:

            jsenabled jsdisabled
          hiddenifjs node is hidden node is visible
          visibleifjs node is visible node is hidden
          hiddenifjs.hiddenifnojs node is hidden node is hidden
          visibleifjs.visibleifnojs node is visible node is visible

          We only need to apply these in the case that the Activity Chooser is disabled.

          The idea is to cater for that fallback where you've turned off the shiny AJAX feature and need it to remain turned off if JS is not available - e.g. to continue hiding if there is no JS, and to continue showing if there is no JS.

          So when the Activity Chooser is enabled, it has the class visibleifjs. When JS is enabled it is shown; when JS is disabled it is hidden.
          When the Activity Chooser is disabled, it has the classes hiddenifjs and hiddenifnojs. When JS is enabled it his hidden by the hiddenifjs logic; when JS is disabled it is hidden by the hiddenifnojs logic.

          Hope this clarifies things a little - I can try and improve the comments in the CSS if that helps?

          Show
          Andrew Nicols added a comment - Hi Fred, Sorry - it's not the most obvious of logic. It too me quite a while to work it out in the first place so I should have made it clearer. The problem at present is that we have two classes: hiddenifjs visibleifjs These will perform one thing when the jsenabled class is present (added by an early JS call), but the opposite if the jsenabled class is absent. So we get the following states:   jsenabled jsdisabled hiddenifjs node is hidden node is visible visibleifjs node is visible node is hidden This is where out breakage occurs. When the activity chooser is disabled, we hide by giving it the hiddenifjs class, and the dropdowns the visibleifjs class. As a result, when JS is disabled, their roles are reversed. The change I've suggested is to add some additional classes to force the activity chooser to remain hidden even if JS is disabled and to force the dropdowns to show. So the possible states are then:   jsenabled jsdisabled hiddenifjs node is hidden node is visible visibleifjs node is visible node is hidden hiddenifjs.hiddenifnojs node is hidden node is hidden visibleifjs.visibleifnojs node is visible node is visible We only need to apply these in the case that the Activity Chooser is disabled. The idea is to cater for that fallback where you've turned off the shiny AJAX feature and need it to remain turned off if JS is not available - e.g. to continue hiding if there is no JS, and to continue showing if there is no JS. So when the Activity Chooser is enabled, it has the class visibleifjs. When JS is enabled it is shown; when JS is disabled it is hidden. When the Activity Chooser is disabled, it has the classes hiddenifjs and hiddenifnojs. When JS is enabled it his hidden by the hiddenifjs logic; when JS is disabled it is hidden by the hiddenifnojs logic. Hope this clarifies things a little - I can try and improve the comments in the CSS if that helps?
          Hide
          Andrew Nicols added a comment -

          Oh and the reason we don't get rid of the class and we combine them is that we need to remove the with JS. So when JS is enabled, but the activity chooser is disable, you can still use the 'Turn activity chooser on' link to remove the hiddenifjs and add the visibleifjs class to the chooser. As a result, the hiddenifjs.hiddenifnojs rule ceases to apply and the visibleifjs rule applies instead.

          Show
          Andrew Nicols added a comment - Oh and the reason we don't get rid of the class and we combine them is that we need to remove the with JS. So when JS is enabled, but the activity chooser is disable, you can still use the 'Turn activity chooser on' link to remove the hiddenifjs and add the visibleifjs class to the chooser. As a result, the hiddenifjs.hiddenifnojs rule ceases to apply and the visibleifjs rule applies instead.
          Hide
          Frédéric Massart added a comment -

          Hi Andrew, thanks for this very thorough explanation! And I feel sorry but that still did not sound right to me (the use of two classes for one goal), so I took the liberty to have a look at course/lib.php. When the activity chooser is disabled we want, regardless of the JS, to hide it by default, and the dropdown to be shown right? So I tried the following, and it seem to work, but I don't know the area really well so I guess you'll be the only judge. If this is irrelevant, please submit your patch for integration!

          diff --git a/course/lib.php b/course/lib.php
          index 619f3e0..abe74ed 100644
          --- a/course/lib.php
          +++ b/course/lib.php
          @@ -1813,8 +1813,8 @@ function print_section_add_menus($course, $section, $modnames = null, $vertical=
                       $output = html_writer::tag('div', $output, array('class' => 'hiddenifjs addresourcedropdown'));
                       $modchooser = html_writer::tag('div', $modchooser, array('class' => 'visibleifjs addresourcemodchooser'));
                   } else {
          -            $output = html_writer::tag('div', $output, array('class' => 'visibleifjs addresourcedropdown'));
          -            $modchooser = html_writer::tag('div', $modchooser, array('class' => 'hiddenifjs addresourcemodchooser'));
          +            $output = html_writer::tag('div', $output, array('class' => 'show addresourcedropdown'));
          +            $modchooser = html_writer::tag('div', $modchooser, array('class' => 'hide addresourcemodchooser'));
                   }
                   $output = $modchooser . $output;
               }
          

          Thank you!
          Fred

          Show
          Frédéric Massart added a comment - Hi Andrew, thanks for this very thorough explanation! And I feel sorry but that still did not sound right to me (the use of two classes for one goal), so I took the liberty to have a look at course/lib.php. When the activity chooser is disabled we want, regardless of the JS, to hide it by default, and the dropdown to be shown right? So I tried the following, and it seem to work, but I don't know the area really well so I guess you'll be the only judge. If this is irrelevant, please submit your patch for integration! diff --git a/course/lib.php b/course/lib.php index 619f3e0..abe74ed 100644 --- a/course/lib.php +++ b/course/lib.php @@ -1813,8 +1813,8 @@ function print_section_add_menus($course, $section, $modnames = null, $vertical= $output = html_writer::tag('div', $output, array('class' => 'hiddenifjs addresourcedropdown')); $modchooser = html_writer::tag('div', $modchooser, array('class' => 'visibleifjs addresourcemodchooser')); } else { - $output = html_writer::tag('div', $output, array('class' => 'visibleifjs addresourcedropdown')); - $modchooser = html_writer::tag('div', $modchooser, array('class' => 'hiddenifjs addresourcemodchooser')); + $output = html_writer::tag('div', $output, array('class' => 'show addresourcedropdown')); + $modchooser = html_writer::tag('div', $modchooser, array('class' => 'hide addresourcemodchooser')); } $output = $modchooser . $output; } Thank you! Fred
          Hide
          Andrew Nicols added a comment -

          Hmm that seems to work too, but I think it works for a similar reason.

          On the chooser disabled, non-js version it works exactly as expected and is simpler than my solution. On the JS chooser disabled, js enabled version it works similar to my solution. The reason it works is because .jsenabled visibleifjs is a more specific class.

          I think that your solution is a little more elegant and much simpler to understand so we'll go with that. I'll submit an updated patch shortly.

          Thanks Fred!

          Andrew

          Show
          Andrew Nicols added a comment - Hmm that seems to work too, but I think it works for a similar reason. On the chooser disabled, non-js version it works exactly as expected and is simpler than my solution. On the JS chooser disabled, js enabled version it works similar to my solution. The reason it works is because .jsenabled visibleifjs is a more specific class. I think that your solution is a little more elegant and much simpler to understand so we'll go with that. I'll submit an updated patch shortly. Thanks Fred! Andrew
          Hide
          Andrew Nicols added a comment -

          Thanks - have updated the patch as discussed.

          Show
          Andrew Nicols added a comment - Thanks - have updated the patch as discussed.
          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
          Andrew Nicols added a comment -

          Both branches rebased and ready.

          Show
          Andrew Nicols added a comment - Both branches rebased and ready.
          Hide
          Dan Poltawski added a comment -

          Integrated, thanks guys!

          Show
          Dan Poltawski added a comment - Integrated, thanks guys!
          Hide
          Adrian Greeve added a comment -

          Tested on the 2.3 and master integration branches.
          When javascript is disabled the activity picker is not displayed and in-active. With JavaScript enabled or disabled, selecting an activity or resource is possible.
          Test passed.

          Show
          Adrian Greeve added a comment - Tested on the 2.3 and master integration branches. When javascript is disabled the activity picker is not displayed and in-active. With JavaScript enabled or disabled, selecting an activity or resource is possible. Test passed.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Amazed. Inspired. Grateful. That’s how your generosity makes me feel.

          (not really)

          Closing, thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Amazed. Inspired. Grateful. That’s how your generosity makes me feel. (not really) Closing, thanks!

            People

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

              Dates

              • Created:
                Updated:
                Resolved: