Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 2.5
    • Fix Version/s: 2.5
    • Component/s: Themes
    • Labels:
    • Testing Instructions:
      Hide

      Using the bootstrap theme. These tests must be run with developer modes both enabled and disabled.

      • Open Site Administration -> Appearance -> Themes -> Theme settings
      • Copy the default menu structure into the "custommenuitems" field and save
      • Open the dev tools
      • Navigate elsewhere
      • Confirm that the 'Networks' tab doesn't show any attempted calls to the Yahoo CDN or any other error codes for JS
      • Confirm that nothing was printed to the JS console
      • Reduce the width of the page until the banner changes
        • Confirm that clicking on the expand button expands the navigation correctly
      Show
      Using the bootstrap theme. These tests must be run with developer modes both enabled and disabled. Open Site Administration -> Appearance -> Themes -> Theme settings Copy the default menu structure into the "custommenuitems" field and save Open the dev tools Navigate elsewhere Confirm that the 'Networks' tab doesn't show any attempted calls to the Yahoo CDN or any other error codes for JS Confirm that nothing was printed to the JS console Reduce the width of the page until the banner changes Confirm that clicking on the expand button expands the navigation correctly
    • Affected Branches:
      MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull Master Branch:
    • Rank:
      48999

      Description

      Saw this error in my js console even though my site is set to not use external yui.

      400 (Bad Request) http://yui.yahooapis.com/combo?gallery-2013.02.27-21-03/build/gallery-bootstrap/gallery-bootstrap.js"

        Activity

        Hide
        Martin Dougiamas added a comment -

        Andrew, any ideas?

        Show
        Martin Dougiamas added a comment - Andrew, any ideas?
        Hide
        Andrew Nicols added a comment -

        Hmm - that is odd. The loader is trying to load from the YUI gallery, but it shouldn't be because the code should already be present on the page.

        Ah - YUI().use is called before YUI.add in javascript/modlebootstrap.js. I did raise this in my peer review, but it looks like none of my suggestions were taken on board.

        Show
        Andrew Nicols added a comment - Hmm - that is odd. The loader is trying to load from the YUI gallery, but it shouldn't be because the code should already be present on the page. Ah - YUI().use is called before YUI.add in javascript/modlebootstrap.js. I did raise this in my peer review, but it looks like none of my suggestions were taken on board.
        Hide
        Andrew Nicols added a comment -

        Have a fix. In addition it seems that we were including bootstrapengine.js and not using it, and dropdowncollapse.js with content which should have been in moodleboostrap.js.

        I'll have a look at the fix some more in a few hours (off out). Could someone who knows the theme provide some testing instructions though?

        Show
        Andrew Nicols added a comment - Have a fix. In addition it seems that we were including bootstrapengine.js and not using it, and dropdowncollapse.js with content which should have been in moodleboostrap.js. I'll have a look at the fix some more in a few hours (off out). Could someone who knows the theme provide some testing instructions though?
        Hide
        Andrew Nicols added a comment -

        Adding Bas as a watcher here.

        Show
        Andrew Nicols added a comment - Adding Bas as a watcher here.
        Hide
        Andrew Nicols added a comment -

        Okay so:

        • It seems bootstrap-engine is used, but I can't see where it's use is defined;
        • I've mergeddropdowncollapse.js into moodlebootstrap.js;
        • I've fixed the issues causing bootstrap to try to be loaded from the gallery.

        To make all of the above simpler, I've converted moodlebootrap.js into a shifted module (moodle-theme_bootstrap-bootstrap), and included the gallery files as prepended files to the module.

        I'm also trying to work out the best way of including the YUI module - doing so in the header is bad and wrong (though htmlshiv is fine there).

        I'm trying to test all of this but hitting a few issues:
        We include bootstrapcollapse.js and we attempt to plug this in to any element matching

        data-toggle="collapse"

        . However, a quick git grep reveals that we only ever use data-toggle with a value of 'workaround-collapse' so this code isn't used ever.

        Show
        Andrew Nicols added a comment - Okay so: It seems bootstrap-engine is used, but I can't see where it's use is defined; I've mergeddropdowncollapse.js into moodlebootstrap.js; I've fixed the issues causing bootstrap to try to be loaded from the gallery. To make all of the above simpler, I've converted moodlebootrap.js into a shifted module (moodle-theme_bootstrap-bootstrap), and included the gallery files as prepended files to the module. I'm also trying to work out the best way of including the YUI module - doing so in the header is bad and wrong (though htmlshiv is fine there). I'm trying to test all of this but hitting a few issues: We include bootstrapcollapse.js and we attempt to plug this in to any element matching data-toggle= "collapse" . However, a quick git grep reveals that we only ever use data-toggle with a value of 'workaround-collapse' so this code isn't used ever.
        Hide
        Andrew Nicols added a comment -

        Okay,

        So I've fixed nearly everything. The thing I haven't fixed is the data-toggle="collapse" side of things.

        This was set to "workaround-collapse" in the theme layout because the gallery bootstrap-collapse has a bug whereby the height of the navbar is fixed upon opening. This means that when you have a menu with >= 3 elements the button element is not shown.
        This works fine in jQuery because jQuery does not use native browser transitions, it uses solely CSS transitions and it transitions from 0px to 'auto'.
        YUI3 uses native browser transitions where available, and it's not possible to transition from a fixed height (0px) to auto. As a result, the node scrollHeight has been used instead. However, the node scrollHeight is never updated after the transition. So when the menu is expanded it is not able to expand the height of the navbar to accomodate.

        For the moment, I'll drop the 'workaround-' part of the data element in layout/general.php, and break the delegation with a note in the JS. This will enable theme designers building child themes to get their classes right now for when we're able to fix the JS in a future release.

        Show
        Andrew Nicols added a comment - Okay, So I've fixed nearly everything. The thing I haven't fixed is the data-toggle="collapse" side of things. This was set to "workaround-collapse" in the theme layout because the gallery bootstrap-collapse has a bug whereby the height of the navbar is fixed upon opening. This means that when you have a menu with >= 3 elements the button element is not shown. This works fine in jQuery because jQuery does not use native browser transitions, it uses solely CSS transitions and it transitions from 0px to 'auto'. YUI3 uses native browser transitions where available, and it's not possible to transition from a fixed height (0px) to auto. As a result, the node scrollHeight has been used instead. However, the node scrollHeight is never updated after the transition. So when the menu is expanded it is not able to expand the height of the navbar to accomodate. For the moment, I'll drop the 'workaround-' part of the data element in layout/general.php, and break the delegation with a note in the JS. This will enable theme designers building child themes to get their classes right now for when we're able to fix the JS in a future release.
        Hide
        Andrew Nicols added a comment -

        I've added a fix for the bootstrap-collapse in https://github.com/yui/yui3-gallery/pull/26
        I'll wait til Jay has looked at the patch before we pull it into Moodle. Will create a new issue as/when that happens.

        The remaining code on this issue can be integrated in the mean time.

        Show
        Andrew Nicols added a comment - I've added a fix for the bootstrap-collapse in https://github.com/yui/yui3-gallery/pull/26 I'll wait til Jay has looked at the patch before we pull it into Moodle. Will create a new issue as/when that happens. The remaining code on this issue can be integrated in the mean time.
        Hide
        Andrew Nicols added a comment -

        Submitting for integration.

        Show
        Andrew Nicols added a comment - Submitting for integration.
        Hide
        Damyon Wiese added a comment -

        Thanks Andrew,

        This looks much neater now. I also removed the code coverage js from this patch, updated the readme_moodle.txt and removed the whitespace error from the gallery
        module (since I had to update ci anyway).

        Integrated to master.

        Show
        Damyon Wiese added a comment - Thanks Andrew, This looks much neater now. I also removed the code coverage js from this patch, updated the readme_moodle.txt and removed the whitespace error from the gallery module (since I had to update ci anyway). Integrated to master.
        Hide
        Bas Brands added a comment -

        Great stuff Andrew! Very cool you have fixed this

        Show
        Bas Brands added a comment - Great stuff Andrew! Very cool you have fixed this
        Hide
        Jason Fowler added a comment -

        Works fine Andrew, thank you.

        Show
        Jason Fowler added a comment - Works fine Andrew, thank you.
        Hide
        Dan Poltawski added a comment -

        Blooming Marvelous! It's time for a knees up - your changes are upstream!

        Thanks for making Moodle better!

        Toodle pip

        Show
        Dan Poltawski added a comment - Blooming Marvelous! It's time for a knees up - your changes are upstream! Thanks for making Moodle better! Toodle pip

          People

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

            Dates

            • Created:
              Updated:
              Resolved: