Issues #17 and #34, proposed resolutions

[jbarker]

Nicholas, thanks for cssembed and all the great blog posts related to data URIs and MHTML.

This pull request attempts to resolve issues #17 & issue #34. It is my first pull request on GitHub.

I experienced the behavior described in #17. The unexpected behavior occurred on IE6/WinXP fully patched and IE7/Vista fully patched. The fix was to add one additional newline. Verified on both environments.

I added a new 'mhtmlfile' option to split the output between a CSS file and an MHTML file. This helps to address recent changes in Internet Explorer -- noted in issue #34 -- that require different MIME types for each of those file types.

This request does not include Ant Task support or additional unit tests. Ant Task support is complicated by the current usage of resource collection and mapper. Unit tests are simply left for another day.

[nzakas]

Thanks for the pull request. I only merge in pull requests that come with unit tests for the changes, so if you'd like to follow up by creating the tests, I'll be happy to merge everything in.

[jbarker]

Thanks, Nicholas. The updated pull request has updated tests for the changes related to issues #17 and #34.

[nzakas]

After reviewing this a bit more, I'm not sure I like the idea of outputting two files from one call to the CLI. When I designed this originally, it was because people wanted straight CSS with data URIs, and MHTML was added later for IE8.

[jbarker]

I understand your concerns about CLI usability and the original intent of cssembed. However, today complete MTHML support does require two files.

The need to output two files is based upon changes in how Internet Explorer handles MHTML content after recent security patches [1,2,3]. This need is also mentioned in cssembed issue #34.

If two file output is not to be in cssembed, it may be best to remove MHTML support entirely. Continuing to offer one file output with MHTML enabled will likely cause issues for anyone that attempts to view the output using patched, up-to-date versions of Internet Explorer.

Also, to clarify... MHTML works in Internet Explorer 6.0 and later. It is an especially viable option for IE6 and IE7, both of which do not support data URIs, as noted by Steve Souders in a post promoting CSSEmbed [4].

1. http://www.phpied.com/the-proper-mhtml-syntax/#comment-78586
2. http://support.microsoft.com/kb/2544893
3. http://stackoverflow.com/questions/7481155/mshtml-fallback-for-data-uris
4. http://www.stevesouders.com/blog/2009/11/16/cssembed-automatically-data-uri-ize/

[nzakas]

I think you've misunderstood my statement. I don't want the CLI outputting two files at once. I want to keep the utility outputting only one file at a time. It's simpler, it's more targeted at the prominent use case (that being data URIs, not MHTML), and developers can still run the CLI a second time produce an MHTML file if they so desire.

Your concern has little to do with two files and everything to do with the format of the MHTML output. I can fix that, and then there should be no further security issues.

You seem to be confusing the MHTML syntax error and "two files". It would be silly to remove MHTML support just because I don't want to output two files at the same time.

If you'd like to submit a pull request for just fixing the MHTML syntax, I'll be happy to merge it in (this is frequently the problem when one merge request does two things).

[jbarker]

While it was not my intent to fork cssembed (I appreciate and respect the work that has gone into it), I will keep this new fork for users who want fixed MHTML support with the usual one call:

https://github.com/jbarker/cssembed

This fork provides up-to-date MHTML support, is verified in fully patched IE6 and IE7, has adequate unit test coverage, and satisfies the criteria from issues #17 and #34.

Nicholas, your new requirement -- to refactor the code to make the same functionality require two calls to the utility -- provides no tangible benefit to the user. As a dev scratching my own itch, it does nothing for me. I simply disagree with the need for this requirement and will not be submitting additional comments or pull requests regarding this issue.

[nzakas]

Once again, I think you're misunderstanding. There is no "new" requirement, I simply want the utility to work the way it works now. It was designed this way for a reason, and that reason is that MHTML is a secondary use case for CSSEmbed. You could very easily create a wrapper in a build script that does everything you want, and it wouldn't matter how many times the utility had to be called.

It's too bad you don't want to contribute to the project. The MHTML changes need to make it back into the main branch anyway. Just keep in mind for pull requests for other projects, that fixing one issue at a time is much more likely to result in your pull request being merged in.

[tivac]

FWIW, I'm with Nicholas on this one. It also has the nice side effect of making Ant support simple & keeping the tool's output more consistent.