Incompatibility with Highcharts #21

Closed
dmnd opened this Issue Dec 24, 2011 · 34 comments

Comments

Projects
None yet
@dmnd

dmnd commented Dec 24, 2011

When html5shiv is present on a page, Highcharts charts render in grey. See this forum post for more details.

@superohm

This comment has been minimized.

Show comment
Hide comment
@superohm

superohm Dec 26, 2011

Hi,

All my highchart graph was down.... Did you have any idea when you can fixed this trouble ?

Regards,
Guillaume

Hi,

All my highchart graph was down.... Did you have any idea when you can fixed this trouble ?

Regards,
Guillaume

@dmitry-ivanov

This comment has been minimized.

Show comment
Hide comment
@dmitry-ivanov

dmitry-ivanov Dec 26, 2011

Hello!

Would it be fixed soon? And where can I get an old version which is stable with highcharts?

Thanks.

Hello!

Would it be fixed soon? And where can I get an old version which is stable with highcharts?

Thanks.

@patrickberkeley

This comment has been minimized.

Show comment
Hide comment
@patrickberkeley

patrickberkeley Dec 28, 2011

I'm seeing this issue too.

I'm seeing this issue too.

@aFarkas

This comment has been minimized.

Show comment
Hide comment
@aFarkas

aFarkas Dec 28, 2011

Owner

@maindgit and others

I can not give any timeframe, but we are working on this.

Until then, you can use the older version 2.2 or can try the innerShiv-branch, which might become next version

Owner

aFarkas commented Dec 28, 2011

@maindgit and others

I can not give any timeframe, but we are working on this.

Until then, you can use the older version 2.2 or can try the innerShiv-branch, which might become next version

@patrickberkeley

This comment has been minimized.

Show comment
Hide comment
@patrickberkeley

patrickberkeley Dec 28, 2011

Thanks @aFarkas. Highcharts works with html5shim v2.2.

Thanks @aFarkas. Highcharts works with html5shim v2.2.

@aFarkas

This comment has been minimized.

Show comment
Hide comment
@aFarkas

aFarkas Dec 28, 2011

Owner

@patrickberkeley

could you also try the innerShiv-branch? It should also work and like I sayed, I want to become this version the next version.

Owner

aFarkas commented Dec 28, 2011

@patrickberkeley

could you also try the innerShiv-branch? It should also work and like I sayed, I want to become this version the next version.

@dmitry-ivanov

This comment has been minimized.

Show comment
Hide comment
@dmitry-ivanov

dmitry-ivanov Dec 29, 2011

@aFarkas, thanks.
I've checked "older version 2.2" and "innerShiv-branch" both - and the're both work correctly with highcharts.

PS: can you plz comment here when the trunk version (http://html5shiv.googlecode.com/svn/trunk/html5.js) would also supply them?...

@aFarkas, thanks.
I've checked "older version 2.2" and "innerShiv-branch" both - and the're both work correctly with highcharts.

PS: can you plz comment here when the trunk version (http://html5shiv.googlecode.com/svn/trunk/html5.js) would also supply them?...

@patrickberkeley

This comment has been minimized.

Show comment
Hide comment
@patrickberkeley

patrickberkeley Dec 29, 2011

@aFarkas the innerShiv-branch works with highcharts too.

@aFarkas the innerShiv-branch works with highcharts too.

@superohm

This comment has been minimized.

Show comment
Hide comment
@superohm

superohm Jan 3, 2012

@aFarkas thanks a lot

superohm commented Jan 3, 2012

@aFarkas thanks a lot

@romanbsd

This comment has been minimized.

Show comment
Hide comment
@romanbsd

romanbsd Jan 13, 2012

Is innerShiv-branch considered production ready?

Is innerShiv-branch considered production ready?

@aFarkas

This comment has been minimized.

Show comment
Hide comment
@aFarkas

aFarkas Jan 24, 2012

Owner

@romanbsd

Yes this branch can be considered as production ready. The only problem with this is, that this version removes a great feature automatically fixing dynamic HTML5 creation (This is why some tests are failing) and I can not say, wether this will be the next version.


I really would like to have some opinion about this especially from @jonathantneal, @paulirish, @jdalton and @rwldrn.

To sumarize our current issues and how the innerShiv helps:

Performance

I think we can improve current performance and make the current implementation 2-3 times faster, but it will be always 10-20 times slower than unshived implementation (I could live with this, simply because I don't think, that this would generate a botttleneck).

Generated DOM-element does not behave right

For example: a shived DOM element currently has always a document fragment as a parentNode, although it should be null. This happens simply by touching element.document.

var elem = document.createElement('div');
elem.parentNode; //-> null
elem.document;
elem.parentNode; //-> bam: document fragment

I haven't found a way to remove the parentNode or create a shived element without a parent.

The innerShiv branch simply removes our automatic fixing features and provides a simple API to create DOM object from a string using. This way, we can guarantee, that html5shiv won't break anything and still comes with a solution for dynamically generating HTML5 elements.

What do you guys think?

Owner

aFarkas commented Jan 24, 2012

@romanbsd

Yes this branch can be considered as production ready. The only problem with this is, that this version removes a great feature automatically fixing dynamic HTML5 creation (This is why some tests are failing) and I can not say, wether this will be the next version.


I really would like to have some opinion about this especially from @jonathantneal, @paulirish, @jdalton and @rwldrn.

To sumarize our current issues and how the innerShiv helps:

Performance

I think we can improve current performance and make the current implementation 2-3 times faster, but it will be always 10-20 times slower than unshived implementation (I could live with this, simply because I don't think, that this would generate a botttleneck).

Generated DOM-element does not behave right

For example: a shived DOM element currently has always a document fragment as a parentNode, although it should be null. This happens simply by touching element.document.

var elem = document.createElement('div');
elem.parentNode; //-> null
elem.document;
elem.parentNode; //-> bam: document fragment

I haven't found a way to remove the parentNode or create a shived element without a parent.

The innerShiv branch simply removes our automatic fixing features and provides a simple API to create DOM object from a string using. This way, we can guarantee, that html5shiv won't break anything and still comes with a solution for dynamically generating HTML5 elements.

What do you guys think?

@paulirish

This comment has been minimized.

Show comment
Hide comment
@paulirish

paulirish Jan 24, 2012

Collaborator

also ping @remy and @jdbartlett

The html5shiv with both print-protection and createElement duckpunch has been live through the 2 google code project since dec 22nd. (modernizr is still not touching createElement)

I'm actually surprised we havent had more reports of issues... So due to that, I'm kind of thinking keeping the createElement|Fragment duckpunches is okay.

Collaborator

paulirish commented Jan 24, 2012

also ping @remy and @jdbartlett

The html5shiv with both print-protection and createElement duckpunch has been live through the 2 google code project since dec 22nd. (modernizr is still not touching createElement)

I'm actually surprised we havent had more reports of issues... So due to that, I'm kind of thinking keeping the createElement|Fragment duckpunches is okay.

@redoPop

This comment has been minimized.

Show comment
Hide comment
@redoPop

redoPop Jan 24, 2012

Collaborator

I agree with @paulirish -- the innerShiv branch is important for these edge cases, but the createElement duckpunch makes the innerHTML fix more accessible to devs and causes surprisingly few issues, so I think it's worth keeping in the mainline.

Collaborator

redoPop commented Jan 24, 2012

I agree with @paulirish -- the innerShiv branch is important for these edge cases, but the createElement duckpunch makes the innerHTML fix more accessible to devs and causes surprisingly few issues, so I think it's worth keeping in the mainline.

@aFarkas

This comment has been minimized.

Show comment
Hide comment
@aFarkas

aFarkas Jan 24, 2012

Owner

Ok, this means:

I work on improving performance next days. + We need a solution for those people having those issues. Which should be an optout configuration.

Something like:

html5shiv.noDynamic = true;

Do you have any suggestions on naming this option?

Owner

aFarkas commented Jan 24, 2012

Ok, this means:

I work on improving performance next days. + We need a solution for those people having those issues. Which should be an optout configuration.

Something like:

html5shiv.noDynamic = true;

Do you have any suggestions on naming this option?

@remy

This comment has been minimized.

Show comment
Hide comment
@remy

remy Jan 24, 2012

Collaborator

What's the actual code in highcharts trigging this issue?

I'm just wary that adding a config option may be overlooked and a developer would just get stuck looking at grey charts.

(I can see a counter argument btw)

On 24 Jan 2012, at 23:09, Alexander Farkas wrote:

Ok, this means:

I work on improving performance next days. + We need a solution for those people having those issues. Which should be an optout configuration.

Something like:

html5shiv.noDynamic = true;

Do you have any suggestions on naming this option?


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

Collaborator

remy commented Jan 24, 2012

What's the actual code in highcharts trigging this issue?

I'm just wary that adding a config option may be overlooked and a developer would just get stuck looking at grey charts.

(I can see a counter argument btw)

On 24 Jan 2012, at 23:09, Alexander Farkas wrote:

Ok, this means:

I work on improving performance next days. + We need a solution for those people having those issues. Which should be an optout configuration.

Something like:

html5shiv.noDynamic = true;

Do you have any suggestions on naming this option?


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

@jonathantneal

This comment has been minimized.

Show comment
Hide comment
@jonathantneal

jonathantneal Jan 24, 2012

Collaborator

I too would love to know what's actually causing the problem.

Collaborator

jonathantneal commented Jan 24, 2012

I too would love to know what's actually causing the problem.

@redoPop

This comment has been minimized.

Show comment
Hide comment
@redoPop

redoPop Jan 24, 2012

Collaborator

+1 opt-out configuration! How about:

html5shiv.fixDomMethods = false;
Collaborator

redoPop commented Jan 24, 2012

+1 opt-out configuration! How about:

html5shiv.fixDomMethods = false;
@remy

This comment has been minimized.

Show comment
Hide comment
@remy

remy Jan 24, 2012

Collaborator

I can see all kinds of dumb with this, but it's kinda useful

html5shiv.compatibilityMode = true;

It's suitable vague and allows you to fix things in the future too.

On 24 Jan 2012, at 23:19, joe bartlett wrote:

+1 opt-out configuration! How about:

html5shiv.fixDomMethods = false;


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

Collaborator

remy commented Jan 24, 2012

I can see all kinds of dumb with this, but it's kinda useful

html5shiv.compatibilityMode = true;

It's suitable vague and allows you to fix things in the future too.

On 24 Jan 2012, at 23:19, joe bartlett wrote:

+1 opt-out configuration! How about:

html5shiv.fixDomMethods = false;


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

@jonathantneal

This comment has been minimized.

Show comment
Hide comment
@jonathantneal

jonathantneal Jan 24, 2012

Collaborator

For performance we could cut documentCreateElementReplaceFunction and change the way we shiv a document to not use replace and use a for loop. This would remove a function, a join, and a replace; and add a for loop.

for (var i = 0, l = html5.elements.length; i < l; ++i) {
    documentCreateElement(html5.elements[i]);
}
Collaborator

jonathantneal commented Jan 24, 2012

For performance we could cut documentCreateElementReplaceFunction and change the way we shiv a document to not use replace and use a for loop. This would remove a function, a join, and a replace; and add a for loop.

for (var i = 0, l = html5.elements.length; i < l; ++i) {
    documentCreateElement(html5.elements[i]);
}
@aFarkas

This comment has been minimized.

Show comment
Hide comment
@aFarkas

aFarkas Jan 24, 2012

Owner

I can not point you to the exact code of highcharts, but the following commit fixes this issue.

@jonathantneal

Yeah, I made a performance test on this and this makes html5shiv significant faster. We can also split the shivDocument into to parts. shivDommethods and shivHoleDocument, with this we won't search for head element so often.

Owner

aFarkas commented Jan 24, 2012

I can not point you to the exact code of highcharts, but the following commit fixes this issue.

@jonathantneal

Yeah, I made a performance test on this and this makes html5shiv significant faster. We can also split the shivDocument into to parts. shivDommethods and shivHoleDocument, with this we won't search for head element so often.

@jonathantneal

This comment has been minimized.

Show comment
Hide comment
@jonathantneal

jonathantneal Jan 24, 2012

Collaborator

Alex, your commit basically tells the shiv not to shiv a element. Should we maintain a list of unshiv-worthy elements or is this a very specific patch for a very specific element?

From what Google tells me, <fill> is a VML element. So, if IE is having issues with shived VML elements then we could either create a list of elements we will not fill or have some kind of detection for VML elements.

Collaborator

jonathantneal commented Jan 24, 2012

Alex, your commit basically tells the shiv not to shiv a element. Should we maintain a list of unshiv-worthy elements or is this a very specific patch for a very specific element?

From what Google tells me, <fill> is a VML element. So, if IE is having issues with shived VML elements then we could either create a list of elements we will not fill or have some kind of detection for VML elements.

@remy

This comment has been minimized.

Show comment
Hide comment
@remy

remy Jan 24, 2012

Collaborator

Initially I'm thinking it feels wrong to maintain a list of unshivable elements - but on the heavy other hand, this is the /only/ bug raised on the new shiv.

Arguably if it kills the issue - and there's no more or a very limited number of additions, as clunky as it might seem, I think it's the right move.

On 24 Jan 2012, at 23:31, Jonathan wrote:

Alex, your commit basically tells the shiv not to shiv a element. Should we maintain a list of unshiv-worthy elements or is this a very specific patch for a very specific element?


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

Collaborator

remy commented Jan 24, 2012

Initially I'm thinking it feels wrong to maintain a list of unshivable elements - but on the heavy other hand, this is the /only/ bug raised on the new shiv.

Arguably if it kills the issue - and there's no more or a very limited number of additions, as clunky as it might seem, I think it's the right move.

On 24 Jan 2012, at 23:31, Jonathan wrote:

Alex, your commit basically tells the shiv not to shiv a element. Should we maintain a list of unshiv-worthy elements or is this a very specific patch for a very specific element?


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

@jonathantneal

This comment has been minimized.

Show comment
Hide comment
@jonathantneal

jonathantneal Jan 24, 2012

Collaborator

VML elements may also respond to a check of tagUrn.

if (element.canHaveChildren && element.tagUrn !== 'urn:schemas-microsoft-com:vml') {
    html5.shivDocument(element.document);
}
Collaborator

jonathantneal commented Jan 24, 2012

VML elements may also respond to a check of tagUrn.

if (element.canHaveChildren && element.tagUrn !== 'urn:schemas-microsoft-com:vml') {
    html5.shivDocument(element.document);
}
@remy

This comment has been minimized.

Show comment
Hide comment
@remy

remy Jan 24, 2012

Collaborator

If that's the bad boy causing all this fuss, then I like it.

I was thinking whether this might affect SVG, but at the same time, SVG isn't supported in IE below 9 - which doesn't need the html5 shiv - so ... we're okay?

Collaborator

remy commented Jan 24, 2012

If that's the bad boy causing all this fuss, then I like it.

I was thinking whether this might affect SVG, but at the same time, SVG isn't supported in IE below 9 - which doesn't need the html5 shiv - so ... we're okay?

@aFarkas

This comment has been minimized.

Show comment
Hide comment
@aFarkas

aFarkas Jan 25, 2012

Owner

My checkin was stupid, did the exact opposite, of what I thought. tagUrn is always an empty string inside of the highcharts demo.

I will look deeper into this issue. Beside this, we have the optout. I have called it fixDomMethods, but I'm open to change it.

Owner

aFarkas commented Jan 25, 2012

My checkin was stupid, did the exact opposite, of what I thought. tagUrn is always an empty string inside of the highcharts demo.

I will look deeper into this issue. Beside this, we have the optout. I have called it fixDomMethods, but I'm open to change it.

@jonathantneal

This comment has been minimized.

Show comment
Hide comment
@jonathantneal

jonathantneal Jan 25, 2012

Collaborator

You've made the two files so different and now I don't have time to clean this up. I want it cleaned up badly.

Collaborator

jonathantneal commented Jan 25, 2012

You've made the two files so different and now I don't have time to clean this up. I want it cleaned up badly.

@jdalton

This comment has been minimized.

Show comment
Hide comment
@jdalton

jdalton Jan 25, 2012

Contributor

1) I don't think the populated parentNode concern is a big one.

If you work with IE at all you know of its issues:

<p id="foo">Hello World</p>
<script>
var elem = document.createElement('div');
var before = elem.parentNode;
elem.document;
var after = elem.parentNode;

// alerts smth like [ null, [object], null ] in IE < 9
alert(['' + before, '' + after, '' + elem.parentElement]);

var elem = document.getElementById('foo');
before = elem.parentNode;
document.body.removeChild(elem);
after = elem.parentNode;

// alerts smth like [ [object], [object], null ] in IE < 9
alert(['' + before, '' + after, '' + elem.parentElement]);
</script>

and should know that elem.parentElement is a better check of if the element has a parent node in cases where it matters.

Contributor

jdalton commented Jan 25, 2012

1) I don't think the populated parentNode concern is a big one.

If you work with IE at all you know of its issues:

<p id="foo">Hello World</p>
<script>
var elem = document.createElement('div');
var before = elem.parentNode;
elem.document;
var after = elem.parentNode;

// alerts smth like [ null, [object], null ] in IE < 9
alert(['' + before, '' + after, '' + elem.parentElement]);

var elem = document.getElementById('foo');
before = elem.parentNode;
document.body.removeChild(elem);
after = elem.parentNode;

// alerts smth like [ [object], [object], null ] in IE < 9
alert(['' + before, '' + after, '' + elem.parentElement]);
</script>

and should know that elem.parentElement is a better check of if the element has a parent node in cases where it matters.

@jdalton

This comment has been minimized.

Show comment
Hide comment
@jdalton

jdalton Jan 25, 2012

Contributor

2) Has anyone contacted Highcharts as this may be a bug on their part?

I haven't worked much with VML so take this with a grain of salt, but when I have and in all the snippets I've quickly Google'd where other devs create VML elements a namespace, like v:, is used:

// something like this
doc.createElement('<rvml:' + tagName + ' class="rvml">');
// or
doc.createElement('<' + tagName + ' xmlns="urn:schemas-microsoft.com:vml" class="rvml">');
// or this
function pushElement( name ) {
  var attrs = me[ attrsPrefix + name ];
  if( attrs ) {
    m.push( tagStart + name );
    pushAttrs( attrs );
    m.push( subElEnd );
  }
}
var nsPrefix = 'pievml';
var tagStart = '<' + nsPrefix + ':';
var m = [ tagStart, tag, ' id="', me.elId, '" style="', me.getStyleCssText(), '" ', me.defaultAttrs ];
pushAttrs( me[ attrsPrefix ] );
m.push( '>' );
pushElement( 'fill' );
m.push( '</' + nsPrefix + ':' + tag + '>' );
var result = m.join( '' );

(also I did some basic tests in IE8 and a namespace was required to get the fill to render as well)

Contributor

jdalton commented Jan 25, 2012

2) Has anyone contacted Highcharts as this may be a bug on their part?

I haven't worked much with VML so take this with a grain of salt, but when I have and in all the snippets I've quickly Google'd where other devs create VML elements a namespace, like v:, is used:

// something like this
doc.createElement('<rvml:' + tagName + ' class="rvml">');
// or
doc.createElement('<' + tagName + ' xmlns="urn:schemas-microsoft.com:vml" class="rvml">');
// or this
function pushElement( name ) {
  var attrs = me[ attrsPrefix + name ];
  if( attrs ) {
    m.push( tagStart + name );
    pushAttrs( attrs );
    m.push( subElEnd );
  }
}
var nsPrefix = 'pievml';
var tagStart = '<' + nsPrefix + ':';
var m = [ tagStart, tag, ' id="', me.elId, '" style="', me.getStyleCssText(), '" ', me.defaultAttrs ];
pushAttrs( me[ attrsPrefix ] );
m.push( '>' );
pushElement( 'fill' );
m.push( '</' + nsPrefix + ':' + tag + '>' );
var result = m.join( '' );

(also I did some basic tests in IE8 and a namespace was required to get the fill to render as well)

@jdalton

This comment has been minimized.

Show comment
Hide comment
@jdalton

jdalton Jan 25, 2012

Contributor

3) We should add unit tests for creating elements like document.createElement('<div>') which is allowed in IE < 9.

Contributor

jdalton commented Jan 25, 2012

3) We should add unit tests for creating elements like document.createElement('<div>') which is allowed in IE < 9.

@jdalton

This comment has been minimized.

Show comment
Hide comment
@jdalton

jdalton Jan 25, 2012

Contributor

Here is a post referencing errors creating fill and stroke elements in IE before the HTML5shiv update:
(though no details on how to recreate the error)

Contributor

jdalton commented Jan 25, 2012

Here is a post referencing errors creating fill and stroke elements in IE before the HTML5shiv update:
(though no details on how to recreate the error)

@patrickberkeley

This comment has been minimized.

Show comment
Hide comment
@patrickberkeley

patrickberkeley Jan 25, 2012

@jdalton Here's the link to the Highcharts forum post on this issue. It's gotten no love.

@jdalton Here's the link to the Highcharts forum post on this issue. It's gotten no love.

@jdalton

This comment has been minimized.

Show comment
Hide comment
@jdalton

jdalton Jan 25, 2012

Contributor

So I dug into this and Highcharts is doing smth like:

document.createElement('<stroke color="black" opacity="0.1" xmlns="urn:schemas-microsoft-com:vml"  style="display:inline-block;behavior:url(#default#VML);"/>');

Simply replacing the element.document.createElement and restoring it is enough to break it.

element.document.createElement = [element.document.createElement, element.document.createElement=function(){}][0];

@aFarkas' patch checking for xmlns and tagUrn should be alright as the element will have one or the other. Though it could be simplified to just:

if (html5.fixDomMethods && element.canHaveChildren && !(element.xmlns || element.tagUrn)){
  html5.shivDom(element.document);  
} 

to avoid other potential gotchas.

Contributor

jdalton commented Jan 25, 2012

So I dug into this and Highcharts is doing smth like:

document.createElement('<stroke color="black" opacity="0.1" xmlns="urn:schemas-microsoft-com:vml"  style="display:inline-block;behavior:url(#default#VML);"/>');

Simply replacing the element.document.createElement and restoring it is enough to break it.

element.document.createElement = [element.document.createElement, element.document.createElement=function(){}][0];

@aFarkas' patch checking for xmlns and tagUrn should be alright as the element will have one or the other. Though it could be simplified to just:

if (html5.fixDomMethods && element.canHaveChildren && !(element.xmlns || element.tagUrn)){
  html5.shivDom(element.document);  
} 

to avoid other potential gotchas.

@jdalton

This comment has been minimized.

Show comment
Hide comment
@jdalton

jdalton Jan 25, 2012

Contributor

Here is a page to experiment with:
http://jsbin.com/esihak

Contributor

jdalton commented Jan 25, 2012

Here is a page to experiment with:
http://jsbin.com/esihak

@paulirish paulirish referenced this issue in Modernizr/Modernizr Jan 29, 2012

Closed

evaluate new html5shiv #426

@aFarkas aFarkas closed this Jan 30, 2012

@paulirish

This comment has been minimized.

Show comment
Hide comment
@paulirish

paulirish Jan 30, 2012

Collaborator

Sweet. afarkas commited a fix in c993fe0 then shorted it with jdalton's help: aaeb7da

Lastly added a test in 65f83d8 and closed the issue. Boom, lookin good.

Thanks everyone!

Collaborator

paulirish commented Jan 30, 2012

Sweet. afarkas commited a fix in c993fe0 then shorted it with jdalton's help: aaeb7da

Lastly added a test in 65f83d8 and closed the issue. Boom, lookin good.

Thanks everyone!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment