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

If specified the "column" for each node, the links between nodes are not drawn correctly #8865

Closed
zlateskud opened this Issue Aug 30, 2018 · 13 comments

Comments

Projects
None yet
7 participants
@zlateskud

zlateskud commented Aug 30, 2018

Hello,

We have a problem with the sankey diagram.

If specified the "column" for each node, the links between nodes are not drawn correctly.
Besides that, in Internet Explorer 11 compatibility mode 7, using oldie.js the graphic isn't showing at all if is specified the column. Without it everything looks ok.

In attachment you find the code example

Some images on different browsers:
1)With no colun in Chrome

normal_chrome

2)With no colun in IE11/CM7

normal

3)Column specified in Chrome

cs_chrome

4)Column specified in IE11/CM7

cs_oldie

Live demo with steps to reproduce

http://jsfiddle.net/0j8hds9u/4/

Product version

Highcharts JS v6.1.1 (2018-06-27)

Affected browser(s)

Chrome 68 and IE11 Compatibility mode 7

Thank you.

@KacperMadej

This comment has been minimized.

Show comment
Hide comment
@KacperMadej

KacperMadej Aug 31, 2018

Contributor

Hi @zlateskud

Thank you for reporting about the bug.

After debugging in IE7 looks like there's a problem with forEachPolyfill.
It should:

executes the provided callback once for each element present in the array in ascending order. It is not invoked for index properties that have been deleted or are uninitialized

but it runs for all array indexes from 0 to length-1

		    H.forEachPolyfill = function (fn, ctx) {
		        var i = 0,
		            len = this.length;
		        for (; i < len; i++) {
		            if (fn.call(ctx, this[i], i, this) === false) {
		                return i;
		            }
		        }
		    };

As a workaround:

use this code after loading oldie module, but before creating any charts:

Highcharts.forEachPolyfill = function (fn, ctx) {
    var i = 0,
        len = this.length;
    for (; i < len; i++) {
        if (
            this[i] !== undefined && // added check
            fn.call(ctx, this[i], i, this) === false
        ) {
            return i;
        }
    }
};
Contributor

KacperMadej commented Aug 31, 2018

Hi @zlateskud

Thank you for reporting about the bug.

After debugging in IE7 looks like there's a problem with forEachPolyfill.
It should:

executes the provided callback once for each element present in the array in ascending order. It is not invoked for index properties that have been deleted or are uninitialized

but it runs for all array indexes from 0 to length-1

		    H.forEachPolyfill = function (fn, ctx) {
		        var i = 0,
		            len = this.length;
		        for (; i < len; i++) {
		            if (fn.call(ctx, this[i], i, this) === false) {
		                return i;
		            }
		        }
		    };

As a workaround:

use this code after loading oldie module, but before creating any charts:

Highcharts.forEachPolyfill = function (fn, ctx) {
    var i = 0,
        len = this.length;
    for (; i < len; i++) {
        if (
            this[i] !== undefined && // added check
            fn.call(ctx, this[i], i, this) === false
        ) {
            return i;
        }
    }
};

@KacperMadej KacperMadej added the Bug label Aug 31, 2018

@zlateskud

This comment has been minimized.

Show comment
Hide comment
@zlateskud

zlateskud Sep 3, 2018

Hi @KacperMadej ,
this solves the problem in IE11/CM7, but remain the problem with the column.
If specified the "column" for each node, the links between nodes are not drawn correctly.

a1

Thank you.

zlateskud commented Sep 3, 2018

Hi @KacperMadej ,
this solves the problem in IE11/CM7, but remain the problem with the column.
If specified the "column" for each node, the links between nodes are not drawn correctly.

a1

Thank you.

@pawelfus

This comment has been minimized.

Show comment
Hide comment
@pawelfus
Contributor

pawelfus commented Sep 3, 2018

@TorsteinHonsi

This comment has been minimized.

Show comment
Hide comment
@TorsteinHonsi

TorsteinHonsi Sep 5, 2018

Collaborator

I think the problem with the badly drawn columns is that you start by assigning nodes to column 1, and there are not nodes assigned to column 0. When starting from 0, it looks correct: http://jsfiddle.net/highcharts/0j8hds9u/18/

Collaborator

TorsteinHonsi commented Sep 5, 2018

I think the problem with the badly drawn columns is that you start by assigning nodes to column 1, and there are not nodes assigned to column 0. When starting from 0, it looks correct: http://jsfiddle.net/highcharts/0j8hds9u/18/

@tynorton

This comment has been minimized.

Show comment
Hide comment
@tynorton

tynorton Sep 26, 2018

This change appears to have broken our column chart plotlines.

image

Is there a case tracking this regression?

tynorton commented Sep 26, 2018

This change appears to have broken our column chart plotlines.

image

Is there a case tracking this regression?

@KacperMadej

This comment has been minimized.

Show comment
Hide comment
@KacperMadej

KacperMadej Sep 26, 2018

Contributor

@tynorton Are you sure this is caused by the fix of this issue? Please create a live demo and add more info about used browser.

Contributor

KacperMadej commented Sep 26, 2018

@tynorton Are you sure this is caused by the fix of this issue? Please create a live demo and add more info about used browser.

@tynorton

This comment has been minimized.

Show comment
Hide comment
@tynorton

tynorton Sep 26, 2018

Maybe not this issue, definitely part of 6.1.4.

tynorton commented Sep 26, 2018

Maybe not this issue, definitely part of 6.1.4.

@keeslinp

This comment has been minimized.

Show comment
Hide comment
@keeslinp

keeslinp Sep 26, 2018

I think 6.1.4 broke stuff for us as well. Our tests started failing with an error m.getSpanWidth is not a function in highcharts. highcharts is a transitive dependency of react-highcharts which doesn't have its dependencies locked down so we were updated unexpectedly.

keeslinp commented Sep 26, 2018

I think 6.1.4 broke stuff for us as well. Our tests started failing with an error m.getSpanWidth is not a function in highcharts. highcharts is a transitive dependency of react-highcharts which doesn't have its dependencies locked down so we were updated unexpectedly.

@KacperMadej

This comment has been minimized.

Show comment
Hide comment
@KacperMadej

KacperMadej Sep 27, 2018

Contributor

@keeslinp Seems to be a react-highcharts issue, so you should probably report this to the authors of that software - it's not an official react wrapper. Without a live demo there's nothing for us to debug, identify and fix, so please provide a way for us to work with a demo showing the problem.

Contributor

KacperMadej commented Sep 27, 2018

@keeslinp Seems to be a react-highcharts issue, so you should probably report this to the authors of that software - it's not an official react wrapper. Without a live demo there's nothing for us to debug, identify and fix, so please provide a way for us to work with a demo showing the problem.

@keeslinp

This comment has been minimized.

Show comment
Hide comment
@keeslinp

keeslinp Sep 27, 2018

keeslinp commented Sep 27, 2018

@TorsteinHonsi

This comment has been minimized.

Show comment
Hide comment
@TorsteinHonsi

TorsteinHonsi Sep 28, 2018

Collaborator

Our tests started failing with an error m.getSpanWidth is not a function in highcharts.

See #9058, it seems that the react tests run jsdom in the background, and jsdom doesn't support tspan.getSubStringLength.

Collaborator

TorsteinHonsi commented Sep 28, 2018

Our tests started failing with an error m.getSpanWidth is not a function in highcharts.

See #9058, it seems that the react tests run jsdom in the background, and jsdom doesn't support tspan.getSubStringLength.

@maljones

This comment has been minimized.

Show comment
Hide comment
@maljones

maljones Sep 28, 2018

@tynorton's issue looks somewhat similar to mine. Since the 6.1.4 fix, line charts have filled areas: https://www.ecosresults.org/states/alabama/. Rolling back to 6.1.3 gives the lines back there proper dimensions (although it's ignoring my series styling when I do that).

ecosresults-linechart-err

Browsers:

Chrome Mac: Version 69.0.3497.100 (Official Build) (64-bit)
Firefox Mac: 62.0
Client reported it on iOS Safari: Unsure which version

maljones commented Sep 28, 2018

@tynorton's issue looks somewhat similar to mine. Since the 6.1.4 fix, line charts have filled areas: https://www.ecosresults.org/states/alabama/. Rolling back to 6.1.3 gives the lines back there proper dimensions (although it's ignoring my series styling when I do that).

ecosresults-linechart-err

Browsers:

Chrome Mac: Version 69.0.3497.100 (Official Build) (64-bit)
Firefox Mac: 62.0
Client reported it on iOS Safari: Unsure which version

@KacperMadej

This comment has been minimized.

Show comment
Hide comment
@KacperMadej

KacperMadej Oct 1, 2018

Contributor

Hi @maljones

If you would like to report a bug please open a new issue - your last comment doesn't look like related to the original issue reported here, so it would be more readable if it were a separate issue. If you have a problem, but are not sure if it's a bug please contact technical support first to confirm - https://www.highcharts.com/support

Contributor

KacperMadej commented Oct 1, 2018

Hi @maljones

If you would like to report a bug please open a new issue - your last comment doesn't look like related to the original issue reported here, so it would be more readable if it were a separate issue. If you have a problem, but are not sure if it's a bug please contact technical support first to confirm - https://www.highcharts.com/support

pinturic added a commit to pinturic/react-highcharts that referenced this issue Oct 9, 2018

The highcharts version has been dumped
It has been fiexd to 6.1.3 to overcome this problem: highcharts/highcharts#8865 . We should wait for a solution or include the ie polyfill: highcharts/highcharts@7078963#diff-c1ddc7f79265aa5b49d13c31eeb19caf
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment