Skip to main content

Bug Tracker

Side navigation

#13077 closed bug (wontfix)

Opened December 18, 2012 05:31PM UTC

Closed January 07, 2013 04:27PM UTC

IE6-8 can't insert html comment with .html()

Reported by: cmcnulty Owned by:
Priority: low Milestone: 1.9
Component: manipulation Version: git
Keywords: Cc:
Blocked by: Blocking:

In IE6-8, see:

This is the same oldIE noScope "feature" that affects the other elements in the rnoInnerhtml regex. Solution should be to simply to add !-- to the regex. Pull request coming soon.

Attachments (0)
Change History (8)

Changed December 18, 2012 05:40PM UTC by dmethvin comment:1

Is there a reason to let people do this? We have gone 6 years without it and nobody requested it.

Changed December 18, 2012 06:33PM UTC by cmcnulty comment:2

Only academic reasons I suppose. But then the fix should only add 4 characters, and makes it works consistently across every other browser, and it's a result of a well-understood issue ("noScope" elements) for which we have a fix that works for the other elements that the issue affects (link, style and script).

To be honest though, when I have time, I'd like to take a little more time to take a crack at one last optimization of the way we handle the "innerHTML" shortcut in .html(), which is what this is all about, really.

On a related note, again, when I have time, I'd like to see if I can't prove that the entire innerHTML shortcut, and all of the work-arounds that are required to support it, are still worth it in jQ2.0. In other words, what's the performance ramifications of never doing an innerHTML for IE9.

Changed December 18, 2012 06:42PM UTC by dmethvin comment:3

Haha, cmcnulty, you got a **fevah**, and the only prescription is more **2.0**!

rwaldron and gibson042 have manipulation.js in their sights, at least for the first pass, but I bet there's still room.

There's also the question of whether 2.0 can use createContextualFragment and/or insertAdjacentHTML for some of these cases, see #10426.

Changed January 07, 2013 12:09AM UTC by dmethvin comment:4

@cmcnulty do you want to submit a patch for this? We'll need a unit test which will go into 2.0 as well.

Changed January 07, 2013 12:10AM UTC by dmethvin comment:5

component: unfiledmanipulation
milestone: None1.9
priority: undecidedlow

Changed January 07, 2013 03:35PM UTC by timmywil comment:6

status: newopen

Changed January 07, 2013 04:12PM UTC by cmcnulty comment:7

There are two fixes, the first of which I actually hate, upon further reflection. It is to add "!--" to the rnoInnerhtml regex. I don't like this because it will force *any* html fragment that has a comment *anywhere* in it to use the much slower (in old ie) append method, instead of innerhtml. So in practice I believe there could be a perf degradation for many implementations with that fix.

The second which I kinda like, but which probably isn't wise, is to just go ahead and "fix" the problem in all cases by, basically, applying a mini-wrapMap fix while in the innerHTML code path. The problem with that is that unlike in the clean function where the wrapMap fix is applied to a non-attached node, in the innerHTML path, the fix would be applied to a (potentially) attached node.

The code is basically to prepend the innerHTML value with the shim that fixes the issue: "<div>x</div>" and then immediately remove the firstchild, which should clean up the fix. It actually passes all unit tests. But I don't think it's an acceptable fix because of the potential for redrawing the screen an extra time as it manipulates an attached element.

So, in short, I guess I recommend this as wontfix.

Changed January 07, 2013 04:27PM UTC by dmethvin comment:8

resolution: → wontfix
status: openclosed

I have no problem closing this without a fix, it seems like a pretty rare case to me anyway and I don't think there is a practical impact on real code. Thanks for thinking it through, at least we have this ticket now as a record.