Moodle
  1. Moodle
  2. MDL-26979

custom menu :: IEs overwrite theme CSS with YUI CSS

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 2.0.2, 2.1.1, 2.2
    • Fix Version/s: 2.2
    • Component/s: Themes
    • Labels:
      None
    • Environment:
      Testet in IE8 and IE9
    • Testing Instructions:
      Hide
      1. Log in as an admin
      2. Browse to Settings > Appearance > Theme settings
      3. Enable theme change on URL
      4. Add a custom menu (just copy paste the example)
      5. Test each core theme in Firefox, Chrome + IE.
      6. Check that in each the custommenu looks consistent. It doesn't have to be perfect the aim of these changes is to simplify CSS and improve consistency.
      Show
      Log in as an admin Browse to Settings > Appearance > Theme settings Enable theme change on URL Add a custom menu (just copy paste the example) Test each core theme in Firefox, Chrome + IE. Check that in each the custommenu looks consistent. It doesn't have to be perfect the aim of these changes is to simplify CSS and improve consistency.
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE
    • Pull Master Branch:
      wip-MDL-26979-master
    • Rank:
      16608

      Description

      In the IE browsers CSS rules set in the theme are overwritten by the YUI CSS for the custom menu.

      You can see this bug in the "Fusion" Moodle 2 theme when you compare the menu in IE and for example Firefox.

      I tried to understand what happens but have had no luck. My guess is that the YUI CSS which is "lazy loaded" for the menu after the theme CSS is not correctly handled by IE. The link is placed before the theme CSS links but the CSS is rendered after the theme CSS overwriting the rules.

      I did not confirm but I guess the bug not only influences the custom menu but all "lazy loaded" YUI CSS in Moodle 2.

      The custom menu is not usable with this bug, therefore I have set the priority to critical.

        Issue Links

        Progress
        Resolved Sub-Tasks

        Sub-Tasks

        There are no Sub-Tasks for this issue.

          Activity

          Hide
          Matthew Cannings added a comment -

          I have the same problem. It was affecting a custom theme so I tried the included themes and have the issue on Fusion, Nimble. The issue is not in Arialist.

          Arialist theme has the reset CSS below. Which does stop the YUI appearing although I think you then have to specify these values further in your own CSS with !important again to override

          /*YUI Reset */
          /*full menu bar */
          .yui3-skin-sam .yui3-menu-horizontal .yui3-menu-content

          { background: none !important; }

          /*single items */
          .yui3-skin-sam .yui3-menu-horizontal .yui3-menu-label

          { background: none !important; }

          /*active items */
          .yui3-skin-sam .yui3-menu-horizontal .yui3-menu-label-active, .yui3-skin-sam .yui3-menu-horizontal .yui3-menuitem-active .yui3-menuitem-content, .yui3-skin-sam .yui3-menu-horizontal .yui3-menu-label-menuvisible

          { background: #f9f9f9 !important; border-color:inherit; }
          Show
          Matthew Cannings added a comment - I have the same problem. It was affecting a custom theme so I tried the included themes and have the issue on Fusion, Nimble. The issue is not in Arialist. Arialist theme has the reset CSS below. Which does stop the YUI appearing although I think you then have to specify these values further in your own CSS with !important again to override /*YUI Reset */ /*full menu bar */ .yui3-skin-sam .yui3-menu-horizontal .yui3-menu-content { background: none !important; } /*single items */ .yui3-skin-sam .yui3-menu-horizontal .yui3-menu-label { background: none !important; } /*active items */ .yui3-skin-sam .yui3-menu-horizontal .yui3-menu-label-active, .yui3-skin-sam .yui3-menu-horizontal .yui3-menuitem-active .yui3-menuitem-content, .yui3-skin-sam .yui3-menu-horizontal .yui3-menu-label-menuvisible { background: #f9f9f9 !important; border-color:inherit; }
          Hide
          Matthew Cannings added a comment -

          I have the same problem. It was affecting a custom theme so I tried the included themes and have the issue on Fusion, Nimble. The issue is not in Arialist.

          Arialist theme has the reset CSS below. Which does stop the YUI appearing although I think you then have to specify these values further in your own CSS with !important again to override

          /*YUI Reset */
          /*full menu bar */
          .yui3-skin-sam .yui3-menu-horizontal .yui3-menu-content

          { background: none !important; }

          /*single items */
          .yui3-skin-sam .yui3-menu-horizontal .yui3-menu-label

          { background: none !important; }

          /*active items */
          .yui3-skin-sam .yui3-menu-horizontal .yui3-menu-label-active, .yui3-skin-sam .yui3-menu-horizontal .yui3-menuitem-active .yui3-menuitem-content, .yui3-skin-sam .yui3-menu-horizontal .yui3-menu-label-menuvisible

          { background: #f9f9f9 !important; border-color:inherit; }
          Show
          Matthew Cannings added a comment - I have the same problem. It was affecting a custom theme so I tried the included themes and have the issue on Fusion, Nimble. The issue is not in Arialist. Arialist theme has the reset CSS below. Which does stop the YUI appearing although I think you then have to specify these values further in your own CSS with !important again to override /*YUI Reset */ /*full menu bar */ .yui3-skin-sam .yui3-menu-horizontal .yui3-menu-content { background: none !important; } /*single items */ .yui3-skin-sam .yui3-menu-horizontal .yui3-menu-label { background: none !important; } /*active items */ .yui3-skin-sam .yui3-menu-horizontal .yui3-menu-label-active, .yui3-skin-sam .yui3-menu-horizontal .yui3-menuitem-active .yui3-menuitem-content, .yui3-skin-sam .yui3-menu-horizontal .yui3-menu-label-menuvisible { background: #f9f9f9 !important; border-color:inherit; }
          Hide
          Sam Hemelryk added a comment -

          Hi guys,

          I've just started looking into this now. The problem arises because the way that the custom menu is currently being initialised leads to the CSS being loaded into the page by JavaScript that executes after everything else is loaded, hence the styles don't apply as you'd expect in IE + other browsers.

          Having looked at the styles in nimble + fusion I can see that they are very confused as well.

          I'll improve the way in which the custom menu is initialised so that we can get the CSS included in the head and I'll then refactor the CSS for the custom menu in fusion, nimble and any other theme that has been negatively affected.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi guys, I've just started looking into this now. The problem arises because the way that the custom menu is currently being initialised leads to the CSS being loaded into the page by JavaScript that executes after everything else is loaded, hence the styles don't apply as you'd expect in IE + other browsers. Having looked at the styles in nimble + fusion I can see that they are very confused as well. I'll improve the way in which the custom menu is initialised so that we can get the CSS included in the head and I'll then refactor the CSS for the custom menu in fusion, nimble and any other theme that has been negatively affected. Cheers Sam
          Hide
          Sam Hemelryk added a comment -

          Hi Petr + everyone,

          I've got a couple of patches now that fix up this problem and tidy up the custom menu CSS in ALL theme's within Moodle.
          You can check them out at:

          1. master
          1. MOODLE_20_STABLE

          If there is anyone who would be kind enough to test these for me it would be most appreciated.

          Petr I would be particularly keen to hear what you think of the changes being made to the core renderer. It wraps a JS call to initialise the custom menu within an anonymous function that can be passed through the yui_module method. It's pretty bang on to what is required and saves us having to somehow support YUI modules for core that don't relate to components (like custommenu).

          I should add I have tested the modified themes in Firefox 4, Chrome 12, and IE8... I still need to test in IE7 although I will likely file separate MDL issues to correct any themes that need major work so that this goes in sooner rather than later.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Petr + everyone, I've got a couple of patches now that fix up this problem and tidy up the custom menu CSS in ALL theme's within Moodle. You can check them out at: master _Repository: _ git://github.com/samhemelryk/moodle.git _Branch: _ wip- MDL-26979 -master _Diff URL: _ https://github.com/samhemelryk/moodle/compare/master...wip-MDL-26979-master MOODLE_20_STABLE _Repository: _ git://github.com/samhemelryk/moodle.git _Branch: _ wip- MDL-26979 -MOODLE_20_STABLE _Diff URL: _ https://github.com/samhemelryk/moodle/compare/MOODLE_20_STABLE...wip-MDL-26979-MOODLE_20_STABLE If there is anyone who would be kind enough to test these for me it would be most appreciated. Petr I would be particularly keen to hear what you think of the changes being made to the core renderer. It wraps a JS call to initialise the custom menu within an anonymous function that can be passed through the yui_module method. It's pretty bang on to what is required and saves us having to somehow support YUI modules for core that don't relate to components (like custommenu). I should add I have tested the modified themes in Firefox 4, Chrome 12, and IE8... I still need to test in IE7 although I will likely file separate MDL issues to correct any themes that need major work so that this goes in sooner rather than later. Cheers Sam
          Hide
          Gabriel Alonso Vilches added a comment -

          Hi guys.

          Thank you for your job but i have some problems with this.

          I have installed the moodle patch of above (made by Sam) because i think that my problem is near to that one, but i still have some probs.

          The principal menu start to work with that version but the css rules are still overwrited. I can see that is the problem because in first charge the IE 6 get the css good, and some seconds away its totally different. Due i have changed some parts of my css (of darkblue theme) theres like 2 menus, one in front of the other, and mine is in the back, overwrited. The colour combination made it almost imposible to read and i can see some diferent colours on the pixels of the parts where i have different size than the normal

          Maybe thats not the problem you was solving, but i think my problem is YUI too, and happens with all the themes.

          Regards Gabriel.

          Show
          Gabriel Alonso Vilches added a comment - Hi guys. Thank you for your job but i have some problems with this. I have installed the moodle patch of above (made by Sam) because i think that my problem is near to that one, but i still have some probs. The principal menu start to work with that version but the css rules are still overwrited. I can see that is the problem because in first charge the IE 6 get the css good, and some seconds away its totally different. Due i have changed some parts of my css (of darkblue theme) theres like 2 menus, one in front of the other, and mine is in the back, overwrited. The colour combination made it almost imposible to read and i can see some diferent colours on the pixels of the parts where i have different size than the normal Maybe thats not the problem you was solving, but i think my problem is YUI too, and happens with all the themes. Regards Gabriel.
          Hide
          Sam Hemelryk added a comment -

          Hi guys,

          I finally found a moment to finish off my work on this and I'm after a peer-review for this now so that I can start it on the path to integration.

          Gabriel it sounds like you are experiencing another problem. The custom menu changing a second or two into page load is because that is when the JavaScript is initialising.
          We no longer support IE6 so I can't be sure whether things will or will not work within it.
          Certainly if you are not using IE6 and are still experiencing problems let me know and we can look to create a new issue with specific details about what you are seeing.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi guys, I finally found a moment to finish off my work on this and I'm after a peer-review for this now so that I can start it on the path to integration. Gabriel it sounds like you are experiencing another problem. The custom menu changing a second or two into page load is because that is when the JavaScript is initialising. We no longer support IE6 so I can't be sure whether things will or will not work within it. Certainly if you are not using IE6 and are still experiencing problems let me know and we can look to create a new issue with specific details about what you are seeing. Cheers Sam
          Hide
          Andrew Davis added a comment - - edited

          I'm not quite sure how to review several hundred lines of CSS changes...

          Sam, what was the reason for switching from this...

          $this->page->requires->js_init_call('M.core_custom_menu.init', array('custom_menu_'.$menucount));
          

          to this...

          // Initialise this custom menu (the custom menu object is contained in javascript-static
          $jscode = js_writer::function_call_with_Y('M.core_custom_menu.init', array('custom_menu_'.$menucount));
          $jscode = "(function(){{$jscode}})";
          $this->page->requires->yui_module('node-menunav', $jscode);
          

          Is it to get M.core_custom_menu.init() to execute earlier? Provided the answer to that is reasonably sensible +1.

          Show
          Andrew Davis added a comment - - edited I'm not quite sure how to review several hundred lines of CSS changes... Sam, what was the reason for switching from this... $ this ->page->requires->js_init_call('M.core_custom_menu.init', array('custom_menu_'.$menucount)); to this... // Initialise this custom menu (the custom menu object is contained in javascript- static $jscode = js_writer::function_call_with_Y('M.core_custom_menu.init', array('custom_menu_'.$menucount)); $jscode = "(function(){{$jscode}})" ; $ this ->page->requires->yui_module('node-menunav', $jscode); Is it to get M.core_custom_menu.init() to execute earlier? Provided the answer to that is reasonably sensible +1.
          Hide
          Sam Hemelryk added a comment -

          Hehe good question!

          The answer is actually quite cool, the code as it would call a method in Javascript-static which would cause the browser to request the node-menunav code from the server and when that had been loaded would initialise the custom menu.
          The new code ensure that the node-menunav code will be included with the initial page request. This way (providing yui combo loading is on) there will be one less http request made by the browser, and more importantly because the code is made available initially rather than through an additional request the whole custommenu is initialised quicker reducing the visible flicker as it loads.

          Hopefully that is satisfactory enough, in which case I'll put this up for integration

          Show
          Sam Hemelryk added a comment - Hehe good question! The answer is actually quite cool, the code as it would call a method in Javascript-static which would cause the browser to request the node-menunav code from the server and when that had been loaded would initialise the custom menu. The new code ensure that the node-menunav code will be included with the initial page request. This way (providing yui combo loading is on) there will be one less http request made by the browser, and more importantly because the code is made available initially rather than through an additional request the whole custommenu is initialised quicker reducing the visible flicker as it loads. Hopefully that is satisfactory enough, in which case I'll put this up for integration
          Hide
          Sam Hemelryk added a comment -

          Rebased now

          Show
          Sam Hemelryk added a comment - Rebased now
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Ho Sam,

          I'm going to integrate this only for master, after fixing a bunch of whitespace problems. Then I'll create followup about to consider backporting this to 21_STABLE and 20_STABLE, I really don't feel confident adding it straight to the stable branches. Let's talk @ MDL-28941 (the followup).

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Ho Sam, I'm going to integrate this only for master, after fixing a bunch of whitespace problems. Then I'll create followup about to consider backporting this to 21_STABLE and 20_STABLE, I really don't feel confident adding it straight to the stable branches. Let's talk @ MDL-28941 (the followup). Ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          This has been integrated in master with extra commit cleaning exclusively the whitespace introduced by the patch (all the themes continue plagued of whitespace).

          Extensive testing for this under all browsers is required. Let's continue @ MDL-28941 for stable branches.

          Show
          Eloy Lafuente (stronk7) added a comment - This has been integrated in master with extra commit cleaning exclusively the whitespace introduced by the patch (all the themes continue plagued of whitespace). Extensive testing for this under all browsers is required. Let's continue @ MDL-28941 for stable branches.
          Hide
          Rossiani Wijaya added a comment -

          Hi Sam,

          The master patch works great with the following browsers: Chrome v.13, firefox v.4, IE7, IE8 and IE9.

          The patch also fixed the issue in fusion theme.

          However, when tested it with nimble theme, I noticed the arrow image that used to indicate sub-content is not display (also tested by changing the background color). This issue occurs in all tested browsers.

          Show
          Rossiani Wijaya added a comment - Hi Sam, The master patch works great with the following browsers: Chrome v.13, firefox v.4, IE7, IE8 and IE9. The patch also fixed the issue in fusion theme. However, when tested it with nimble theme, I noticed the arrow image that used to indicate sub-content is not display (also tested by changing the background color). This issue occurs in all tested browsers.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Hi Rossie,

          good hunt! I'd propose (as far as it's "tangencial") to create a follow-up issue with the Nimble problem and pass this.

          TIA and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Hi Rossie, good hunt! I'd propose (as far as it's "tangencial") to create a follow-up issue with the Nimble problem and pass this. TIA and ciao
          Hide
          Rossiani Wijaya added a comment -

          Test passed.

          I will create new issue regarding image display for Nimble theme and link it to this issue.

          Show
          Rossiani Wijaya added a comment - Test passed. I will create new issue regarding image display for Nimble theme and link it to this issue.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Such an effort has get its prize. Universal Award to the very-best Moodle developer in the family. Thanks!

          Closing. Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Such an effort has get its prize. Universal Award to the very-best Moodle developer in the family. Thanks! Closing. Ciao
          Hide
          Kris Besley added a comment -

          Has this been fixed in Moodle 2.2 or can I get a patch for it please? Above patch links are dead

          Show
          Kris Besley added a comment - Has this been fixed in Moodle 2.2 or can I get a patch for it please? Above patch links are dead
          Hide
          Sam Hemelryk added a comment -

          Hi Kris, this has indeed been fixed in Moodle 2.2.
          If you find that you are still affected by this issue could you please open a new issue.
          Versions 2.0.5+, 2.1.2+, 2.2 and any version higher than 2.2 should be fixed.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Kris, this has indeed been fixed in Moodle 2.2. If you find that you are still affected by this issue could you please open a new issue. Versions 2.0.5+, 2.1.2+, 2.2 and any version higher than 2.2 should be fixed. Cheers Sam
          Hide
          Kris Besley added a comment -

          Hi Sam,

          Thanks for getting back to me. I have opened a new issue here: http://tracker.moodle.org/browse/MDL-32486

          Just out of interest, how was this corrected? Can you make the patch available for me?

          Thanks,

          Kris

          Show
          Kris Besley added a comment - Hi Sam, Thanks for getting back to me. I have opened a new issue here: http://tracker.moodle.org/browse/MDL-32486 Just out of interest, how was this corrected? Can you make the patch available for me? Thanks, Kris
          Hide
          Sam Hemelryk added a comment -

          Hi Kris,
          My apologies, I just found MDL-28941, when this issue was integrated it was only integrated on master, and the integrator created a separate issue to backport it to 2.1 and the fix versions weren't updated. Consequently Moodle 2.2 and greater have the fix, but 2.1 and 2.0 do not.

          I've attached the patch that was used for master, however it contains quite a lot of whitespace fix as well so is rather large.
          Perhaps it is best to push MDL-28941 to get this fixed in 2.1 if that is what you require.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Kris, My apologies, I just found MDL-28941 , when this issue was integrated it was only integrated on master, and the integrator created a separate issue to backport it to 2.1 and the fix versions weren't updated. Consequently Moodle 2.2 and greater have the fix, but 2.1 and 2.0 do not. I've attached the patch that was used for master, however it contains quite a lot of whitespace fix as well so is rather large. Perhaps it is best to push MDL-28941 to get this fixed in 2.1 if that is what you require. Cheers Sam

            People

            • Votes:
              3 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: