Moodle
  1. Moodle
  2. MDL-39673

Backport MDL-38441 (stylesheet system problems with large sheets in IE)

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 2.4.4
    • Fix Version/s: 2.4.5
    • Component/s: Themes
    • Labels:
    • Testing Instructions:
      Hide

      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, and clean.
      Switch to IE browse around the site and check things look as expected. Repeat for the same themes as above.

      Open lib/csslib.php in an editor.
      Search for the use of css_chunk_by_selector_count within css_store_css and make its third arg 1500.
      Save the change.
      Using IE log in as an admin and purge your caches.
      Inspect the CSS loaded once the redirecting is finished.
      Check that the style sheets that the import statements within the first three CSS sheets contain either "/_s/" or "svg=0"

      Show
      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, and clean. Switch to IE browse around the site and check things look as expected. Repeat for the same themes as above. Open lib/csslib.php in an editor. Search for the use of css_chunk_by_selector_count within css_store_css and make its third arg 1500. Save the change. Using IE log in as an admin and purge your caches. Inspect the CSS loaded once the redirecting is finished. Check that the style sheets that the import statements within the first three CSS sheets contain either "/_s/" or "svg=0"
    • Affected Branches:
      MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE
    • Pull 2.4 Branch:
      MDL-39673-m24
    • Rank:
      50398

      Description

      MDL-38441 was applied for Moodle 2.5+ only although it also affects 2.4, which is still supported.

      In 2.4, the problem does not affect standard core but if you have a lot of plugins, the plugins CSS file can exceed the 4095 selector limit.

      The change backports cleanly.

        Issue Links

          Activity

          Hide
          Sam Marshall added a comment -

          Note: I am submitting this backport for integration review. There was no change whatever to the commit, which cherry-picked cleanly from the original master commit.

          A developer on my team has tested this change in OU Moodle (where we have enough plugins that the limit is overshot by quite some distance) and it works to fix this problem.

          Show
          Sam Marshall added a comment - Note: I am submitting this backport for integration review. There was no change whatever to the commit, which cherry-picked cleanly from the original master commit. A developer on my team has tested this change in OU Moodle (where we have enough plugins that the limit is overshot by quite some distance) and it works to fix this problem.
          Hide
          Mary Evans added a comment -

          ...take it away Sam...

          Show
          Mary Evans added a comment - ...take it away Sam...
          Hide
          Dan Poltawski added a comment -

          I'm taking this out of integration while we make a decision on this Will pull it back to integration when we have an decision.

          +1 from me, but does this need 23 too?

          Show
          Dan Poltawski added a comment - I'm taking this out of integration while we make a decision on this Will pull it back to integration when we have an decision. +1 from me, but does this need 23 too?
          Hide
          Sam Marshall added a comment -

          Note: I have now checked and this doesn't backport directly to 23 (i.e. cherrypick fails) so is a riskier change on that version. (I can't immediately see how to resolve the conflict, although obviously it is possible.)

          Show
          Sam Marshall added a comment - Note: I have now checked and this doesn't backport directly to 23 (i.e. cherrypick fails) so is a riskier change on that version. (I can't immediately see how to resolve the conflict, although obviously it is possible.)
          Hide
          Eloy Lafuente (stronk7) added a comment -

          +1 for 24_STABLE only

          Show
          Eloy Lafuente (stronk7) added a comment - +1 for 24_STABLE only
          Hide
          Tim Hunt added a comment -

          This is breaking the OU theme. We have an release at the start of June, and to get this in, we could really do with it being integrated this week. Are we took late?

          Anyway, bumping priority.

          Show
          Tim Hunt added a comment - This is breaking the OU theme. We have an release at the start of June, and to get this in, we could really do with it being integrated this week. Are we took late? Anyway, bumping priority.
          Hide
          Sam Marshall added a comment -

          Regarding Tim's last comment - I've now subverted our usual processes to get the fix into our release even though it isn't in core yet, so we are no longer desperate for it to be integrated immediately. Obviously it will be good if it does get in fairly soon on the normal schedule.

          (And if it wasn't clear from my earlier comment, I agree with Eloy that 24_STABLE only is the best approach for this, at least unless somebody has a demand for it in 23!)

          Show
          Sam Marshall added a comment - Regarding Tim's last comment - I've now subverted our usual processes to get the fix into our release even though it isn't in core yet, so we are no longer desperate for it to be integrated immediately. Obviously it will be good if it does get in fairly soon on the normal schedule. (And if it wasn't clear from my earlier comment, I agree with Eloy that 24_STABLE only is the best approach for this, at least unless somebody has a demand for it in 23!)
          Hide
          Damyon Wiese added a comment -

          Looking now - this fix branch does not contain the fixes for regressions from the original issue (MDL-39326). I'll pull those in and keep looking.

          Show
          Damyon Wiese added a comment - Looking now - this fix branch does not contain the fixes for regressions from the original issue ( MDL-39326 ). I'll pull those in and keep looking.
          Hide
          Marina Glancy added a comment -

          This patch looks like it is not going to do anything (=harm) if CSS files are small so it will be applied only when there is actually a problem.

          Just something I noticed:
          variable $chunk is not used after this line: https://github.com/sammarshallou/moodle/compare/MOODLE_24_STABLE...MDL-39673-m24#L1R137
          One of the comments says: "Unit tests are essential for making sure this works." and no unit tests are provided.

          Show
          Marina Glancy added a comment - This patch looks like it is not going to do anything (=harm) if CSS files are small so it will be applied only when there is actually a problem. Just something I noticed: variable $chunk is not used after this line: https://github.com/sammarshallou/moodle/compare/MOODLE_24_STABLE...MDL-39673-m24#L1R137 One of the comments says: "Unit tests are essential for making sure this works." and no unit tests are provided.
          Hide
          Damyon Wiese added a comment -

          Combined the testing instructions from the original issue + regression.

          Show
          Damyon Wiese added a comment - Combined the testing instructions from the original issue + regression.
          Hide
          Damyon Wiese added a comment -

          Thanks Sam(s)!

          This issue has waited it's time - passed an integrator vote and now been backported (only to 24).

          Show
          Damyon Wiese added a comment - Thanks Sam(s)! This issue has waited it's time - passed an integrator vote and now been backported (only to 24).
          Hide
          David Monllaó added a comment -

          It passes, tested in:

          • Win7 + IE9
          • Linux + Chrome
          • Linux + Firefox
          • OS x + Safari

          I can see the first two CSS files (not the YUI ones) with:

          • @import url(/INTEGRATION/MOODLE_24_STABLE/theme/styles.php?theme=standard&rev=1369295472&type=plugins&svg=0&chunk=0);
          • @import url(/INTEGRATION/MOODLE_24_STABLE/theme/styles.php?theme=standard&rev=1369295472&type=parents&svg=0&chunk=0);
          Show
          David Monllaó added a comment - It passes, tested in: Win7 + IE9 Linux + Chrome Linux + Firefox OS x + Safari I can see the first two CSS files (not the YUI ones) with: @import url(/INTEGRATION/MOODLE_24_STABLE/theme/styles.php?theme=standard&rev=1369295472&type=plugins&svg=0&chunk=0); @import url(/INTEGRATION/MOODLE_24_STABLE/theme/styles.php?theme=standard&rev=1369295472&type=parents&svg=0&chunk=0);
          Hide
          Damyon Wiese added a comment -

          Thanks for your contribution! This issue has been reviewed, integrated, tested and now released to everyone.

          Closing as Fixed!

          Show
          Damyon Wiese added a comment - Thanks for your contribution! This issue has been reviewed, integrated, tested and now released to everyone. Closing as Fixed!
          Hide
          Jason Hardin added a comment -

          Was this tested with @media CSS rules? I see in the code a special case if the last element of a chunk ends in a comma, but I am not seeing a check if the last chunk starts with an @ or if the chunk itself is within a @media definition.

          Show
          Jason Hardin added a comment - Was this tested with @media CSS rules? I see in the code a special case if the last element of a chunk ends in a comma, but I am not seeing a check if the last chunk starts with an @ or if the chunk itself is within a @media definition.
          Hide
          Sam Marshall added a comment -

          Jason: I don't think that is different from the original code MDL-38441 code that is on master / 2.5 branch. This appears to be a known issue, identified by Sam Hemelryk in this comment:

          https://tracker.moodle.org/browse/MDL-38441?focusedCommentId=216606&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-216606

          Show
          Sam Marshall added a comment - Jason: I don't think that is different from the original code MDL-38441 code that is on master / 2.5 branch. This appears to be a known issue, identified by Sam Hemelryk in this comment: https://tracker.moodle.org/browse/MDL-38441?focusedCommentId=216606&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-216606
          Hide
          Jason Hardin added a comment -

          Thanks Sam.

          Show
          Jason Hardin added a comment - Thanks Sam.

            People

            • Votes:
              1 Vote for this issue
              Watchers:
              9 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: