Opened 8 years ago

Closed 8 years ago

#14108 closed defect (fixed)

dojo.css is skipped when interning claro theme's document.css

Reported by: xMartin Owned by: Rawld Gill
Priority: high Milestone: 1.7
Component: BuildSystem Version: 1.7.0b1
Keywords: Cc: ben hockey
Blocked By: Blocking:

Description

dijit/themes/claro/document.css imports "../../../dojo/resources/dojo.css". I'm doing a build with optimizeCss having a custom package with a CSS file that imports claro's document.css. The build report tells me that dojo.css has been skipped and in the built resource there's still the line

@import url(../../dojo/resources/dojo.css);

Attachments (2)

optimizeCss.diff (1.3 KB) - added by Mike Wilcox 8 years ago.
Fix for CSS @import
optimizeCss.svn.patch (1.2 KB) - added by Mike Wilcox 8 years ago.
This patch is better named and fixes the indentation so it's a little easier to see the changes.

Download all attachments as: .zip

Change History (17)

comment:1 Changed 8 years ago by xMartin

Hmm, interesting: The original line in dijit/themes/claro/document.css was

@import url("../../../dojo/resources/dojo.css");

so some processing has been done but it's not inlined.

It's also strange that it is left in the middle of the file since import statements are only allowed at the beginning of a CSS file.

comment:2 Changed 8 years ago by xMartin

To reproduce just make a profile like

var profile = {
	packages: [
		{
			name: "dojo",
			location: "js/dojo"
		},
		{
			name: "dijit",
			location: "js/dijit"
		}
	]
}

and start the script with something like:

./build.sh --profile ../../../profile.js --release --cssOptimize comments.keepLines --releaseDir js/release

comment:3 Changed 8 years ago by ben hockey

Cc: ben hockey added

comment:4 Changed 8 years ago by ben hockey

Priority: normalhigh

Changed 8 years ago by Mike Wilcox

Attachment: optimizeCss.diff added

Fix for CSS @import

comment:5 Changed 8 years ago by Mike Wilcox

I've attached a file that partially fixes this problem. The optimizeCss transform checks for file imports and even has a mechanism to recurse through them, but no where could I find where the @imports are actually converted into build resource objects. This patch does that.

Still a problem though is that path for background-images appear to not get resolved.

Changed 8 years ago by Mike Wilcox

Attachment: optimizeCss.svn.patch added

This patch is better named and fixes the indentation so it's a little easier to see the changes.

comment:6 Changed 8 years ago by ben hockey

by updating the profile to remove the copyOnly tag from any css files in dojo/resources, the specific problem listed in this ticket can be fixed without any additional patches.

  • ../../dojo/dojo.profile.js

     
    1313                       "dojo/tests/_base/loader/requirejs/relative/relative-tests":1,
    1414                       "dojo/tests/_base/loader/requirejs/exports/exports-tests":1
    1515               };
    16                return (mid in list) ||  /^dojo\/_base\/config\w+$/.test(mid) || /^dojo\/resources\//.test(mid);
     16               return (mid in list) ||  /^dojo\/_base\/config\w+$/.test(mid) || /^dojo\/resources\/[^.]+\.(?!css)/.test(mid);
    1717       };
    1818
    1919var profile = {
Last edited 8 years ago by ben hockey (previous) (diff)

comment:7 Changed 8 years ago by Mike Wilcox

I made that change (and removed mine) and still have my inline @imports. It certainly sounds plausible that it is copying instead of building - maybe I missed a step. (my custom profile is not copyOnly'ing css)

I stuck a log in all the copyOnly functions, and I'm not seeing any CSS files coming through there.

comment:8 Changed 8 years ago by ben hockey

here's what i was testing with

./build.sh -p baseplus --release --cssOptimize comments.keepLines

by making that one change in dojo.profile.js and using the above command i was able to see the missing @import inlining in dijit/themes/claro/document.css fixed. you need to make sure that your css resources are NOT marked as copyOnly AND that there is a cssOptimize setting in your profile. if either one of these are not right then you won't see optimized css in your output.

comment:9 Changed 8 years ago by Mike Wilcox

I am able to recreate your findings. However, I still have concerns.

You are using a 1.6 style profile; the build docs don't say to use that, other than that they supported for back-compat. I did try using that style, and while the dojo css built, my custom app css did not build, and I couldn't even get it to copy over (I could in 1.6, even though there were some issues). Also when using 1.6, the builder complains: warn(205) Module not tagged as pure AMD yet it contains AMD API applications. module: app/app (repeated for all my modules).

comment:10 Changed 8 years ago by Mike Wilcox

I've committed my Dojo Build Test code to GitHub?: https://github.com/clubajax/dojo-scaffold

comment:11 Changed 8 years ago by Rawld Gill

In [26898]:

ensure css files are not marked as copyOnly during build, thereby allowing them to be subject to css transforms; refs #14108

comment:12 in reply to:  9 Changed 8 years ago by Rawld Gill

Replying to mwilcox:

I am able to recreate your findings. However, I still have concerns.

You are using a 1.6 style profile; the build docs don't say to use that, other than that they supported for back-compat. I did try using that style, and while the dojo css built, my custom app css did not build, and I couldn't even get it to copy over (I could in 1.6, even though there were some issues). Also when using 1.6, the builder complains: warn(205) Module not tagged as pure AMD yet it contains AMD API applications. module: app/app (repeated for all my modules).

That warning comes from the 1.7 builder (the 1.6 builder doesn't have warning machinery).

If you are not getting the CSS to copy over then one of two things is wrong:

  1. They are being filtered by this:
    (bc.mini && resource.tag.miniExclude) || (!bc.copyTests && resource.tag.test) || (resource.tag.ignore);
    

Which says the mini flag has been specified and the resource is tagged by miniExclude

...or...

the copyTests flag has not been specified and the resource is tagged by test

...or...

the resource is tagged as ignore

  1. The resource is in a file tree that is not specified in the build.

comment:13 Changed 8 years ago by Mike Wilcox

Ah ha. The problem was that the CSS needs to be in the app code - whereas in 1.6 it needed to be in its own namespace (outside of the app code). Or at least it worked that way in 1.6, I'm not positive it needed to be that way.

However, the img directory within the css directory still does not copy over. I've tried using the trees array, and adding it and removing it from the copyOnly list to no avail.

comment:14 Changed 8 years ago by ben hockey

closing this ticket because the original problem is fixed. i took a look at https://github.com/clubajax/dojo-scaffold and i think mike is just having problems configuring the profile to do what he wants.

mike, if you still think there's a bug then open another ticket and attach the profile to that ticket. i just want to close this one since i don't think there's any reason to keep this from blocking 1.7. #14128 still stands as a blocking issue though.

comment:15 Changed 8 years ago by ben hockey

Resolution: fixed
Status: newclosed
Note: See TracTickets for help on using tickets.