Opened 13 years ago

Closed 12 years ago

Last modified 12 years ago

#1958 closed defect (wontfix)

dojo.html.insertCssText causes "Invalid procedure call or argument" in IE when styleSheet.disabled is true

Reported by: aaronmevans@… Owned by: alex
Priority: high Milestone: 0.9
Component: General Version: 0.4.1rc1
Keywords: stylesheet, disabled, IE, error Cc:
Blocked By: Blocking:

Description

In dojo.html.style.js, in the function dojo.html.insertCssText, there is some special handling for IE for setting the css text on the style dom node:

if(style.styleSheet){ IE

style.styleSheet.cssText = cssStr;

}

However, if style.styleSheet.disabled is true, then you get the error "Invalid procedure call or argument" (both IE 6 and IE 7).

Why style.styleSheet.disabled is sometimes true I have no idea, it just is in one of my particular test cases. Unfortunately, I cannot reduce the test case to something small enough to post.

However, I have attached a workaround that solves the issue. I just do this instead:

if(style.styleSheet){ IE

if(!style.styleSheet.disabled){

style.styleSheet.cssText = cssStr;

}

}

Now I realize that without the reproducible test case, you might be a little hesitant to apply this patch (which I will attach shortly).

However, it stands to reason that even if you can't produce a test case where the style sheet ends up with disabled=true, if we just suppose that if it were somehow disabled, that attempting to modify it would throw some kind of error (and it does, trust me).

So why not just add the extra handling anyway as a precaution? It's not that many extra characters. :)

-aaron

Attachments (5)

1958.patch (529 bytes) - added by aaronmevans@… 13 years ago.
patch for dojo.html.style.js
style-fix.patch (1.9 KB) - added by Adam Peller 13 years ago.
patch from Josh Steiger (IBM)
styleTest.html (1.2 KB) - added by Adam Peller 13 years ago.
from Josh Steiger (IBM)
ie 30 style tags then die.html (1.5 KB) - added by morris 13 years ago.
Test file showing use of import stylesheets
dojo0.3.1_ContentPaneCssUnloadFix.diff (1.3 KB) - added by robert.coup@… 13 years ago.
Fix for ContentPane? styles not being unloaded in 0.3.1

Download all attachments as: .zip

Change History (23)

Changed 13 years ago by aaronmevans@…

Attachment: 1958.patch added

patch for dojo.html.style.js

comment:1 Changed 13 years ago by aaronmevans@…

Oh, BTW, in my case, I only saw this when at least all of the following were true (like I said, I can't quite pin it down):

  1. This was with a custom widget.
  2. Only happened when using a custom build (ie doesn't happen in dev mode) with intern strings.
  3. Only happened when I had a bunch of these widgets in the page.
  4. Never happened for widgets created from inline HTML. Happened only sometimes when another instance of the widget was created programmatically because of some user action in the UI. But it doesn't always happen under these circumstances.

-aaron

comment:2 Changed 13 years ago by alex

Owner: changed from anonymous to alex
Status: newassigned

comment:3 Changed 13 years ago by alex

Aaron: I'm a bit worried about this patch since it seems to imply that we'll just drop the style rules on the floor. That doesn't seem like a good idea. Given that we're inserting a brand new stylesheet for just this rule, I think I'm going to modify the patch to try again with a delay.

Regards

comment:4 Changed 13 years ago by alex

Resolution: fixed
Status: assignedclosed

(In [6645]) fail more gracefully in the face of IE foibles. Fixes #1958

comment:5 Changed 13 years ago by aaronmevans@…

Sounds good. Out of curiosity, do you have any idea how the stylesheet object's disabled property would get set to true?

comment:6 Changed 13 years ago by bgreenlee@…

Resolution: fixed
Status: closedreopened

It is still broken for me, even with the fix from the latest nightly build. It seems to break once you have too many widgets on a page. The page I'm having problems with has 33 tooltips and 23 dropdown menus. The tooltips work fine, but none of the dropdown menus do.

comment:7 Changed 13 years ago by aaronmevans@…

Josh Staiger (joshstaiger@…) and I were just discussing the issue on a thread on the mailing list.

See the thread "strange IE error".

Josh seems to have figured it out that IE can only handle up to 31 stylesheets associated with the document.

Further, that dojo must be adding duplicates causing this limit to be hit faster than necessary when the templateCssString property is used which is why you might not see the problem until you create your custom build with intern strings.

I think he has some kind of a patch and was going to check out this bug report and/or perhaps file a new one...

-aaron

comment:8 Changed 13 years ago by bgreenlee@…

Looks like it works if I use an uncompressed dojo build, but not compressed.

Changed 13 years ago by Adam Peller

Attachment: style-fix.patch added

patch from Josh Steiger (IBM)

comment:9 Changed 13 years ago by Adam Peller

Patch from Josh Steiger (IBM). Basically, keep all CSS in a single <STYLE> element to get around the problem. Just keep appending text to the node. Does this cause significant performance problems? We could optimize by keeping a cache or looking for duplicate strings.

Changed 13 years ago by Adam Peller

Attachment: styleTest.html added

from Josh Steiger (IBM)

comment:10 Changed 13 years ago by aaronmevans@…

The reason why it works if you use the uncompressed dojo build is because even if you specific intern-strings the uncompressed build doesn't have strings interned.

So the culprit here is definitely intern-strings (I've tested with a compressed build without intern-strings and it works fine).

I have also discovered that when you exclude latency, intern-strings *kills* performance when you have many homogeneous widgets in a page (both FF and IE).

comment:11 Changed 13 years ago by alex

Milestone: 0.4.10.5

the patch is suspect and most of the problem is fixed in James' recent patch. Pushing to 0.5 until we get a patch that modifies individual CSS style rules instead of doing the += thing to redefine them all.

comment:12 Changed 13 years ago by bill

FYI: James' fix is #2080 / [6789]

comment:13 Changed 13 years ago by morris

I have found a partial solution to this problem, although it would need quite a bit of work to make it tidy.

Although IE has a limit of 31 stylesheets, each stylesheet can have another 31 import stylesheets. And each import stylesheet can have more import stylesheets.

E.g. you can get around the 32 limit by creating dummy stylesheets at the 'head' level, and only adding stylesheets as import stylesheets:

var ss = style.styleSheet;
ss.addImport();
ss.imports[ss.imports.length-1].cssText = cssStr;

The main downside is that you cannot return a reference to the style element (one style element may own many different imported stylesheets).

I will attached some crappy example files to show the fix in action.

Morris

Changed 13 years ago by morris

Test file showing use of import stylesheets

comment:14 Changed 13 years ago by morris

And a poorly written example of a function that allows you to add up to 60 stylesheets in IE:

function insertCssTextHack(cssStr){
	if(!cssStr){ return; }
	if(!dojo.render.html.ie){
		return dojo.html.insertCssText(cssStr);
	}

	var doc = document;

	var style = window.insertCssTextHackStyle;
	if (!style || style.styleSheet.imports.length == 30) {
		window.insertCssTextHackStyle = style = doc.createElement("style");
		style.setAttribute("type", "text/css");
		// IE is b0rken enough to require that we add the element to the doc
		// before changing it's properties
		var head = doc.getElementsByTagName("head")[0];
		head.appendChild(style);
	}

	var ss = style.styleSheet;
	ss.addImport();
	ss.imports[ss.imports.length-1].cssText = cssStr;
	return undefined;
}

comment:15 Changed 13 years ago by terry.field@…

We ran into this problem recently. The most recent fix of creating a timeout if the stylesheet is disable didn't help. I found that Josh Steiger's patch worked well but I was concerned about the performance implications (per Alex's comments).

The patch I have in place right now is based on Josh Steiger's, but tests document.styleSheets.length first before deciding whether to create a new style sheet or append to an existing one. In the latter case, the most recent stylesheet is used (document.styleSheets[30]). Care is taken to limit this special processing to IE only. It's a compromise but a reasonable one.

To alleviate the potential for this problem, we have done an audit on our use of templateCssString in our home-grown widgets. There have been some benefits in doing this.

Just food for thought, but maybe there's an opportunity here to enhance the widget core code and provide another alternative to making css styles available to a widgets.

comment:16 Changed 13 years ago by robert.coup@…

Just for those seeking solutions for v0.3.1:

There was a bug in ContentPane? that was fixed as part of a re-org in [4494] that meant <style> sections in ContentPane? were never removed when new content was loaded. Loading a few pages with <style> into ContentPane? would cause the error described in this ticket.

The patch (dojo0.3.1_ContentPaneCssUnloadFix.diff) fixes this against v0.3.1.

Changed 13 years ago by robert.coup@…

Fix for ContentPane? styles not being unloaded in 0.3.1

comment:17 Changed 12 years ago by Adam Peller

Resolution: wontfix
Status: reopenedclosed

Despite all the clever workarounds to this problem, marking won't fix, as we don't dynamically include CSS in Dijit anymore, and this dojo.html routine also goes away.

comment:18 Changed 12 years ago by guest

Morris here:

FYI Josh Steiger's solution works, my import stylesheets solution may cause IE6 memory leaks (detected using sIEve). I got a memory leak for *every* element in the page that had a filter expression (either from CSS rules or directly in element style of HTML).

The ingredients to cause the leak were:

  • to use addImport (on blank stylesheet served in page, or on dynamically created stylesheet)
  • to use imports[].cssText (even if cssText set to nothing)
  • to use IE filter expressions (e.g. filter: progid:DXImageTransform.Microsoft.AlphaImageLoader or filter: progid:DXImageTransform.Microsoft.Alpha)
  • some other magic ingredient (leak didn't occur on static page, only on dynamic page)

I spent hours trying to work out what the magic ingredient was with no luck. If I removed the line setting cssText, or if I removed all CSS filter: expressions, then the leak went away.

Note: See TracTickets for help on using tickets.