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

      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.

        Gliffy Diagrams

          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: