Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Limit data-uri size to 32KB, falling back to relative url() #1190

Closed
wants to merge 7 commits into from
Closed

Limit data-uri size to 32KB, falling back to relative url() #1190

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
d262e64
Change tan function test value.
Feb 19, 2013
0fadc57
Prevent data-uri function from embedding files larger than 32KB.
Feb 19, 2013
2d69b88
Functions use evalEnv, not parseEnv. (move ieCompat env flag)
Feb 19, 2013
b51d0df
Actually pass ieCompat into evalEnv for lessc.
Feb 19, 2013
6624216
Pass silent and verbose flags to evalEnv.
Feb 19, 2013
312e65a
Wrap data-uri warnings in an env.silent check, silencing test set.
Feb 19, 2013
f10f5c2
Fallback url() value for data-uri function should be quoted.
Feb 19, 2013
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions bin/lessc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ var options = {
strictImports: false,
rootpath: '',
relativeUrls: false,
ieCompat: true,
strictMaths: true
};
var continueProcessing = true,
Expand Down Expand Up @@ -72,6 +73,9 @@ args = args.filter(function (arg) {
case 'no-color':
options.color = false;
break;
case 'no-ie-compat':
options.ieCompat = false;
break;
case 'include-path':
if (!match[2]) {
sys.puts("include-path option requires a parameter");
Expand Down
1 change: 1 addition & 0 deletions lib/less/env.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
'strictImports', // option -
'dumpLineNumbers', // option - whether to dump line numbers
'compress', // option - whether to compress
'ieCompat', // option - whether to enforce IE compatibility (IE8 data-uri)
'mime', // browser only - mime type for sheet import
'entryPath', // browser only - path of entry less file
'rootFilename', // browser only - href of the entry less file
Expand Down
18 changes: 18 additions & 0 deletions lib/less/functions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -416,6 +416,24 @@ tree.functions = {

var buf = fs.readFileSync(filePath);

// IE8 cannot handle a data-uri larger than 32KB. If this is exceeded
// and the --ieCompat flag is enabled, return a normal url() instead.
var DATA_URI_MAX_KB = 32,
fileSizeInKB = parseInt((buf.length / 1024), 10);
if (fileSizeInKB >= DATA_URI_MAX_KB) {
// the url() must be relative, not an absolute file path
filePath = path.relative(this.currentDirectory, filePath);

if (this.env.ieCompat !== false) {
// TODO: respect verbose or silent flags here
console.error("Skipped data-uri embedding of %s because its size (%dKB) exceeds IE8-safe %dKB!", filePath, fileSizeInKB, DATA_URI_MAX_KB);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure about console.error - is this an error or a warning or just info?

whats our plan with supporting verbose/silent flags? Guess they just need putting in the env from the options?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Semantically, you're right, this is not strictly an error. A warning is more appropriate here. However, in node, console.warn and console.error both emit to stderr, so the difference is academic.

I tried to think of a different place to put the verbose and silent flags, but nothing makes as much sense as the env object (I get nervous about bloating it too much, but then realize it's just a lightweight object with a series of properties...). I like the approach of a utility logging function, similar to less.writeError in lib/less/index.js. It would allow a central point of config evaluation, formatting, etc. However, that pattern would involve a lot more monkeying around with scope and whatnot, since functions.js does not have access to the less object, nor is there currently a concept of "global" config flags (instead, they're copied and passed around).

For now, adding verbose and silent to the env flags will do. I'll patch it later today.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

long term I would probably add a log function to env - that's why I made it an object is to provide the glue methods that need to be done in multiple places.. but I don't mind if you just add those 2 properties for now.

return new(tree.URL)(new(tree.Anonymous)(filePath));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use tree.Quoted instead of tree.Anonymous then the url( will be quoted

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, I'll push that shortly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the other hand, quoting isn't necessary inside url() statements. I'm not sure why the browser version inserts the quotes, but neither syntax is breaking anything except conformity of output between the browser and CLI versions.

} else {
// if explicitly disabled (via --no-ie-compat on CLI, or env.ieCompat === false), merely warn
console.warn("WARNING: Embedding %s (%dKB) exceeds IE8's data-uri size limit of %dKB!", filePath, fileSizeInKB, DATA_URI_MAX_KB);
}
}

buf = useBase64 ? buf.toString('base64')
: encodeURIComponent(buf);

Expand Down
1 change: 1 addition & 0 deletions lib/less/lessc_helper.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ var lessc_helper = {
sys.puts(" -h, --help Print help (this message) and exit.");
sys.puts(" --include-path Set include paths. Separated by `:'. Use `;' on Windows.");
sys.puts(" --no-color Disable colorized output.");
sys.puts(" --no-ie-compat Disable IE compatibility checks.");
sys.puts(" -l, --lint Syntax check only (lint).");
sys.puts(" -s, --silent Suppress output of error messages.");
sys.puts(" --strict-imports Force evaluation of imports.");
Expand Down
3 changes: 3 additions & 0 deletions test/browser/css/urls.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,3 +44,6 @@
uri-1: url('http://localhost:8081/browser/less/../../data/page.html');
uri-2: url('http://localhost:8081/browser/less/../../data/page.html');
}
#data-uri-toobig {
uri: url('http://localhost:8081/browser/less/../../data/data-uri-fail.png');
}
4 changes: 4 additions & 0 deletions test/browser/less/urls.less
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,3 +43,7 @@
uri-1: data-uri('text/html', '../../data/page.html');
uri-2: data-uri('../../data/page.html');
}

#data-uri-toobig {
uri: data-uri('../../data/data-uri-fail.png');
}
2 changes: 1 addition & 1 deletion test/css/functions.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@
pi: 3.141592653589793;
mod: 2m;
abs: 4%;
tan: 0.8390996311772799;
tan: 0.9004040442978399;
sin: 0.17364817766693033;
cos: 0.8438539587324921;
atan: 0.1rad;
Expand Down
3 changes: 3 additions & 0 deletions test/css/urls.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,3 +52,6 @@
uri-1: url('data:text/html,%3Ch1%3EThis%20page%20is%20100%25%20Awesome.%3C%2Fh1%3E%0A');
uri-2: url('data:text/html,%3Ch1%3EThis%20page%20is%20100%25%20Awesome.%3C%2Fh1%3E%0A');
}
#data-uri-toobig {
uri: url(../data/data-uri-fail.png);
}
Binary file added test/data/data-uri-fail.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
2 changes: 1 addition & 1 deletion test/less/functions.less
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@
pi: pi();
mod: mod(13m, 11cm); // could take into account units, doesn't at the moment
abs: abs(-4%);
tan: tan(40deg);
tan: tan(42deg);
sin: sin(10deg);
cos: cos(12);
atan: atan(tan(0.1rad));
Expand Down
5 changes: 5 additions & 0 deletions test/less/urls.less
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,3 +46,8 @@
uri-1: data-uri('text/html', '../data/page.html');
uri-2: data-uri('../data/page.html');
}

// so, weirdly, the browser output is quoted, the less output is UNquoted
#data-uri-toobig {
uri: data-uri('../data/data-uri-fail.png');
}