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

      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.

        Gliffy Diagrams

          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: