Moodle
  1. Moodle
  2. MDL-38441

Style sheet system creates files with > 4095 selectors

    Details

    • Testing Instructions:
      Hide

      This is an amazingly hard issue to test with confidence. IE stops displaying any styles after the 4095'th in a stylesheet.
      In order to test properly you would need to find a theme with stylesheets containing more than 4095 selectors when combined.
      You'd then need to identify styles being applied to selectors > 4095 and test in IE that with this patch applied those styles are shown.

      To test in best faith you could follow through:

      1. Run the unit tests.
      2. Log in as an admin, ensure theme designer mode is off and then purge your caches.
      3. Using Firefox/Chrome/Safari browse around the site and check things look as expected. Repeat for a couple of themes including standard, bootstrap and a theme based upon bootstrap if there is one yet.
      4. Switch to IE browse around the site and check things look as expected. Repeat for the same themes as above.
      Show
      This is an amazingly hard issue to test with confidence. IE stops displaying any styles after the 4095'th in a stylesheet. In order to test properly you would need to find a theme with stylesheets containing more than 4095 selectors when combined. You'd then need to identify styles being applied to selectors > 4095 and test in IE that with this patch applied those styles are shown. To test in best faith you could follow through: Run the unit tests. Log in as an admin, ensure theme designer mode is off and then purge your caches. Using Firefox/Chrome/Safari browse around the site and check things look as expected. Repeat for a couple of themes including standard, bootstrap and a theme based upon bootstrap if there is one yet. Switch to IE browse around the site and check things look as expected. Repeat for the same themes as above.
    • Workaround:
      Hide

      Use a better browser.

      This is a joke btw, I understand some are stuck using IE and can't escape its undeniable glory.
      To those in this boat, sorry.

      Show
      Use a better browser. This is a joke btw, I understand some are stuck using IE and can't escape its undeniable glory. To those in this boat, sorry.
    • Affected Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull Master Branch:
      wip-MDL-38441-m25
    • Rank:
      48379

      Description

      There's a little known bug in IE 6,7,8,9 that means if you have more than 4095 selectors in a single CSS file then the following style rules will be simply ignored.

      Moodle's styles.php puts all the styles from the theme and modules into a single file, following the best practice of the web to reduce download times and increase rendering times.

      But it doesn't do anything if Themes go over this limit. Simply splitting the CSS into two files is enough to work around this bug.

      A test file that shows the bug:
      http://marc.baffl.co.uk/browser_bugs/css-selector-limit/index.html

      A tool to check how many selectors in a CSS file via upload or url:

      http://snippet.bevey.com/css/selectorCount.php

      And some selector counts from standard themes:

      Standard: 4649
      Arialist: 4219
      Moodle.org: 6031

      So it looks like nearly every Moodle theme has sections that are simply ignored on IE < 10, and Moodle.org has nearly 2000 selectors, almost a third of the total, that are being ignored.

      If you were to use just the "Base" theme on it's own (not possible by default) then it, in conjunction with the module CSS would use 3720 selectors, leaving room for 376 selectors for any theme descending from Base, assuming is not excluding any parent theme sheets, before you hit this limit.

        Issue Links

          Activity

          Hide
          Matteo Scaramuccia added a comment -

          Nice and hidden bug!
          In the second link provided by David Scotson, there is the MSDN reference for this issue: http://msdn.microsoft.com/en-us/library/aa358796%28VS.85%29.aspx, see the Remarks section.

          Show
          Matteo Scaramuccia added a comment - Nice and hidden bug! In the second link provided by David Scotson , there is the MSDN reference for this issue: http://msdn.microsoft.com/en-us/library/aa358796%28VS.85%29.aspx , see the Remarks section.
          Hide
          Stuart Lamour added a comment -

          That is a really well documented issue with ie that anyone worth their salt doing css knows about (or should do!). Easily avoided using oocss/html patterns.

          We told Martin about this at it moot in dublin https://twitter.com/lexx_koto/status/304632389907382272

          Video is very much worth watching, and the only way i know to realistically solve this issue for now and the future with such a large system as moodle.

          Show
          Stuart Lamour added a comment - That is a really well documented issue with ie that anyone worth their salt doing css knows about (or should do!). Easily avoided using oocss/html patterns. We told Martin about this at it moot in dublin https://twitter.com/lexx_koto/status/304632389907382272 Video is very much worth watching, and the only way i know to realistically solve this issue for now and the future with such a large system as moodle.
          Hide
          Mary Evans added a comment -

          I know Sam is aware of the way IE loads/doesn't load all CSS and as far as I know there has been some fix added. But I may be wrong.

          Show
          Mary Evans added a comment - I know Sam is aware of the way IE loads/doesn't load all CSS and as far as I know there has been some fix added. But I may be wrong.
          Hide
          Paul Vaughan added a comment -

          Just a quick not for anyone looking at this.

          I've used the above test on IE 8 (our default browser) and received a red bar, however I've tested it in IE10 using IE7, IE8 and IE9 rendering modes with both strict and quirks mode, and get nothing but green bars for everything (so assume that this bug does not relate to the various 'older' rendering modes within IE 10, but does affect those specific versions of those browsers).

          Show
          Paul Vaughan added a comment - Just a quick not for anyone looking at this. I've used the above test on IE 8 (our default browser) and received a red bar, however I've tested it in IE10 using IE7, IE8 and IE9 rendering modes with both strict and quirks mode, and get nothing but green bars for everything (so assume that this bug does not relate to the various 'older' rendering modes within IE 10, but does affect those specific versions of those browsers).
          Hide
          David Scotson added a comment -

          @Stuart,

          simply splitting the CSS file into two separate ones will work in the short term.

          Reducing the size of Moodle's CSS has plenty of other benefits though (like, understandability for people working on the front-end, as well as download times on slow connections and rendering times on low powered-mobile devices). And focussing on standardized HTML is the key to reducing the CSS required, which again has many spin-off benefits (like, understandabality for people working on the front-end, more consistent UI etc.)

          Show
          David Scotson added a comment - @Stuart, simply splitting the CSS file into two separate ones will work in the short term. Reducing the size of Moodle's CSS has plenty of other benefits though (like, understandability for people working on the front-end, as well as download times on slow connections and rendering times on low powered-mobile devices). And focussing on standardized HTML is the key to reducing the CSS required, which again has many spin-off benefits (like, understandabality for people working on the front-end, more consistent UI etc.)
          Hide
          Mary Evans added a comment -

          This is a known bug as can be seen in MDL-22353

          Show
          Mary Evans added a comment - This is a known bug as can be seen in MDL-22353
          Hide
          Mary Evans added a comment -

          David Amy Groshek is on to this already. We are wanting to move to implementing 00CSS, there are some tracker issues which I will add as links.

          Show
          Mary Evans added a comment - David Amy Groshek is on to this already. We are wanting to move to implementing 00CSS, there are some tracker issues which I will add as links.
          Hide
          David Scotson added a comment -

          Hi Mary,

          that MDL-22353 bug (although it mentions selector limits in comments) seems to have been about CSS taking a long time to apply, possibly due to javascript, rather than not applying at all which is the case here, though the fact that it was only showing when theme developer mode was off makes it seem like it might have been related.

          Also, although OOCSS seems to be the accepted term, it might be worth emphasizing (as Stuart does above when mentioning the same concept) that the HTML needs to change to make this work. Changing the HTML in Moodle is a much, much trickier task than changing the CSS. In fact my primary goal for working on the Bootstrap theme has been to help move Moodle to more standardized HTML ("standardized" as in using the same HTML every time you use a checkbox or a cancel button or a pager or a form legend, not as in web "standards").

          Show
          David Scotson added a comment - Hi Mary, that MDL-22353 bug (although it mentions selector limits in comments) seems to have been about CSS taking a long time to apply, possibly due to javascript, rather than not applying at all which is the case here, though the fact that it was only showing when theme developer mode was off makes it seem like it might have been related. Also, although OOCSS seems to be the accepted term, it might be worth emphasizing (as Stuart does above when mentioning the same concept) that the HTML needs to change to make this work. Changing the HTML in Moodle is a much, much trickier task than changing the CSS. In fact my primary goal for working on the Bootstrap theme has been to help move Moodle to more standardized HTML ("standardized" as in using the same HTML every time you use a checkbox or a cancel button or a pager or a form legend, not as in web "standards").
          Hide
          Mary Evans added a comment -

          Obviously the HTML too, I appreciate that. And as you rightly say that is a bigger task. And I wish you every success with it too.

          Show
          Mary Evans added a comment - Obviously the HTML too, I appreciate that. And as you rightly say that is a bigger task. And I wish you every success with it too.
          Hide
          David Scotson added a comment -

          Hi Mary,

          I don't think it is that obvious that the HTML has to change to people not familiar with the concept of OOCSS. I think for many people "Implementing OOCSS" sounds easy, and more importantly "someone else's problem", whereas "rewriting all of Moodle's HTML to be consistent" is a bit more blatant about what needs to be done, how much work it is, and who's going to have to be involved for it to actually happen. If it succeeds it'll be because the whole of the Moodle community works together makes it happen, because if they don't then it won't.

          Show
          David Scotson added a comment - Hi Mary, I don't think it is that obvious that the HTML has to change to people not familiar with the concept of OOCSS. I think for many people "Implementing OOCSS" sounds easy, and more importantly "someone else's problem", whereas "rewriting all of Moodle's HTML to be consistent" is a bit more blatant about what needs to be done, how much work it is, and who's going to have to be involved for it to actually happen. If it succeeds it'll be because the whole of the Moodle community works together makes it happen, because if they don't then it won't.
          Hide
          Amy Groshek added a comment -

          David Scotson This is not bad news. This is great news!!! Because now all the developers who have been telling us that we're needlessly harping on OOCSS and that the markup doesn't have to be made more consistent now have a legitimate bug to chew on! This is worth undertaking, but first we need to give people clear instructions. I've already contributed my 2 cents. Let's hammer out a solid set of recommendations. Once we have that, we can worry about who's going to help us.

          Show
          Amy Groshek added a comment - David Scotson This is not bad news. This is great news!!! Because now all the developers who have been telling us that we're needlessly harping on OOCSS and that the markup doesn't have to be made more consistent now have a legitimate bug to chew on! This is worth undertaking, but first we need to give people clear instructions. I've already contributed my 2 cents. Let's hammer out a solid set of recommendations. Once we have that, we can worry about who's going to help us.
          Hide
          Bas Brands added a comment -

          I tried leaving out module css and only loading it when required to try and stay below 4096 selectors see: https://github.com/bmbrands/theme_bootstrap/commit/8e259d2ac0d89cc7560bed09b2da93e16348c467

          With fix I still have: 5377 selectors (loading module CSS only when required)

          Without fix I have: 6222 selectors (loading all CSS)

          I can try to get rid of other css as well. Perhaps this could be a temporary fix if i can get below the 4096 selectors.

          Show
          Bas Brands added a comment - I tried leaving out module css and only loading it when required to try and stay below 4096 selectors see: https://github.com/bmbrands/theme_bootstrap/commit/8e259d2ac0d89cc7560bed09b2da93e16348c467 With fix I still have: 5377 selectors (loading module CSS only when required) Without fix I have: 6222 selectors (loading all CSS) I can try to get rid of other css as well. Perhaps this could be a temporary fix if i can get below the 4096 selectors.
          Hide
          David Scotson added a comment -

          My numbers were for everything else that isn't in the theme sheets, so I think there's questions and plugins and blocks contributing to that total of 1796, not just mods.

          As I said in the github link , if we do have to work around this bug, I'd rather leave the stuff that works for IE10 and all the other browsers alone, and only change things for IE6789. So we could create an extra CSS file that's only linked to from IE, either by checking the user agent in PHP or putting the stylesheet link in conditional comments.

          This could also be a prototype of what the actual core fix could do, maybe?

          I was checking out the bless project, and they don't seem to do anything more sophisticated than count the number of commas and opening brackets to figure out how many selectors the file has.

          Show
          David Scotson added a comment - My numbers were for everything else that isn't in the theme sheets, so I think there's questions and plugins and blocks contributing to that total of 1796, not just mods. As I said in the github link , if we do have to work around this bug, I'd rather leave the stuff that works for IE10 and all the other browsers alone, and only change things for IE6789. So we could create an extra CSS file that's only linked to from IE, either by checking the user agent in PHP or putting the stylesheet link in conditional comments. This could also be a prototype of what the actual core fix could do, maybe? I was checking out the bless project, and they don't seem to do anything more sophisticated than count the number of commas and opening brackets to figure out how many selectors the file has.
          Hide
          David Scotson added a comment -

          So I went looking in theme/styles.php and there's already a call to a function in there called css_send_ie_css (located in lib/csslib.php) which has comments that suggest it's trying to work around this issue.

          https://github.com/moodle/moodle/blob/master/lib/csslib.php#L96-L139

          It sends the parent theme, the plugins and the current theme as separate files to work around the limits.

          So the reason why we're hitting this with Bootstrap theme and no-one else has is that we've gone over the 4096 limit within the single theme (a combination of not inherting from Base, import the full Bootstrap, and then using workarounds to generate Moodle compatible CSS from Bootstrap).

          Show
          David Scotson added a comment - So I went looking in theme/styles.php and there's already a call to a function in there called css_send_ie_css (located in lib/csslib.php) which has comments that suggest it's trying to work around this issue. https://github.com/moodle/moodle/blob/master/lib/csslib.php#L96-L139 It sends the parent theme, the plugins and the current theme as separate files to work around the limits. So the reason why we're hitting this with Bootstrap theme and no-one else has is that we've gone over the 4096 limit within the single theme (a combination of not inherting from Base, import the full Bootstrap, and then using workarounds to generate Moodle compatible CSS from Bootstrap).
          Hide
          David Scotson added a comment -

          Is the above linked code still used? Looking at the stylesheets link produced in 2.5 master and moodle.org it seems to be writing out the separate links directly, rather than linking to one file which @imports them (which I was just about to suggest was perhaps a bad idea). Anyone know where the code to do that is?

          Show
          David Scotson added a comment - Is the above linked code still used? Looking at the stylesheets link produced in 2.5 master and moodle.org it seems to be writing out the separate links directly, rather than linking to one file which @imports them (which I was just about to suggest was perhaps a bad idea). Anyone know where the code to do that is?
          Hide
          Matteo Scaramuccia added a comment -
          Show
          Matteo Scaramuccia added a comment - It should be used here: https://github.com/moodle/moodle/blob/master/theme/styles.php#L77 . HTH, Matteo
          Hide
          David Scotson added a comment -

          I think it's been semi-replaced with these lines:

          https://github.com/moodle/moodle/blob/master/lib/outputlib.php#L628-L633

          though it still gets called from later in that same function:

          https://github.com/moodle/moodle/blob/master/lib/outputlib.php#L704-L713

          Most of these places should probably be updated now that IE10 has fixed this, even if people don't mind the slightly suboptimal performance of multiple stylesheets for older IEs.

          Show
          David Scotson added a comment - I think it's been semi-replaced with these lines: https://github.com/moodle/moodle/blob/master/lib/outputlib.php#L628-L633 though it still gets called from later in that same function: https://github.com/moodle/moodle/blob/master/lib/outputlib.php#L704-L713 Most of these places should probably be updated now that IE10 has fixed this, even if people don't mind the slightly suboptimal performance of multiple stylesheets for older IEs.
          Hide
          Mary Evans added a comment - - edited

          @David:

          With regards the Bootstrap theme, can you not, for the sake of getting a foothold into Moodle, create a simple theme, that over time can evolve into the Moodle Bootstrap theme you dream of?

          It's too late in the day to go delving into more and more Moodle code, and finding dead ends.

          If you don't watch out Martin may just put a hold on the whole project, as Martin's ideas for where Moodle is heading are not yours, and visa versa.

          Show
          Mary Evans added a comment - - edited @David: With regards the Bootstrap theme, can you not, for the sake of getting a foothold into Moodle, create a simple theme, that over time can evolve into the Moodle Bootstrap theme you dream of? It's too late in the day to go delving into more and more Moodle code, and finding dead ends. If you don't watch out Martin may just put a hold on the whole project, as Martin's ideas for where Moodle is heading are not yours, and visa versa.
          Hide
          Stuart Lamour added a comment -

          would it be fun to load the core bootstrap css from http://www.bootstrapcdn.com/ in older browsers?
          i know its an extra request, but seems like a getoutofjailfree card for the mo?

          Show
          Stuart Lamour added a comment - would it be fun to load the core bootstrap css from http://www.bootstrapcdn.com/ in older browsers? i know its an extra request, but seems like a getoutofjailfree card for the mo?
          Hide
          Sam Hemelryk added a comment -

          Hi everyone.

          The 4095 selector limit is a well know issue. It has indeed been factored into the design of our CSS delivery, the links David provided a couple of posts above illustrate how that is being achieved.
          We split styles out into three sheets:

          • Theme - the style sheets for the current theme.
          • Parents - the style sheets for the parent themes.
          • Plugins - the styles.css files that can exist within every plugin in Moodle.

          Each provides a gotcha for this issue still.
          If you manage to create a theme that has > 4095 selectors, if you inherit several themes as parents that when combined introduce > 4095 selectors, or if you have so many plugins with styles.css files that they combined introduce > 4095 selectors.
          So the risk of crossing this invisible line is there still, to date we have only minimised the chance of hitting it.

          IE10 has increased the imposed limits to much more reasonable figures. Altering our delivery code to split for IE 6-9 rather than all IE would be a worthwhile endeavour.
          Unfortunately I can't test the mechanisms presently. I'm in the process of moving house and the old computer I still have Windows on has been packed up.

          Reading through the comments here truthfully I am not certain I understand what is being asked for.
          Let me just run through how I have read things so far.
          This limitation on the number of selectors is a real issue. If you are experiencing that while using IE 6-9 then either how we are splitting things is no longer enough, or the mechanism is broken.
          If its no longer enough then we need to track down the area causing the issue. If I read right it is the combined styles for the theme that is blowing out the limit.
          Presently we provide no way for theme's to "split" their CSS when its being delivered. They get locked into the same caching and performance optimisation routines as all other CSS.
          What I think is required is some way to "split" theme CSS particularly for IE. I think I have an idea regarding that and how it may be achieved.

          If that is it then I'm sure we can come up with a solution. The final thing there is to know whether this is proving to be a blocker for the development of a core bootstrap theme.

          Now, OOCSS, great idea, truly worthwhile, huge change. Not going to happen for 2.5 unless someone is well on there way and have been working with others to get it ready for integration. Otherwise should anyone tackle that and need a hand I would be more than happy to help out where I can.
          If this issue is about achieving that then there's not much I can do (presently) sorry. Code freeze for 2.5 is at the end of March, everything new must be ready by then, and I've already got a queue that will push me to my limits.

          Stuart you mentioned using Bootstrap CDN. If bootstrap lands as a core library (lib/bootstrap or whatever) then we can make that an option, just as we have an option to use YUI's CDN. However Moodle out of the box MUST work without external connectivity, if bootstrap is to be used within a theme provided with Moodle then it must be fully contained within the theme.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi everyone. The 4095 selector limit is a well know issue. It has indeed been factored into the design of our CSS delivery, the links David provided a couple of posts above illustrate how that is being achieved. We split styles out into three sheets: Theme - the style sheets for the current theme. Parents - the style sheets for the parent themes. Plugins - the styles.css files that can exist within every plugin in Moodle. Each provides a gotcha for this issue still. If you manage to create a theme that has > 4095 selectors, if you inherit several themes as parents that when combined introduce > 4095 selectors, or if you have so many plugins with styles.css files that they combined introduce > 4095 selectors. So the risk of crossing this invisible line is there still, to date we have only minimised the chance of hitting it. IE10 has increased the imposed limits to much more reasonable figures. Altering our delivery code to split for IE 6-9 rather than all IE would be a worthwhile endeavour. Unfortunately I can't test the mechanisms presently. I'm in the process of moving house and the old computer I still have Windows on has been packed up. Reading through the comments here truthfully I am not certain I understand what is being asked for. Let me just run through how I have read things so far. This limitation on the number of selectors is a real issue. If you are experiencing that while using IE 6-9 then either how we are splitting things is no longer enough, or the mechanism is broken. If its no longer enough then we need to track down the area causing the issue. If I read right it is the combined styles for the theme that is blowing out the limit. Presently we provide no way for theme's to "split" their CSS when its being delivered. They get locked into the same caching and performance optimisation routines as all other CSS. What I think is required is some way to "split" theme CSS particularly for IE. I think I have an idea regarding that and how it may be achieved. If that is it then I'm sure we can come up with a solution. The final thing there is to know whether this is proving to be a blocker for the development of a core bootstrap theme. Now, OOCSS, great idea, truly worthwhile, huge change. Not going to happen for 2.5 unless someone is well on there way and have been working with others to get it ready for integration. Otherwise should anyone tackle that and need a hand I would be more than happy to help out where I can. If this issue is about achieving that then there's not much I can do (presently) sorry. Code freeze for 2.5 is at the end of March, everything new must be ready by then, and I've already got a queue that will push me to my limits. Stuart you mentioned using Bootstrap CDN. If bootstrap lands as a core library (lib/bootstrap or whatever) then we can make that an option, just as we have an option to use YUI's CDN. However Moodle out of the box MUST work without external connectivity, if bootstrap is to be used within a theme provided with Moodle then it must be fully contained within the theme. Cheers Sam
          Hide
          Daniel Wahl added a comment -
          • @amy: While there are reasons to use oocss I don't think this is a certifiable bug to show that we need it. This is a certifiable browser hack that oocss could help solve.
          • While this is a bug for IE6-9 I think it's important to remember we only need to worry about 8-9, and if it happens to work on 6-7, even better
          • how about a function (when a user is using IE) that counts selectors in type->theme, if it's >4095 then instead of sending the theme sheets as 1 sheet, we do it like we do parent sheets, with a foreach and they get sent individually. The danger here is a theme containing more sheets than IE will include (32)
          • Or alternatively (harder) when counting selectors if we hit 4095 the counter breaks with the filename stored in a var - then at the filename we split into theme.css and theme0.css - the danger here is theme0 having >4095 selectors without checking...

          this function would probably be slow as we're running some kind of regex thousands of times but:

          • it's better than styles not being served
          • it only runs on the affected browsers
          • hopefully caching is turned on so it only runs with theme css is updated or expires
          Show
          Daniel Wahl added a comment - @amy: While there are reasons to use oocss I don't think this is a certifiable bug to show that we need it. This is a certifiable browser hack that oocss could help solve. While this is a bug for IE6-9 I think it's important to remember we only need to worry about 8-9, and if it happens to work on 6-7, even better how about a function (when a user is using IE) that counts selectors in type->theme, if it's >4095 then instead of sending the theme sheets as 1 sheet, we do it like we do parent sheets, with a foreach and they get sent individually. The danger here is a theme containing more sheets than IE will include (32) Or alternatively (harder) when counting selectors if we hit 4095 the counter breaks with the filename stored in a var - then at the filename we split into theme.css and theme0.css - the danger here is theme0 having >4095 selectors without checking... this function would probably be slow as we're running some kind of regex thousands of times but: it's better than styles not being served it only runs on the affected browsers hopefully caching is turned on so it only runs with theme css is updated or expires
          Hide
          David Scotson added a comment -

          Hi Sam,

          Yes, your summary is pretty much where I am now after digging into this. The Bootstrap theme currently nudges over the 4095 limit within just the theme sheets (for various reasons), so the current splitting mechanism fails for our edge case.

          I'm fairly sure I can work around this in the Bootstrap theme just by a combination of reducing the number of unnecessary selectors and moving less "vital" styles to the end of the file which will be ignore by IE < 9.

          Fixing IE10 would be good, as it's a performance hit, but the current IE6-9 situation is also not ideal from a performance perspective so I think a top level function that compacts all the styles then (if necessary) splits on 4095 selectors would be good, (something like Daniel mentions above). Though the fact that you have to have both the side that writes out the links to the stylesheet and the side that generates the concatenated stylesheet working in sync makes this tricky.

          I'm hopeful that the Bootstrap theme may actually end up under 4095 selectors total, so it would be a shame if in that case IE < 9 continued to serve three style sheets where one would do, but that's a subset of the above problem.

          Show
          David Scotson added a comment - Hi Sam, Yes, your summary is pretty much where I am now after digging into this. The Bootstrap theme currently nudges over the 4095 limit within just the theme sheets (for various reasons), so the current splitting mechanism fails for our edge case. I'm fairly sure I can work around this in the Bootstrap theme just by a combination of reducing the number of unnecessary selectors and moving less "vital" styles to the end of the file which will be ignore by IE < 9. Fixing IE10 would be good, as it's a performance hit, but the current IE6-9 situation is also not ideal from a performance perspective so I think a top level function that compacts all the styles then (if necessary) splits on 4095 selectors would be good, (something like Daniel mentions above). Though the fact that you have to have both the side that writes out the links to the stylesheet and the side that generates the concatenated stylesheet working in sync makes this tricky. I'm hopeful that the Bootstrap theme may actually end up under 4095 selectors total, so it would be a shame if in that case IE < 9 continued to serve three style sheets where one would do, but that's a subset of the above problem.
          Hide
          Sam Hemelryk added a comment -

          Alrighty, sorry I've been out of the loop due on personal matter the past couple of weeks.

          I've marked this as a blocker and its not at the top of my list as must fix before the release of 2.5.

          Many thanks
          Sam

          Show
          Sam Hemelryk added a comment - Alrighty, sorry I've been out of the loop due on personal matter the past couple of weeks. I've marked this as a blocker and its not at the top of my list as must fix before the release of 2.5. Many thanks Sam
          Hide
          David Scotson added a comment -

          I'm guessing from the rest of the sentence you meant "now at the top of my list"?

          The last time I checked the workarounds I put in place (partly slimming the number of selectors, partly moving debug stuff to the bottom) meant this was only affecting IE9 now and in a non-vital way, but the exact impact changes as selectors get added or removed in the core styles.

          The current plan for Bootstrap generally is to have it as a hidden parent theme and users (and people creating new themes) would use a child theme called Simple (which mostly just acts as a template for new themes) so a new avenue for workarounds is to move/repeat the end of the CSS that is getting cut off in the child theme. I believe (though I've not actually checked) that if you did that the parent and child CSS would be sent to IE as two separate files with the current system.

          But a more comprehensive fix would help load times on IE (and IE10 in particular if it's opted out of this process).

          Show
          David Scotson added a comment - I'm guessing from the rest of the sentence you meant "now at the top of my list"? The last time I checked the workarounds I put in place (partly slimming the number of selectors, partly moving debug stuff to the bottom) meant this was only affecting IE9 now and in a non-vital way, but the exact impact changes as selectors get added or removed in the core styles. The current plan for Bootstrap generally is to have it as a hidden parent theme and users (and people creating new themes) would use a child theme called Simple (which mostly just acts as a template for new themes) so a new avenue for workarounds is to move/repeat the end of the CSS that is getting cut off in the child theme. I believe (though I've not actually checked) that if you did that the parent and child CSS would be sent to IE as two separate files with the current system. But a more comprehensive fix would help load times on IE (and IE10 in particular if it's opted out of this process).
          Hide
          Sam Hemelryk added a comment - - edited

          Haha yes indeed I meant top of my list.

          I've put up a fix now that chunks CSS files when serving for IE.
          The impact of chunking is noticeable but not too bad and at least we can be sure things will work after this change.

          Petr I've added you as a watcher here.
          Could you please take a look at this and just check that I havn't busted up any of the smarts that go on behind the scenes that I may have overlooked.
          Worth noting is that while I have wired the chunking into the etags I haven't separated the chunks by directory. I don't think that is required but will let you be the judge of that.

          I think after the release of 2.5 it may be worth revisiting this area.
          At present I chunk the separated files served to IE - plugins, parents, and theme. I think really we could probably do away with that separation after this change and just serve all or chunked all.
          What do you think?

          Many thanks
          Sam

          Show
          Sam Hemelryk added a comment - - edited Haha yes indeed I meant top of my list. I've put up a fix now that chunks CSS files when serving for IE. The impact of chunking is noticeable but not too bad and at least we can be sure things will work after this change. Petr I've added you as a watcher here. Could you please take a look at this and just check that I havn't busted up any of the smarts that go on behind the scenes that I may have overlooked. Worth noting is that while I have wired the chunking into the etags I haven't separated the chunks by directory. I don't think that is required but will let you be the judge of that. I think after the release of 2.5 it may be worth revisiting this area. At present I chunk the separated files served to IE - plugins, parents, and theme. I think really we could probably do away with that separation after this change and just serve all or chunked all. What do you think? Many thanks Sam
          Hide
          Petr Škoda added a comment - - edited

          Hello, I do not understand why did you keep the css_send_ie_css(), it seems to me that the new chunks alone could replace it, right?

          Show
          Petr Škoda added a comment - - edited Hello, I do not understand why did you keep the css_send_ie_css(), it seems to me that the new chunks alone could replace it, right?
          Hide
          Sam Hemelryk added a comment - - edited

          Hi Petr,

          It could certainly replace css_send_ie_css. Presently I don't think that function is actually utilised is it as theme_config::css_urls is detecting browser version and splitting there.

          The reason I've taken this approach was because it was going to result in less significant changes, something I thought would be worth doing as this needs to land to master before release.
          My intentions were to open a new issue when this got integrated to make that change and clean things.
          However I'm happy to drop the current splitting entirely in favour of chunking.
          What do you think?

          Show
          Sam Hemelryk added a comment - - edited Hi Petr, It could certainly replace css_send_ie_css. Presently I don't think that function is actually utilised is it as theme_config::css_urls is detecting browser version and splitting there. The reason I've taken this approach was because it was going to result in less significant changes, something I thought would be worth doing as this needs to land to master before release. My intentions were to open a new issue when this got integrated to make that change and clean things. However I'm happy to drop the current splitting entirely in favour of chunking. What do you think?
          Hide
          Petr Škoda added a comment -

          ok, it makes sense to rewrite it in future 2.6dev only.

          Show
          Petr Škoda added a comment - ok, it makes sense to rewrite it in future 2.6dev only.
          Hide
          Sam Hemelryk added a comment -

          Thanks Petr - I'll push this for integration now. Lets let the integrators decide

          Show
          Sam Hemelryk added a comment - Thanks Petr - I'll push this for integration now. Lets let the integrators decide
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Hope the "chunker" aka, css_chunk_by_selector_count() does it job properly, really it would be worth having it covered by unit tests.

          Show
          Eloy Lafuente (stronk7) added a comment - Hope the "chunker" aka, css_chunk_by_selector_count() does it job properly, really it would be worth having it covered by unit tests.
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          Thanks for all the clarifications to my questions, Sam.

          I think that yes, the area needs some work but surely for 2.6... that double split+chunk doesn't seem necessary at all, "all-together" & "all-together-chunked" should be the only options, keeping the old split out.

          So,

          1) if you create the new issue for reviewing, taking rid of the split & old code.
          2) you cover the "chunker" with some unit tests to verify it does not bork anything.
          3) Add some testing instructions.

          Then,

          0) I'll be happy to integrate this.

          Sounds ok?

          Show
          Eloy Lafuente (stronk7) added a comment - - edited Thanks for all the clarifications to my questions, Sam. I think that yes, the area needs some work but surely for 2.6... that double split+chunk doesn't seem necessary at all, "all-together" & "all-together-chunked" should be the only options, keeping the old split out. So, 1) if you create the new issue for reviewing, taking rid of the split & old code. 2) you cover the "chunker" with some unit tests to verify it does not bork anything. 3) Add some testing instructions. Then, 0) I'll be happy to integrate this. Sounds ok?
          Hide
          Sam Hemelryk added a comment -

          Great thanks Eloy - will ping you when unit tests are in place, issue created and test instructions added.

          Show
          Sam Hemelryk added a comment - Great thanks Eloy - will ping you when unit tests are in place, issue created and test instructions added.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          super, tia!

          Show
          Eloy Lafuente (stronk7) added a comment - super, tia!
          Hide
          Sam Hemelryk added a comment -

          Updated testing instructions and added comical workaround.

          Show
          Sam Hemelryk added a comment - Updated testing instructions and added comical workaround.
          Hide
          Sam Hemelryk added a comment -

          Linked to MDL-39212 to clean up the CSS splitting/chunking business.

          Show
          Sam Hemelryk added a comment - Linked to MDL-39212 to clean up the CSS splitting/chunking business.
          Hide
          Sam Hemelryk added a comment -

          And have pushed up a branch containing unit tests

          There is one circumstance that I have thought of that the code does not account for that I should mention.
          Should a media query span the 4095 selector count then things will break. The styles at the end of the chunk containing the start of the media query will be fine, however the styles in the following chunk won't be within the media query and as such will be applied in a general sense.
          Personally I don't think we should sweat about this at this point in time. I can't think of a good way around this and it is a very obscure problem with an incredibly low chance of occurring.

          All yours again Eloy.

          Show
          Sam Hemelryk added a comment - And have pushed up a branch containing unit tests There is one circumstance that I have thought of that the code does not account for that I should mention. Should a media query span the 4095 selector count then things will break. The styles at the end of the chunk containing the start of the media query will be fine, however the styles in the following chunk won't be within the media query and as such will be applied in a general sense. Personally I don't think we should sweat about this at this point in time. I can't think of a good way around this and it is a very obscure problem with an incredibly low chance of occurring. All yours again Eloy.
          Hide
          Sam Hemelryk added a comment -

          ping

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

          pong! Sorry I re-started looking to this yesterday. Just doing some tests here breaking it into smaller chunks.. and will land...

          Show
          Eloy Lafuente (stronk7) added a comment - pong! Sorry I re-started looking to this yesterday. Just doing some tests here breaking it into smaller chunks.. and will land...
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          Hi Sam,

          I've been playing with this for some time (reducing the chunk-size to 150 makes things clearer) and I think it's perfect and I'm going to push it now.

          No matter of that... here there are some questions, for confirmation / know more:

          1) When debugging the calls to css_store_css() immediately after purging caches, I get, from safari, 4 calls about to store:

          • cache/theme/standard/css/plugins.css
          • cache/theme/standard/css/parents.css
          • cache/theme/standard/css/theme.css
          • cache/theme/standard/css/all.css

          (with the first 3 being chunked. That's correct, so the chunks are already generated and ready to be served).

          2) But when I debug the same calls to css_store_css() immediately after the point above, from IE 8, I get 12 calls to css_store_css():

          • 3 calls to cache/theme/standard/css/nosvg/plugins.css
          • 3 calls to cache/theme/standard/css/nosvg/parents.css
          • 3 calls to cache/theme/standard/css/nosvg/theme.css
          • 3 calls to cache/theme/standard/css/nosvg/all.css

          I imagine the "nosvg" is correct, and it's about some dark magic changing svgs to pngs or whatever for browsers not supporting them, but the point I don't get is why there are 3 calls for each. Waste of chunking/storing happening apparently.

          3) Is there any reason to keep the css_send_ie_css() working at all? Cannot we simply chunk the all.css and forget? Is that the same point commented by Petr above (the split).

          4) There is one line:

          $chunk = in_array($type, array('plugins', 'parents', 'theme'));
          

          that apparently is there for nothing. Or perhaps you was planning to make the call to css_store_css() (in the loop), using that $chunk value? For your consideration if it can be deleted or you want to use it.

          5) And another detail, perhaps important, again with the "nosvg" versions... it seems that the @imports there are NOT using slasharguments ever, I get, in "nosvg/parents.css":

          @import url(/..../theme/styles.php?theme=standard&rev=1366760667&type=parents&chunk=0);
          

          While the "normal" one seems to be observing the slasharguments thing.

          Uhm... more yet... those imports... are pointing to the "nosvg" chunks? or to the "normal" ones? Shouldn't them be different? Again, for your consideration (followups/whatever).

          And that's all. Pushing now...thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - - edited Hi Sam, I've been playing with this for some time (reducing the chunk-size to 150 makes things clearer) and I think it's perfect and I'm going to push it now. No matter of that... here there are some questions, for confirmation / know more: 1) When debugging the calls to css_store_css() immediately after purging caches, I get, from safari, 4 calls about to store: cache/theme/standard/css/plugins.css cache/theme/standard/css/parents.css cache/theme/standard/css/theme.css cache/theme/standard/css/all.css (with the first 3 being chunked. That's correct, so the chunks are already generated and ready to be served). 2) But when I debug the same calls to css_store_css() immediately after the point above, from IE 8, I get 12 calls to css_store_css(): 3 calls to cache/theme/standard/css/nosvg/plugins.css 3 calls to cache/theme/standard/css/nosvg/parents.css 3 calls to cache/theme/standard/css/nosvg/theme.css 3 calls to cache/theme/standard/css/nosvg/all.css I imagine the "nosvg" is correct, and it's about some dark magic changing svgs to pngs or whatever for browsers not supporting them, but the point I don't get is why there are 3 calls for each. Waste of chunking/storing happening apparently. 3) Is there any reason to keep the css_send_ie_css() working at all? Cannot we simply chunk the all.css and forget? Is that the same point commented by Petr above (the split). 4) There is one line: $chunk = in_array($type, array('plugins', 'parents', 'theme')); that apparently is there for nothing. Or perhaps you was planning to make the call to css_store_css() (in the loop), using that $chunk value? For your consideration if it can be deleted or you want to use it. 5) And another detail, perhaps important, again with the "nosvg" versions... it seems that the @imports there are NOT using slasharguments ever, I get, in "nosvg/parents.css": @ import url(/..../theme/styles.php?theme=standard&rev=1366760667&type=parents&chunk=0); While the "normal" one seems to be observing the slasharguments thing. Uhm... more yet... those imports... are pointing to the "nosvg" chunks? or to the "normal" ones? Shouldn't them be different? Again, for your consideration (followups/whatever). And that's all. Pushing now...thanks!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Integrated, thanks! Plz, take a look to the 1-5 points.

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated, thanks! Plz, take a look to the 1-5 points.
          Hide
          Sam Hemelryk added a comment -

          Hi Eloy,

          Thanks for looking at and integrating that.

          In regards to the points:

          1. Thats the correct behaviour, yay!
          2. This is the downside to the current system and asking the browser to load the three separate sheets (plugins, parent, theme). Styles.php works out the candidate sheet and if it doesn't exist then it generates all of the CSS for the requested them. Thats all + plugins + parent + theme. The issue is likely that the requests after the first request to styles.php arrive before the first request has finished generating the CSS and writing the files. Essentially causing the subsequent requests to trigger the generation as well.
          It does mean that if the first person to hit the site after the caches have been purged is using IE that they will cause the generation to occur 3 times where as someone using any other browser would lead to the generation occurring only once.
          This was occurring before my changes as well and is something that will be addressed when we tidy up the chunking of CSS as Petr suggested. I've created MDL-39212 for that.
          3. Nope no point having it there. That will be cleaned up by MDL-39212 no doubt.
          4. Ewww, that should have been removed sorry.
          5. Yip, best I can figure this is an existing bug. https://github.com/samhemelryk/moodle/blob/3a8c4380c06d632a897714c6251be949dd5cc657/lib/outputlib.php#L653 shows master before this change. If you note within the IE if..else slashargs is ignored completely. IE sheets are never requested using slashargs. Perhaps best to create a new bug for that I think as MDL-39212 will be master only.
          6. (not passing the no svg key on) This is a bug with my code that needs addressing.

          So....
          I'll open a new bug to address points 4 and 6 which require code changes before the release of 2.5.
          I'll create a new issue for point 5 and see whether this is a bug or has been done for some reason.

          Many thanks
          Sam

          Show
          Sam Hemelryk added a comment - Hi Eloy, Thanks for looking at and integrating that. In regards to the points: 1. Thats the correct behaviour, yay! 2. This is the downside to the current system and asking the browser to load the three separate sheets (plugins, parent, theme). Styles.php works out the candidate sheet and if it doesn't exist then it generates all of the CSS for the requested them. Thats all + plugins + parent + theme. The issue is likely that the requests after the first request to styles.php arrive before the first request has finished generating the CSS and writing the files. Essentially causing the subsequent requests to trigger the generation as well. It does mean that if the first person to hit the site after the caches have been purged is using IE that they will cause the generation to occur 3 times where as someone using any other browser would lead to the generation occurring only once. This was occurring before my changes as well and is something that will be addressed when we tidy up the chunking of CSS as Petr suggested. I've created MDL-39212 for that. 3. Nope no point having it there. That will be cleaned up by MDL-39212 no doubt. 4. Ewww, that should have been removed sorry. 5. Yip, best I can figure this is an existing bug. https://github.com/samhemelryk/moodle/blob/3a8c4380c06d632a897714c6251be949dd5cc657/lib/outputlib.php#L653 shows master before this change. If you note within the IE if..else slashargs is ignored completely. IE sheets are never requested using slashargs. Perhaps best to create a new bug for that I think as MDL-39212 will be master only. 6. (not passing the no svg key on) This is a bug with my code that needs addressing. So.... I'll open a new bug to address points 4 and 6 which require code changes before the release of 2.5. I'll create a new issue for point 5 and see whether this is a bug or has been done for some reason. Many thanks Sam
          Hide
          Sam Hemelryk added a comment -

          Linking to MDL-39326 to fix up point 6. It is up for integration already

          Show
          Sam Hemelryk added a comment - Linking to MDL-39326 to fix up point 6. It is up for integration already
          Hide
          Sam Hemelryk added a comment -

          Linking to MDL-39327 to investigate what slasharguments is not used within when serving CSS for IE.

          Show
          Sam Hemelryk added a comment - Linking to MDL-39327 to investigate what slasharguments is not used within when serving CSS for IE.
          Hide
          Rajesh Taneja added a comment -

          Thanks for fixing this Sam,

          All seems to be working fine, except in ie8.
          Following is broken (But it's broken in master as well)

          1. TinyMCE is not being loaded
          2. Field group is collapsed and there is no way to expand it.
          3. Filepicker is not being loaded.

          Will test more before passing it.

          Show
          Rajesh Taneja added a comment - Thanks for fixing this Sam, All seems to be working fine, except in ie8. Following is broken (But it's broken in master as well) TinyMCE is not being loaded Field group is collapsed and there is no way to expand it. Filepicker is not being loaded. Will test more before passing it.
          Hide
          Rajesh Taneja added a comment -

          Thanks Sam,

          All seems to be working as expected. I checked in most of the browsers and with few different themes and all seem fine.

          Show
          Rajesh Taneja added a comment - Thanks Sam, All seems to be working as expected. I checked in most of the browsers and with few different themes and all seem fine.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          I feel myself really alone tonight! So was time to push your fixes upstream!

          "Lest we forget. We will remember them."

          Thanks and ciao!

          Show
          Eloy Lafuente (stronk7) added a comment - I feel myself really alone tonight! So was time to push your fixes upstream! "Lest we forget. We will remember them." Thanks and ciao!
          Hide
          Sam Marshall added a comment -

          Forgot to add comment here, there's a linked issue but just to document it - I filed MDL-39673 to request backport for this issue into 2.4 branch.

          Show
          Sam Marshall added a comment - Forgot to add comment here, there's a linked issue but just to document it - I filed MDL-39673 to request backport for this issue into 2.4 branch.

            People

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

              Dates

              • Created:
                Updated:
                Resolved: