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
    • Rank:
      29385

      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.

        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
        Robert 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
        Robert 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 Škoda added a comment -

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

        Show
        Petr Škoda 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: