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.

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

When creating style tag, include type="text/css" #747

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

When creating style tag, include type="text/css" #747

wants to merge 1 commit into from

Conversation

chuseman
Copy link

When trying to create the <style> tag in IE9, an exception is thrown.

// First time, create an extra stylesheet for adding rules
extraSheet = put(document.getElementsByTagName("head")[0], "style");
// Keep reference to actual StyleSheet object (`styleSheet` for IE < 9)
extraSheet = extraSheet.sheet || extraSheet.styleSheet;

extraSheet.sheet throws the exception.

It looks like you need to supply type="text/css" when creating the tag.

@kfranqueiro
Copy link
Contributor

Can you provide more information about how you encountered this problem? We test dgrid with IE regularly and have never encountered this - and it would have come up awfully quick given that addCssRule is used throughout dgrid.

I'm wondering if your document is perhaps missing a doctype and thus is running in quirks mode?

@chuseman
Copy link
Author

According to the F12 dev tools, the browser mode is IE9 and the document mode is IE9 standards. Is IE9 included in your tests (with X-UA-Compatible set appropriately)?

The exception is "Invalid argument" when trying to access the extraSheet's sheet property. Looking a little more into the style tag, it looks like type may be required? Or at least the content model changes depending on its value according to the HTML5 reference. It seems appropriate to include type="text/css" at any rate.

http://dev.w3.org/html5/html-author/#the-style-element

@kfranqueiro
Copy link
Contributor

Yes, we test frequently with IE8-10 and also test IE6-7 and haven't seen this issue on any of the test pages in dgrid's test folder, and they would all run into it given that addCssRule is used when creating the first dgrid instance on a page. Can you provide a test page that reproduces the issue?

You mention X-UA-Compatible, but really the "appropriate" way to set that is to not set it at all. Including a doctype and omitting X-UA-Compatible should use the standard rendering engine of the version of IE you are using.

The style tag's type attribute actually defaults to "text/css" as per the HTML5 specification, so it really shouldn't need to be added... I don't mean to be difficult, but I can't justify adding code for the sake of a problem we can't reproduce.

@chuseman
Copy link
Author

I haven't been able to create a simple test page to demonstrate the behavior. When I require("dgrid/util/misc") and directly call addCssRule, everything works without adding type="text/css". So there must be something else here that is breaking it for us.

Here's what extraSheet looks like in our project (without my change):
invalidargument
Note that accessing the sheet property throws an exception.

I understand your hesitation to add it to your tree. I've spent an hour trying to give you a simple reproduction and haven't been able to do so... go ahead and close this if you wish.

@chuseman
Copy link
Author

We've stumbled across another IE9 bug so I thought of a new way to try to recreate this and I was successful. Apparently IE9 can only have 31 stylesheets, period. style tags and link rel="stylesheet" count in this number.

During development, we don't compress/reduce the number of stylesheets so we were actually right at this magic number. When the misc module attempts to add a style tag to the page when you are over the limit, IE9 will throw the "Invalid argument" exception. Adding style type="text/css" won't throw an exception but the added stylesheet will still be ignored by the browser.

We will probably be switching over to compress during development for the bulk of the CSS so we'll be under this number. It still may be worth fixing so you at least don't have to deal with a strange exception.

References:
http://blogs.msdn.com/b/ieinternals/archive/2011/05/14/10164546.aspx
http://support.microsoft.com/kb/262161

@chuseman
Copy link
Author

Here's the reproduction. Remove one of the <style> tags to see it work. Apply my patch to get rid of the exception.

<!DOCTYPE html>
<html>
<head>
    <title>addCssRule test</title>
    <style type="text/css">/*  1 */</style>
    <style type="text/css">/*  2 */</style>
    <style type="text/css">/*  3 */</style>
    <style type="text/css">/*  4 */</style>
    <style type="text/css">/*  5 */</style>
    <style type="text/css">/*  6 */</style>
    <style type="text/css">/*  7 */</style>
    <style type="text/css">/*  8 */</style>
    <style type="text/css">/*  9 */</style>
    <style type="text/css">/* 10 */</style>
    <style type="text/css">/* 11 */</style>
    <style type="text/css">/* 12 */</style>
    <style type="text/css">/* 13 */</style>
    <style type="text/css">/* 14 */</style>
    <style type="text/css">/* 15 */</style>
    <style type="text/css">/* 16 */</style>
    <style type="text/css">/* 17 */</style>
    <style type="text/css">/* 18 */</style>
    <style type="text/css">/* 19 */</style>
    <style type="text/css">/* 20 */</style>
    <style type="text/css">/* 21 */</style>
    <style type="text/css">/* 22 */</style>
    <style type="text/css">/* 23 */</style>
    <style type="text/css">/* 24 */</style>
    <style type="text/css">/* 25 */</style>
    <style type="text/css">/* 26 */</style>
    <style type="text/css">/* 27 */</style>
    <style type="text/css">/* 28 */</style>
    <style type="text/css">/* 29 */</style>
    <style type="text/css">/* 30 */</style>
    <style type="text/css">/* 31 */</style>
</head>
<body>
<script src="dojo-release-1.9.1-src/dojo/dojo.js"></script>
<script>
require(["dgrid/util/misc"],
    function(misc) {
        misc.addCssRule("body", "background-color: red");
    });
</script>
</body>
</html>

@kfranqueiro
Copy link
Contributor

Ah, so this is what you were running up against... yes, this is quite a well-known and despised limitation of IE. If dgrid were to do something about this it would basically have to fall back to appending style rules to some random other stylesheet in the page, which I'm not sure how I feel about, but maybe it's a fair compromise for IE... (It'd be just about the only compromise, really.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants