added support for merging imported stylesheets

[ju1ius]

This commit allows to resolve @import rules, merging their declarations into the main stylesheet.
Required a few changes to the API, see the comments.

[ju1ius]

The constructor now accepts an array of user-defined options.
Parsing now occurs in the parseFile, parseURL adn/or parseString methods.

[sabberworm]

Oh, how embarassing. I fixed this but forgot to commit.

[sabberworm]

I’m considering your changes but would like to propose some modifications to keep compatibility with existing API: I want to be able to give all options in the constructor (but also allow a chainable API to set the options) and introduce static methods for createWithURL and createWithFile. You could then use the parser as follows, for example: CSSParser::createWithURL('http://example.com/css/…')->absoluteURLs(true)->resolveImports(false)->parse().

Other than API, there are some more fundamental issues I see with your solution:

• Firstly, I’ll have to sleep on the introduction of CSSIgnoredValue. I don’t think there should be anything like an “ignored value”. Especially not any CSSImport.
• Charset declaration after the first or not at top of the document are invalid and SHOULD throw an Exception, not just be ignored. Also, if an @imported file has a different charset than the first, the imported file’s contents should be converted to match the charset of the first file OR the file should not be inlined and the @import should be left as-is. With your solution, the imported file is just included but its charset ignored.
• Imported files should ALWAYS occupy the spot that the @import statement had. This way, CSSImports may also appear within a CSSMediaQuery and the correct ordering of rules is obeyed.
• If resolve_imports is false, CSSImports should not have to be removed and then re-added to the document.
• Local files should be able to import absolute urls (including schema). In other words: get rid of $sImportMode and replace it with a dynamic check to see whether the included path, relative or absolute, appended to base_path, if necessary (also depends on whether base_path is URL or path) is a URL or a path.
• When parsing a file, it also has to be clear, which directory is root, i.e. what @import url(/some/style-sheet.css) would refer to.

All in all, I’d rather not include the ability to resolve imports than to have it only work correctly if certain constraints are met (both files have the same charset, @import declaration need always to appear on top, included files use the same means of access as the originally parsed file, etc.)…

[ju1ius]

With your solution, the imported file is just included but its charset ignored.

Nope. If you look at the code in the CSSParser::postParse() method, you'll see that for each imported stylesheet, a new instance of CSSParser is created with it's default_charset option set to the charset of the parent stylesheet, and it's ignore_charset_rules option set to true. So imported stylesheets actually inherits the charset from their parent, and their @charset rules are ignored.

Imported files should ALWAYS occupy the spot that the @import statement had. This way, CSSImports may also appear within a CSSMediaQuery and the correct ordering of rules is obeyed.

The CSSParser::postParse() method respects the order of imports and wraps them in a CSSMediaQuery if needed.
Have a look at the test case in tests/CSSImportTest.php.

Local files should be able to import absolute urls

Absolutely. I don't know why I didn't think about it...
The parser also needs to keep track of every imported file in order to prevent circular imports.

As for the API, what about this:

$oParser->getOption('my_option');
$oParser->setOption('my_option', $myValue)->setOption('other_option', $otherValue);
$oParser->getOptions();
$oParser->setOptions(array(
  'my_option' => $myValue,
  'other_option' => $otherValue
));

This way the API doesn't change everytime you need to add a new option.

Firstly, I’ll have to sleep on the introduction of CSSIgnoredValue. I don’t think there should be anything like an “ignored value”.

Charset declaration after the first or not at top of the document are invalid and SHOULD throw an Exception, not just be ignored.

Yep, I understand that. It's a bit out of the topic of this commit though. I should have removed this.
But just to chat about the subject, I would say it depends on what you want the parser to actually do.
If all you want to do is to validate stylesheets, then let the parser raise Exceptions for every error encountered.
But if you want to actually work with real-life stylesheets, the parser MUST handle errors gracefully. Which means being able to ignore invalid css to build the document object model and just go on with parsing, just like web browsers do.
See http://www.w3.org/TR/CSS2/syndata.html#parsing-errors
But of course, adding this CSSIgnoredValue class is just a (too) quick and (too) dirty way to handle this, and it's absolutely not necessary for the import resolving feature to work.

[ju1ius]

Local files should be able to import absolute urls (including schema). In other words: get rid of $sImportMode and replace it with a dynamic check to see whether the included path, relative or absolute, appended to base_path, if necessary (also depends on whether base_path is URL or path) is a URL or a path.

This check already happens in CSSParser::parseUrlValue(), so making the code works is just a matter of adding one check in the postParse method, see the above comment.

[ju1ius]

Also, if an @imported file has a different charset than the first, the imported file’s contents should be converted to match the charset of the first file OR the file should not be inlined and the @import should be left as-is. With your solution, the imported file is just included but its charset ignored.

Having read parts of the webkit source code, it seemed to me that browsers only detect the charset of the main stylesheet and apply it to all imported stylesheets.
Can you point me to the relevant part of the spec ?

[sabberworm]

Can you point me to the relevant part of the spec?

Sure

From http://www.w3.org/TR/CSS2/syndata.html#charset:

When a style sheet resides in a separate file, user agents must observe the following priorities when determining a style sheet's character encoding (from highest priority to lowest):
An HTTP "charset" parameter in a "Content-Type" field (or similar parameters in other protocols)
BOM and/or @charset (see below)
<link charset=""> or other metadata from the linking mechanism (if any)
charset of referring style sheet or document (if any)
Assume UTF-8

Meaning what your code currently does is what a browser would do if all other means fail. Wo do, however, have two means of charset-detection at our disposal: a possible BOM and @charset-Rule of the included stylesheet.

[sabberworm]

It seems, however, that I have been wrong about replacing @imports inline: they really are only allowed after @charset but must always come before all other rules. And definitely not inside an @media query. I did a simple testcase and it seems like all browsers follow this spec, indeed.
I light of this, I would suggest adding CSSImports whenever they occur (to have the rendered output remain unchanged) but to inline them only in places where the browser would actually load the imported stylesheet.

[ju1ius]

I light of this, I would suggest adding CSSImports whenever they occur (to have the rendered output remain unchanged) but to inline them only in places where the browser would actually load the imported stylesheet.

Which is the reason why I added CSSIgnoredValue in the first place...

[sabberworm]

Yes, I know. But 1. CSSIgnoredValue’s __toString() just returns an empty string and 2. Looking at the CSSDocument hierarchy, you’ll never know what the value was that was ignored. Maybe you want this.

On 11.09.2011, at 10:11, ju1ius reply@reply.github.com wrote:

I light of this, I would suggest adding CSSImports whenever they occur (to have the rendered output remain unchanged) but to inline them only in places where the browser would actually load the imported stylesheet.

Which is the reason why I added CSSIgnoredValue in the first place...

Reply to this email directly or view it on GitHub:
#22 (comment)