Moodle

Always include YUI CSS via the theme CSS so we don't have to worry about doing $PAGE->requires->yui_lib before $OUTPUT->header

Details

  • Type: Improvement Improvement
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Fixed
  • Affects Version/s: 2.0
  • Fix Version/s: 2.0
  • Component/s: AJAX
  • Labels:
    None
  • Affected Branches:
    MOODLE_20_STABLE
  • Fixed Branches:
    MOODLE_20_STABLE

Description

At the moment we have an annoying problem where some YUI libraries rely on related CSS files. This means that if you do $PAGE->requries->yui_lib on some libraries after $OUTPUT->header you get an exception.

Instead, we should adopt an approach more like we do with plugin sheets. There should be a $THEME->yuisheets in theme_config which defaults to true, and if that is true, then theme/styles.php should include all the CSS for all the YUI components in the output it is building.

The list of CSS files that are needed should come from the big array of dependencies in ajaxlib.php

Attachments

  1. Text File
    MDL-19935.20090817.patch
    17/Aug/09 2:48 PM
    11 kB
    Sam Hemelryk
  2. Text File
    MDL-19935.20090818.patch
    18/Aug/09 10:57 AM
    12 kB
    Sam Hemelryk
  3. Text File
    MDL-19935.20090819.fixed.patch
    19/Aug/09 4:07 PM
    18 kB
    Sam Hemelryk
  4. Text File
    MDL-19935.20090819.patch
    19/Aug/09 4:02 PM
    18 kB
    Sam Hemelryk

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Tim Hunt added a comment -

Adding some watchers so they know what is going on.

Show
Tim Hunt added a comment - Adding some watchers so they know what is going on.
Hide
Permalink
Petr Škoda (skodak) added a comment -

This would also solve problem with inability to override YUI CSS in standard themes because now they are loaded always after theme CSS, am I right?

Show
Petr Škoda (skodak) added a comment - This would also solve problem with inability to override YUI CSS in standard themes because now they are loaded always after theme CSS, am I right?
Hide
Permalink
Tim Hunt added a comment -

That is right. The standard YUI sheets should be the first thing included by theme/styles.php

Show
Tim Hunt added a comment - That is right. The standard YUI sheets should be the first thing included by theme/styles.php
Hide
Permalink
Sam Hemelryk added a comment -

Hi Tim,
I've attached a patch to solve this issue, if you could please look over it to check it is acceptable I would greatly appreciate it. Worth noting is that I have put the array containing YUI dependencies into a class with several static methods to retrieve JS and/or CSS requirements and removed the original function.
Cheers
Sam

Show
Sam Hemelryk added a comment - Hi Tim, I've attached a patch to solve this issue, if you could please look over it to check it is acceptable I would greatly appreciate it. Worth noting is that I have put the array containing YUI dependencies into a class with several static methods to retrieve JS and/or CSS requirements and removed the original function. Cheers Sam
Hide
Permalink
Sam Hemelryk added a comment -

Hmmmm the attached patch doesn't work for external CSS sheets, I'll look into this now and post another patch once that has been resolved

Show
Sam Hemelryk added a comment - Hmmmm the attached patch doesn't work for external CSS sheets, I'll look into this now and post another patch once that has been resolved
Hide
Permalink
Sam Hemelryk added a comment -

Hi guys,
I've just attached MDL-19935.20090818.patch, unfortunately I could not think of a good way to solve external CSS calls after the header has been printed, so this patch only implements it partially, if YUI is being loaded from the local install then the CSS files are included when by styles.php (about 160KB of CSS).
This patch also fixed a bug whereby external YUI doesnt work because the require management code is trying to include lib/yui/version.php which doesn't exists. This is resolved by getting the version from lib/thirdpartylib.php

If people could please let me know if they have any wonderful ideas about how to go about this, or whether it is even worth it still.
I'm in two minds about this patch, a) it doesnt solve the problem completely, and b) thats a fair bit off CSS being included for VERY little use

Show
Sam Hemelryk added a comment - Hi guys, I've just attached MDL-19935.20090818.patch, unfortunately I could not think of a good way to solve external CSS calls after the header has been printed, so this patch only implements it partially, if YUI is being loaded from the local install then the CSS files are included when by styles.php (about 160KB of CSS). This patch also fixed a bug whereby external YUI doesnt work because the require management code is trying to include lib/yui/version.php which doesn't exists. This is resolved by getting the version from lib/thirdpartylib.php If people could please let me know if they have any wonderful ideas about how to go about this, or whether it is even worth it still. I'm in two minds about this patch, a) it doesnt solve the problem completely, and b) thats a fair bit off CSS being included for VERY little use
Hide
Permalink
Petr Škoda (skodak) added a comment -

I ma having some whitespace problems with this patch

Show
Petr Škoda (skodak) added a comment - I ma having some whitespace problems with this patch
Hide
Permalink
Petr Škoda (skodak) added a comment - - edited

Oh! the YUI issue not solved for me at all because YUI is loaded repeatedly in all themes and in incorrect order, grrrrrrr.

The rules should be:
1/ load YUI styles only once
2/ load YUI styles before any other theme styles
2/ load YUI in correct order which means reset* first

Proposals:
1/ The partial loading of YUI CSS seems very problematic to me, the standard theme is going to load them all, no other theme derived from it can not uninclude it. I propose to check all involved themes and first look for true (means include all), then search for arrays to make a sum and last if no true or array found (most probably outcome) include all.

2/ I do not understand why we load parent and child themes separately - why not just one huge file which includes YUI just once at the beginning?

3/ I do not like the CSS specification per library in yui_translation class - we do not need this association any more, why not just ordered array of CSS files? this would solve the ordering issues.

—

I have discussed this with Tim today, this issue is missing very important detail - the custom CSS and also HEAD JS should be included BEFORE the themes.

Show
Petr Škoda (skodak) added a comment - - edited Oh! the YUI issue not solved for me at all because YUI is loaded repeatedly in all themes and in incorrect order, grrrrrrr. The rules should be: 1/ load YUI styles only once 2/ load YUI styles before any other theme styles 2/ load YUI in correct order which means reset* first Proposals: 1/ The partial loading of YUI CSS seems very problematic to me, the standard theme is going to load them all, no other theme derived from it can not uninclude it. I propose to check all involved themes and first look for true (means include all), then search for arrays to make a sum and last if no true or array found (most probably outcome) include all. 2/ I do not understand why we load parent and child themes separately - why not just one huge file which includes YUI just once at the beginning? 3/ I do not like the CSS specification per library in yui_translation class - we do not need this association any more, why not just ordered array of CSS files? this would solve the ordering issues. — I have discussed this with Tim today, this issue is missing very important detail - the custom CSS and also HEAD JS should be included BEFORE the themes.
Hide
Permalink
Petr Škoda (skodak) added a comment -

4/ links to YUI images do not seem to have correct path because they use relative links

Show
Petr Škoda (skodak) added a comment - 4/ links to YUI images do not seem to have correct path because they use relative links
Hide
Permalink
Petr Škoda (skodak) added a comment - - edited

hmm, what happened to the idea of storing themes in dataroot? Looking at the code if we served all CSS though theme/styles.php and one auxiliary themefile.php it would be relatively easy to implement...
another related problem is the theme images issue - a few months ago we (me and sam) thought we found a general solution that could replace smartpix and all the other pix hacks we have now, unfortunately Tim might have not noticed our discussion and conclusions

Show
Petr Škoda (skodak) added a comment - - edited hmm, what happened to the idea of storing themes in dataroot? Looking at the code if we served all CSS though theme/styles.php and one auxiliary themefile.php it would be relatively easy to implement... another related problem is the theme images issue - a few months ago we (me and sam) thought we found a general solution that could replace smartpix and all the other pix hacks we have now, unfortunately Tim might have not noticed our discussion and conclusions
Hide
Permalink
Sam Hemelryk added a comment -

Latest patch MDL-19935.20090819.patch

Hi Petr,
Thank you for taking the time to look at the patch and giving me some very good feedback.
The following changes have been made to the patch in regards to your feedback:
1. YUI CSS is only loaded once, within the first call to styles.php (for standard theme)
2. It is loaded before any other style sheets, it is at the top of styles.php and a new priority system ensure nothing comes before styles.php
3. It is loaded in the correct order every time
4. The patch fixes the issue of relative links in YUI CSS by appending an absolute path to the resource path.

As well as the above it is also worth noting that while looking more into the YUI css I spotted that assets/skin/sam/skin.css is actually the minified version of all individual YUI component css files in one convenient css file. Because of this we now check if all YUI css is being loaded then we only actually include the reset* files + this file.

In regards to your proposals:
1. This should be fixed now that YUI is only loaded once. The THEME object in both calls to styles.php is that of the theme for the page. My understanding of the way that theme config works would lead me to think that the theme has full control over what YUI libs will be loaded as we look at the THEME object to find out what the theme wants to load. Correct me if I'm wrong here, I'd be happy to look into it more.
2. My +1 for the huge file
3. I like the association of the YUI components and their resources in this method. I think that it offers greatly flexibility when designing this in a way that will allow users to customise how YUI is loaded... should there ever be a need for that. However feel free to convince me otherwise.

By the way do you have a link to the discussion Sam and yourself had that you mentioned above? I'd love to have a read if it's still around.
Cheers
Sam

Show
Sam Hemelryk added a comment - Latest patch MDL-19935.20090819.patch Hi Petr, Thank you for taking the time to look at the patch and giving me some very good feedback. The following changes have been made to the patch in regards to your feedback: 1. YUI CSS is only loaded once, within the first call to styles.php (for standard theme) 2. It is loaded before any other style sheets, it is at the top of styles.php and a new priority system ensure nothing comes before styles.php 3. It is loaded in the correct order every time 4. The patch fixes the issue of relative links in YUI CSS by appending an absolute path to the resource path. As well as the above it is also worth noting that while looking more into the YUI css I spotted that assets/skin/sam/skin.css is actually the minified version of all individual YUI component css files in one convenient css file. Because of this we now check if all YUI css is being loaded then we only actually include the reset* files + this file. In regards to your proposals: 1. This should be fixed now that YUI is only loaded once. The THEME object in both calls to styles.php is that of the theme for the page. My understanding of the way that theme config works would lead me to think that the theme has full control over what YUI libs will be loaded as we look at the THEME object to find out what the theme wants to load. Correct me if I'm wrong here, I'd be happy to look into it more. 2. My +1 for the huge file 3. I like the association of the YUI components and their resources in this method. I think that it offers greatly flexibility when designing this in a way that will allow users to customise how YUI is loaded... should there ever be a need for that. However feel free to convince me otherwise. By the way do you have a link to the discussion Sam and yourself had that you mentioned above? I'd love to have a read if it's still around. Cheers Sam
Hide
Permalink
Sam Hemelryk added a comment -

Sorry just spotted that Netbeans wrote some waste into the last patch (MDL-19935.20090819.patch).
Please use this patch instead: MDL-19935.20090819.fixed.patch

Show
Sam Hemelryk added a comment - Sorry just spotted that Netbeans wrote some waste into the last patch (MDL-19935.20090819.patch). Please use this patch instead: MDL-19935.20090819.fixed.patch
Hide
Permalink
Tim Hunt added a comment -

I guess this is on hold until http://docs.moodle.org/en/Development:Theme_changes_proposal is agreed.

Show
Tim Hunt added a comment - I guess this is on hold until http://docs.moodle.org/en/Development:Theme_changes_proposal is agreed.
Hide
Permalink
Sam Hemelryk added a comment -

Resolved in recent theme changes

Show
Sam Hemelryk added a comment - Resolved in recent theme changes

People

Vote (0)
Watch (4)

Dates

  • Created:
    Updated:
    Resolved:
Atlassian JIRA (v4.4.3#663-r165197) | Report a problem
Powered by a free Atlassian JIRA open source license for Moodle. Try JIRA - bug tracking software for your team.