Moodle
  1. Moodle
  2. MDL-10201

The hide/show eye icon on labels is incorrect when AJAX is enabled

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.8, 1.8.1, 1.9.2
    • Fix Version/s: 1.8.9, 1.9.5
    • Component/s: AJAX and JavaScript
    • Labels:
      None
    • Environment:
      Any
    • Affected Branches:
      MOODLE_18_STABLE, MOODLE_19_STABLE
    • Fixed Branches:
      MOODLE_18_STABLE, MOODLE_19_STABLE

      Description

      The hide/show eye icon on labels always shows as open if AJAX is enabled.

      To reproduce:

      Make sure AJAX is enabled.
      Turn editing on.
      Create a label.
      Hide the label by clicking the eye icon, or by going to the update label screen and setting the visible property to hide.
      Navigate back to the course page (or refresh it) - the eye is now open again.

      Note that the label is actually hidden from students - it is just that the eye icon is incorrect, leading teachers to believe that the label is not hidden when it really is.

        Gliffy Diagrams

          Activity

          Hide
          Ann Adamcik added a comment -

          Other resources are links, with 'class="dimmed"' if they are hidden. Labels are spans, with 'class="dimmed_text"' if they are hidden. The AJAX code only checks links when determining whether or not the eye icon should be closed. It needs to check for dimmed_text spans as well. Here's a patch for 1.8+ and 1.9 -

          RCS file: /cvsroot/moodle/moodle/lib/ajax/section_classes.js,v
          retrieving revision 1.32.2.2
          diff -c -r1.32.2.2 section_classes.js

              • section_classes.js 27 Aug 2007 17:27:00 -0000 1.32.2.2
              • section_classes.js 6 Nov 2007 14:21:19 -0000
                ***************
              • 516,522 ****
                this.id = this.getEl().id.replace(/module-/i, '');

          this.hidden = false;
          ! if (YAHOO.util.Dom.hasClass(this.getEl().getElementsByTagName('a')[0], 'dimmed'))

          { this.hidden = true; }
          this.hiddenStored = null;
          — 516,522 ----
          this.id = this.getEl().id.replace(/module-/i, '');

          this.hidden = false;
          ! if (YAHOO.util.Dom.hasClass(this.getEl().getElementsByTagName('a')[0], 'dimmed') || YAHOO.util.Dom.hasClass(this.getEl().getElementsByTagName('span')[0], 'dimmed_text')) { this.hidden = true; }

          this.hiddenStored = null;

          Show
          Ann Adamcik added a comment - Other resources are links, with 'class="dimmed"' if they are hidden. Labels are spans, with 'class="dimmed_text"' if they are hidden. The AJAX code only checks links when determining whether or not the eye icon should be closed. It needs to check for dimmed_text spans as well. Here's a patch for 1.8+ and 1.9 - RCS file: /cvsroot/moodle/moodle/lib/ajax/section_classes.js,v retrieving revision 1.32.2.2 diff -c -r1.32.2.2 section_classes.js section_classes.js 27 Aug 2007 17:27:00 -0000 1.32.2.2 section_classes.js 6 Nov 2007 14:21:19 -0000 *************** 516,522 **** this.id = this.getEl().id.replace(/module-/i, ''); this.hidden = false; ! if (YAHOO.util.Dom.hasClass(this.getEl().getElementsByTagName('a') [0] , 'dimmed')) { this.hidden = true; } this.hiddenStored = null; — 516,522 ---- this.id = this.getEl().id.replace(/module-/i, ''); this.hidden = false; ! if (YAHOO.util.Dom.hasClass(this.getEl().getElementsByTagName('a') [0] , 'dimmed') || YAHOO.util.Dom.hasClass(this.getEl().getElementsByTagName('span') [0] , 'dimmed_text')) { this.hidden = true; } this.hiddenStored = null;
          Hide
          Markus Hillig added a comment -

          Works perfectly in Moodle 1.8.3+. Thanks for the fix, Ann.

          Show
          Markus Hillig added a comment - Works perfectly in Moodle 1.8.3+. Thanks for the fix, Ann.
          Hide
          Martin Dougiamas added a comment -

          Nicolas can you review and merge this please?

          Show
          Martin Dougiamas added a comment - Nicolas can you review and merge this please?
          Hide
          Kristian Thornley added a comment -

          Looks like this fix works partially on Moodle 1.9.2.+

          Problem now is the style changes but this is not reflected in the DOM

          proposed solution:

          change <<Moodle Root>>/course/lib.php and make labels have the class dimmed if they are hidden and always wrapped in a div

          replace lines 1350 - 1356 with
          echo "<div ";
          if (!$mod->visible)

          { echo "class='dimmed' "; }

          echo ">";
          echo format_text($extra, FORMAT_HTML, $labelformatoptions);
          echo "</div>";

          then modify css for theme so that div's with dimmed are greyed out e.g.

          a.dimmed:link,
          a.dimmed:visited,
          div.dimmed

          { color:#AAAAAA; }

          then modify and insert following code round about line 537 of <<Moodle Root>>/lib/ajax/section_classes.js (look for this line this.linkContainer = this.getEl().getElementsByTagName('a')[0]

          if (YAHOO.util.Dom.hasClass(this.getEl().getElementsByTagName('div')[0], 'dimmed'))

          { this.hidden = true; }

          if(this.linkContainer.parentNode.className == 'commands')

          { this.linkContainer = this.getEl().getElementsByTagName('div')[0]; }
          Show
          Kristian Thornley added a comment - Looks like this fix works partially on Moodle 1.9.2.+ Problem now is the style changes but this is not reflected in the DOM proposed solution: change <<Moodle Root>>/course/lib.php and make labels have the class dimmed if they are hidden and always wrapped in a div replace lines 1350 - 1356 with echo "<div "; if (!$mod->visible) { echo "class='dimmed' "; } echo ">"; echo format_text($extra, FORMAT_HTML, $labelformatoptions); echo "</div>"; then modify css for theme so that div's with dimmed are greyed out e.g. a.dimmed:link, a.dimmed:visited, div.dimmed { color:#AAAAAA; } then modify and insert following code round about line 537 of <<Moodle Root>>/lib/ajax/section_classes.js (look for this line this.linkContainer = this.getEl().getElementsByTagName('a') [0] if (YAHOO.util.Dom.hasClass(this.getEl().getElementsByTagName('div') [0] , 'dimmed')) { this.hidden = true; } if(this.linkContainer.parentNode.className == 'commands') { this.linkContainer = this.getEl().getElementsByTagName('div')[0]; }
          Hide
          Bob Puffer added a comment -

          This is still an issue in 1.92+ (20080711) and I'd definitely consider it major. Not only does it display the incorrect state of the "eye" but it disallows instructors from making a resource visible once it takes on an invisible state with the eye opened. A patch would be great.

          Show
          Bob Puffer added a comment - This is still an issue in 1.92+ (20080711) and I'd definitely consider it major. Not only does it display the incorrect state of the "eye" but it disallows instructors from making a resource visible once it takes on an invisible state with the eye opened. A patch would be great.
          Hide
          Markus Hillig added a comment -

          For what it's worth it: for Moodle 1.9.2+ I had to adapt Ann's above patch from 07/Nov/07 since now dimmed text gets enclosed between div tags on no longer between span tags. This is what I did. For me it works so far.

          [root@ecampus html]# diff -c moodle/lib/ajax/section_classes.js.dist moodle/lib/ajax/section_classes.js

              • moodle/lib/ajax/section_classes.js.dist 2008-07-19 02:17:39.000000000 +0200
              • moodle/lib/ajax/section_classes.js 2008-09-26 16:14:42.000000000 +0200
                ***************
              • 526,532 ****
                this.id = this.getEl().id.replace(/module-/i, '');

          this.hidden = false;
          ! if (YAHOO.util.Dom.hasClass(this.getEl().getElementsByTagName('a')[0], 'dimmed'))

          { this.hidden = true; }
          this.hiddenStored = null;
          — 526,532 ----
          this.id = this.getEl().id.replace(/module-/i, '');

          this.hidden = false;
          ! if (YAHOO.util.Dom.hasClass(this.getEl().getElementsByTagName('a')[0], 'dimmed') || YAHOO.util.Dom.hasClass(this.getEl().getElementsByTagName('div')[0], 'dimmed_text')) { this.hidden = true; }

          this.hiddenStored = null;

          Show
          Markus Hillig added a comment - For what it's worth it: for Moodle 1.9.2+ I had to adapt Ann's above patch from 07/Nov/07 since now dimmed text gets enclosed between div tags on no longer between span tags. This is what I did. For me it works so far. [root@ecampus html] # diff -c moodle/lib/ajax/section_classes.js.dist moodle/lib/ajax/section_classes.js moodle/lib/ajax/section_classes.js.dist 2008-07-19 02:17:39.000000000 +0200 moodle/lib/ajax/section_classes.js 2008-09-26 16:14:42.000000000 +0200 *************** 526,532 **** this.id = this.getEl().id.replace(/module-/i, ''); this.hidden = false; ! if (YAHOO.util.Dom.hasClass(this.getEl().getElementsByTagName('a') [0] , 'dimmed')) { this.hidden = true; } this.hiddenStored = null; — 526,532 ---- this.id = this.getEl().id.replace(/module-/i, ''); this.hidden = false; ! if (YAHOO.util.Dom.hasClass(this.getEl().getElementsByTagName('a') [0] , 'dimmed') || YAHOO.util.Dom.hasClass(this.getEl().getElementsByTagName('div') [0] , 'dimmed_text')) { this.hidden = true; } this.hiddenStored = null;
          Hide
          Nicolas Connault added a comment -

          Applied patch to 1.9.3 and HEAD. Works fine.

          Show
          Nicolas Connault added a comment - Applied patch to 1.9.3 and HEAD. Works fine.
          Hide
          Jérôme Mouneyrac added a comment - - edited

          It works however when I hide the label, the label only becomes grey when I reload the page. Tested on 1.9 with AJAX activated.

          Show
          Jérôme Mouneyrac added a comment - - edited It works however when I hide the label, the label only becomes grey when I reload the page. Tested on 1.9 with AJAX activated.
          Hide
          Jérôme Mouneyrac added a comment -

          I change the priority issue as I confirm that the initial problem has been fixed.

          Show
          Jérôme Mouneyrac added a comment - I change the priority issue as I confirm that the initial problem has been fixed.
          Hide
          Jérôme Mouneyrac added a comment -

          Finally I changed back to Major as the problem still exists on 1.8

          Show
          Jérôme Mouneyrac added a comment - Finally I changed back to Major as the problem still exists on 1.8
          Hide
          Nicolas Connault added a comment -

          Finally backported this fix to 1.8

          Show
          Nicolas Connault added a comment - Finally backported this fix to 1.8
          Hide
          Petr Skoda added a comment -

          reopening the label itself is not grayed out after clicking the eye icon

          Show
          Petr Skoda added a comment - reopening the label itself is not grayed out after clicking the eye icon
          Hide
          Nicolas Connault added a comment -

          Finally fixed!

          Show
          Nicolas Connault added a comment - Finally fixed!
          Hide
          Rossiani Wijaya added a comment -

          Tested and works.

          Resolved.

          Closing.

          Show
          Rossiani Wijaya added a comment - Tested and works. Resolved. Closing.

            People

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

              Dates

              • Created:
                Updated:
                Resolved: