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

Chart will still trigger reflow after destoryed when window resize in internet explorer 8 #6746

Closed
Pennnn opened this issue May 19, 2017 · 8 comments
Assignees
Labels

Comments

@Pennnn
Copy link

@Pennnn Pennnn commented May 19, 2017

Expected behaviour

In IE8, it should not trigger the reflow event when the chart has been destroyed.

Actual behaviour

In IE8, after the chart has been destroyed, but the event attach to window hasn't been correctly removed;
so it still invoke the reflow when window is resizing.

Live demo with steps to reproduce

Affected browser(s)

Internet explorer 8

@KacperMadej
Copy link
Contributor

@KacperMadej KacperMadej commented May 22, 2017

Hi Pennnn,

Thank you for reporting about the problem. However, I am unable to recreate the issue. Here's the code I used:

<html> 
  <head> 
    <meta http-equiv="X-UA-Compatible" content="IE=8" />
    <script src="http://ajax.googleapis.com/ajax/libs/jquery/1.9.1/jquery.min.js"> 
    </script>
    <script src="http://code.highcharts.com/highcharts.src.js"></script>
    <script src="http://code.highcharts.com/modules/exporting.src.js"></script>
  </head> 
  <body>
    <button id="button">Destroy the chart</button>
    <div id="container" style="height: 400px; margin: 0 auto;"></div>
    <script>
      Highcharts.chart('container', {
        series: [{
          data: [1,2,3]
        }]
      }, function (chart) {
        // the button handler
        $('#button').click(function() {
          chart.destroy();
          $(this).attr('disabled', true);
        });
      });
    </script>
  </body> 
</html>

Steps:

  1. Press the button
  2. Resize window

All is fine, no error.

@Pennnn Please provide a demo that will show the problem or a way to recreate, test and debug the issue.

@sebastianbochan
Copy link
Contributor

@sebastianbochan sebastianbochan commented May 23, 2017

@Pennnn please supply us a full number of IE8 version.

@Pennnn
Copy link
Author

@Pennnn Pennnn commented May 23, 2017

<html> 
  <head> 
     <meta http-equiv="X-UA-Compatible" content="IE=8" />
    <script src="http://ajax.googleapis.com/ajax/libs/jquery/1.9.1/jquery.min.js"> 
    </script>
    <script src="http://code.highcharts.com/highcharts.src.js"></script>
    <script src="http://code.highcharts.com/modules/exporting.src.js"></script>
  </head> 
  <body>
    <button id="button">Destroy the chart</button>
    <button id="createBtn">create the chart</button>
    <div id="container1" style="height: 400px; margin: 0 auto;"></div>
    <div id="container2" style="height: 400px; margin: 0 auto;"></div>
    <div id="container3" style="height: 400px; margin: 0 auto;"></div>
    <script>
    $(function() {
      var charts = [];
      var createChart = function(id) {
        return new Highcharts.Chart({
            chart : {
              defaultSeriesType : 'pie',
              renderTo: id,
            },
            series: [{
              data: [1,2,3]
            }]
          });
      };
      var createMutipleCharts = function() {
        $.each([1, 2, 3], function(index, item){
          charts.push(createChart('container' + item));
        });
      }

      $('#createBtn').click(createMutipleCharts);
      $('#button').click(function() {
          var c;
          while(c = charts.pop()) {
            c.destroy();
          }
      });
    
      createMutipleCharts();
    });
    </script>
  </body> 
</html>

@KacperMadej thanks for your reply.
I found that this will only occurs when there are mutiple charts.
you can reproduce by step below.
Steps:

  1. Press the button 'Destroy the chart'
  2. resize window
    then it will throw error: 'options.chart' cannot be undefined in line 16047
    But this plays well in chrome.
@Pennnn
Copy link
Author

@Pennnn Pennnn commented May 23, 2017

@sebastianbochan the version is 8.0.7601.17514

@Pennnn
Copy link
Author

@Pennnn Pennnn commented May 23, 2017

 H.addEvent = function(el, type, fn) {
    
    var events = el.hcEvents = el.hcEvents || {};
    
    function wrappedFn(e) {
        e.target = e.srcElement || win; // #2820
        fn.call(el, e);
    }
    
    // Handle DOM events in modern browsers
    if (el.addEventListener) {
        el.addEventListener(type, fn, false);
        
        // Handle old IE implementation
    } else if (el.attachEvent) {
        
        if (!el.hcEventsIE) {
            el.hcEventsIE = {};
        }

        // Link wrapped fn with original fn, so we can get this in removeEvent
        el.hcEventsIE[fn.toString()] = wrappedFn;

        el.attachEvent('on' + type, wrappedFn);
    }
    
    if (!events[type]) {
        events[type] = [];
    }
    
    events[type].push(fn);
    
    // Return a function that can be called to remove this event.
    return function() {
        H.removeEvent(el, type, fn);
    };
};

I think el.hcEventsIE[fn.toString()] = wrappedFn; cause this problem.
When you create mutiple charts, it will overide the previous one because fn.toString()is the same.
So the 'resize' event cannot be correctly removed except the final one;

@KacperMadej
Copy link
Contributor

@KacperMadej KacperMadej commented May 23, 2017

@Pennnn Thank you very much for the debugging and help. I was also able to check that only one event is being removed, so in case of 3 charts - 2 events are not removed and are causing errors.

The problem seems to be as you have explained.

@fredure
Copy link

@fredure fredure commented Aug 6, 2019

@KacperMadej @TorsteinHonsi
Hello, this error is repeated if there are 4 diagrams on the page in IE11.
Will this commit fix the bug?
073df36

@pawelfus
Copy link
Contributor

@pawelfus pawelfus commented Aug 6, 2019

It seems that IE11 was just updated (?) and broke something - just today, you are the third user reporting this issue, see #11609 - @KacperMadej is investigating the issue, will update #11609 ticket once he's ready. Please follow #11609

Edit:
Wrong ticket number, should be #11609 - updated.

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

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.