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 zoom is not smooth #15569

Closed
govinda18 opened this issue Apr 23, 2021 · 26 comments · Fixed by #19083
Closed

Chart zoom is not smooth #15569

govinda18 opened this issue Apr 23, 2021 · 26 comments · Fixed by #19083

Comments

@govinda18
Copy link

Expected behaviour

Chart zoom should be smooth

Actual behaviour

We have encountered this issue mostly with stock charts with large dataset. When we zoom into the chart with a relatively faster mouse movement, the selection often gets stuck.

Right now, mouse-zooming charts is done by dragging the mouse inside the axes area. Unfortunately, dragging beyond the edge of that area stops the JS code from receiving events (or something like that) and it's extremely hard to get the selection to go all the way to the left or right edge. When we move the mouse slowly, we are able to select that. Refer the following gif:

gif

Live demo with steps to reproduce

https://jsfiddle.net/o3pnxkjw/

Product version

Highstock 9.0.1

Affected browser(s)

Browser

@highsoft-bot highsoft-bot added this to To do in Development-Flow Apr 23, 2021
@KacperMadej
Copy link

Hi @govinda18

I am able to recreate this problem only when moving the cursor extremely fast. I can only imagine doing so for the sake of recreating the problem, so maybe there are some real-life scenarios that you could share when this would really be a problem?

Only the mouse move events inside of the chart's container are registered for the drag, so you could also hold the zoom selection and go under the chart, exit the container while still dragging, and move horizontally under the chart - the zoom selection will not be changed until you return into the chart's container.

@pawelfus
Copy link
Contributor

pawelfus commented Apr 28, 2021

Not related to Highcharts Stock only: https://jsfiddle.net/BlackLabel/4xdsr8ov/

Isn't that a problem that happens in jsFiddle? Events from the left iframe are not available in the results frame. It doesn't have to be a fast move to the left, simply move to the left, then release the mouse button (in the code tab), go back to the chart. The selection marker will still follow the mouse pointer.

Regarding this:

dragging beyond the edge of that area stops the JS code from receiving events (or something like that)

We are limited by events processed by the browser. The same happens when you move your mouse really fast, out of the browser. Check this short description of why it happens: #12998 (comment)

@govinda18
Copy link
Author

govinda18 commented Apr 29, 2021

so maybe there are some real-life scenarios that you could share when this would really be a problem?

So we have build a wrapper to render highcharts in python frameworks such as JupyterLab. This problem scales up somehow there.

It is a bad UX as it seems pretty natural for people not using range selector to view say the last 7 days of financial data and quickly move the mouse to the left or right edge of the chart. Even while moving slow, we have observed times when we are just not able to move to the very end of the chart.

@TorsteinHonsi
Copy link
Collaborator

@pawelfus Can you look into this again?

Maybe the mouseup position currently used is the last mouse position inside the plot area? Is that how mouseleave works? And if so, maybe we should look out the first mouse position outside too.

@pawelfus
Copy link
Contributor

Sure thing!

@govinda18 could you try this plugin? Load this plugin after loading Highcharts and other plugins.
Demo: https://jsfiddle.net/BlackLabel/wcLzhq71/
Plugin:

(function(H) {
  H.wrap(
    H.Pointer.prototype,
    'onContainerMouseDown',
    function(proceed, e) {
      const prevDef = e.preventDefault;

      e.preventDefault = () => {};

      const ret = proceed.call(this, e);

      e.preventDefault = prevDef.bind(e);

      return ret;
    }
  )
})(Highcharts);

If this doesn't help, could you reupload the GIF?

@rdp1414
Copy link

rdp1414 commented Apr 24, 2023

Hello @pawelfus ,

Our stakeholders have been stressing on this problem of unable to zoom to right-most points since a year back; and in the last week, they spot-lighted this bug once again as critical-immediate-fix because they almost misinterpreted their financial zoomed data, by saying:

When we are looking at financial stats, missing the most recent point makes the chart miss the thing you care the most about. Please be careful to ensure all the datapoints get into the zoomed charts. Including the very last data point in the chart, which for financial purposes, is often very important. Dropping even a single point is not acceptable.

I created a reproducer - See demo fiddle. When you try zooming (briskly), the selection skips points near the right-end as shown below:
hc-fast-zooming

Is there a way to leverage into internal highcharts functionality to do the below:

  1. If mouseDown happens inside the container of chart and when the subsequent mouseUp event is fired at a certain x co-ordinate which is greater than the x co-ordinate of the chart's right boundary, then we zoom till the very end.
  2. Otherwise, if mouseDown happens inside the chart and so the subsequent mouseUp, then nothing is to be done

To do the above, knowledge of internal highcharts working is required, that is why I'm requesting some help / assistance from highcharts developers.

Thank You,
Rajdeep

@rdp1414
Copy link

rdp1414 commented Apr 24, 2023

Unable to zoom quickly is an issue, but the bigger issue is that, even if we zoom slowly, still the very last date point is dropped - See #18863.

Just to mention: In the above example gif, I used Highcharts with datatime as xAxis.type for giving you reproducer; but we use stockChart for our datetime-financial charts.

@pawelfus
Copy link
Contributor

Hi @rdp1414

Thank you for providing a great demo to reproduce this! In your case, it is a bit different issue: the mouseup happens inside the window, so we are able to change this. Please check this: https://jsfiddle.net/BlackLabel/ezxbpqmh/1/

Plugin:

(function(H) {
  H.wrap(
    H.Pointer.prototype,
    'onContainerMouseLeave',
    function(proceed, e) {
		
      this.onContainerMouseMove(e);
			
      return proceed.apply(this, Array.prototype.slice.call(arguments));
    }
  )
})(Highcharts);

Could you please test it and let us know the results? If that resolves the issue, we will implement this in the library too.
If not, please check also alternative solution: https://jsfiddle.net/BlackLabel/1bwyj2et/

Plugin:

(function(H) {
  H.wrap(
    H.Pointer.prototype,
    'setDOMEvents',
    function(proceed) {
      const ret = proceed.apply(this, Array.prototype.slice.call(arguments));

      // Move mousemove event from container, to document:
      const pointer = this;

      pointer.chart.container.onmousemove = Highcharts.noop;
      Highcharts.addEvent(
        pointer.chart.container.ownerDocument,
        'mousemove',
        function() {
          pointer.onContainerMouseMove(...arguments);
        }
      );

      return ret;
    }
  )
})(Highcharts);

@rdp1414
Copy link

rdp1414 commented Apr 25, 2023

Hello @pawelfus ,

First of all, thank you for replying back promptly. There are 3 sub-problems here which I explained in the code-comments and applied solutions given by you for 2 of them and 1 is still waiting- See demo fiddle.

Could you please test it and let us know the results?

Yes, I tested out both plugins provided by you in our highcharts project and faced following problems (we're using highstock 10.3.3):

Plugin-1

When I try to zoom till the right-extreme end, the area is selected but zoom doesn't happen. While trying to zoom within the bounds of the chart, it works as normal:
plugin1

Plugin-2

No matter where I hover mouse on the screen, even if not on the chart, Large number (more then 1000) of below errors are logged on the console:
image
Plus, after 1 or 2 seconds, the place where chart was displayed, there the below errors are shown:

←→1 of 126 errors on the page

TypeError: Cannot read properties of undefined (reading 'tooltip')

Pointer.onContainerMouseMove
.../node_modules/highcharts/highstock.src.js:27845
   27842 |  */
   27843 | Pointer.prototype.onContainerMouseMove = function (e) {
   27844 |     var chart = this.chart,
> 27845 |             tooltip = chart.tooltip,
   27876  | ^         pEvt = this.normalize(e);
   27847 |     this.setHoverChartIndex();
   27848 |     // In IE8 we apparently need this returnValue set to false in order to



View compiled
HTMLDocument.mouseMoveHandler
<the file where this plugin was written>.js:76
   73 |     pointer.chart.container.onmousemove = Highcharts.noop;
   74 |     Highcharts.addEvent(pointer.chart.container.ownerDocument, 'mousemove', function mouseMoveHandler() {
   75 |         // eslint-disable-next-line prefer-rest-params
> 76 |         pointer.onContainerMouseMove(...arguments);
   77  |    });
   78 |     return ret;
   79 | });

@pawelfus
Copy link
Contributor

Thanks for testing it @rdp1414

When I try to zoom till the right-extreme end, the area is selected but zoom doesn't happen. While trying to zoom within the bounds of the chart, it works as normal:

This won't work - dev tools don't fire mouseup events for the document. However it shouldn't be a problem - stakeholders don't read the financial charts with dev-tools open, right?

The same when you do the mouseup outside the browser. Events must happen within the browser.

You can fire mouseup event upon leaving the browser, demo and a snippet for you:

  document
    .documentElement
    .addEventListener(
      'mouseleave',
      (e) => document.dispatchEvent(new MouseEvent('mouseup', e))
    );

No matter where I hover mouse on the screen, even if not on the chart, Large number (more then 1000) of below errors are logged on the console:

Fixed plugin: https://jsfiddle.net/BlackLabel/1bwyj2et/2/ - should be fine now, however I would focus on solution 1 at this moment.

@rdp1414
Copy link

rdp1414 commented Apr 25, 2023

Hello @pawelfus ,

However it shouldn't be a problem - stakeholders don't read the financial charts with dev-tools open, right?

Yes, stakeholders won't look at financial charts with dev-tools open.

Plugin-1

You can fire mouseup event upon leaving the browser, demo and a snippet for you:

Even with the code in this new demo, our charts still show the same problem - See gif:
plugin1

Plugin-2

Fixed plugin: https://jsfiddle.net/BlackLabel/1bwyj2et/2/ - should be fine now

Thank you for this plugin-2. Applying this in our project, zooming, no matter how fast it is done and the pointer moves out due to momentum of the hand, zooming works correctly - See gif:
plugin2

Had both plugins worked correctly for us, then I think, you would have suggested plugin-1 and not plugin-2, is that true? But given that only plugin-2 works for us, so, we have to go with plugin-2 only - See final fiddle.

@rdp1414
Copy link

rdp1414 commented Apr 25, 2023

Now, this leaves us with the only bug / issue - #18863 which is dropping only the very last date-point.

Zooming, briskly or slowly, till the extreme ends of the chart functions correctly. Still, there is one particular case - Highstock chart in which the very last-date is dropped while trying to zoom till the right-end of the Highstock chart. This error does NOT happen for the very first-date while trying to zoom till the left-end of the Highstock chart. Also, for other cases, Highcharts numeric and Highcharts datetime, this error does NOT happen.

@pawelfus
Copy link
Contributor

Good to hear the second solution works better, thanks for the info!

The second issue seems to be a bug, very specific to the dataset - answered in #18863

@rdp1414
Copy link

rdp1414 commented May 9, 2023

Hello @pawelfus ,

  1. The fix which you gave (see fiddle) for zooming - We patched it to production. After that, we've received many high priority regressions - When there are multiple charts, then tooltip misbehaves in strange ways.
  2. In the previous point's fiddle, if we simply have 2 charts instead of 1 chart - you'll see that tooltip is messed up - see fiddle.
  3. I created a minimal reproducer - See fiddle. Note that when pointer is hovered in lower chart, the tooltip is visible in the upper chart - which shouldn't be the case.
    messedup-tooltip
  4. In the above minimal reproducer, if all the plugins which you suggested are removed, the tooltip issue is fixed - See fiddle. Below is the correct and expected behavior:
    correct-tooltip

This regression has started to show-up in our production charts, so, this bug is PRIORITY for us.

Thank you,
Rajdeep

@rdp1414
Copy link

rdp1414 commented May 9, 2023

From the previous Comment,

  1. I created a minimal reproducer - See fiddle. Note that when pointer is hovered in lower chart, the tooltip is visible in the upper chart - which shouldn't be the case.

In that fiddle, there exists 3 plugins:

  1. The first one for zoom when pointer moves out of the chart.
  2. The second one for fast hand movement while zooming.
  3. the third one is for ensuring the very last point for Highstock points are not dropped while zooming to right end of the chart.

I narrowed down that this tooltip issue is happening due to the second plugin, commenting out second plugin fixes the tooltip issue - See fiddle: Maybe this will help your debugging.

@pawelfus
Copy link
Contributor

Thank you for reporting the issue!

That means we have to resign from the second solution, and polish the first one:

(function(H) {
  H.wrap(
    H.Pointer.prototype,
    'onContainerMouseLeave',
    function(proceed, e) {
		
      this.onContainerMouseMove(e);
			
      return proceed.apply(this, Array.prototype.slice.call(arguments));
    }
  )
})(Highcharts);

Replaced the second plugin with the above and it works fine: https://jsfiddle.net/BlackLabel/zusLdb7y/ - it will still not capture events when leaving the browser (eg reaching Chrome's dev-tools). I know you said it still doesn't solve the problem: #15569 (comment) - but I'm not able to reproduce this. Could you try reproducing the issue? Thanks!

@rdp1414
Copy link

rdp1414 commented Jun 1, 2023

Could you try reproducing the issue?

A tooltip issue can be reproduced in the demo which you provided https://jsfiddle.net/BlackLabel/zusLdb7y/. In this demo, the charts are placed one-below-other. If the charts are placed one-beside-other, then tooltip issue is happening- See fiddle.

Tooltips on both the charts are visible simultaneously. When the chart is hovered on the second chart, the first chart's tooltip is still visible. See below gif.
side-by-side-charts-tooltip

@rdp1414
Copy link

rdp1414 commented Jun 1, 2023

Hello @pawelfus ,

In this update, you said

That means we have to resign from the second solution, and polish the first one:

When I used the first one in our environment, then zoom is not happening when pointer is released outside chart. See the below gif from in our environment:
chart-act-unable-to-zoom

In your update, the above issue was not reproducible. But, when the charts in your demo are placed side-by-side, and you zoom on the first chart and pointer goes out of the chart, exactly the same bug is seen which is seen in our environment - See reproducer fiddle. See gif:
fiddle-chart-unable-to-zoom-1

In the above reproducer fiddle, if the pulgin is commented out, then the above problem is resolved. Zoom happens correctly on the first chart - See plugin removed fiddle.
fiddle-chart-zoom-without-plugin

This is a production issue which we're facing since a long time. Stakeholders have complained multiple times. Please give some suggestions/solutions to address this issue.

Thank you,
Rajdeep

@pawelfus
Copy link
Contributor

pawelfus commented Jun 1, 2023

Hi @rdp1414

Thank you for the details! It was a mistake in my plugin, fixed version: https://jsfiddle.net/BlackLabel/Lz5txbe3/

Fixed part:

(function(H) {
  H.wrap(
    H.Pointer.prototype,
    'onContainerMouseLeave',
    function(proceed, e) {
      this.onContainerMouseMove(e);
      return proceed.apply(this, Array.prototype.slice.call(arguments, 1)); // missing "1" in arguments
    }
  );
})(Highcharts);

Please let us know if you find any issues with a given solution.

@rdp1414
Copy link

rdp1414 commented Jun 1, 2023

Hello @pawelfus ,

let us know if you find any issues with a given solution.

Yes, the zoom problem explained in this update is still occurring even with your given solution.

But the tooltip issue explained in this update is fixed with your solution.

@kamil-musialowski
Copy link
Contributor

kamil-musialowski commented Jun 2, 2023

Hello @rdp1414, I have created this pull request (#19083) so that we can add and run tests on the branch, and fix the issues mentioned in that topic one by one. Right now, to test new fixes you can pull the transpiled code directly from the brach, just like in this example: https://jsfiddle.net/BlackLabel/vwrb1u8y/

Here are the notes for the current state of the code:

Quick mouse drag to zoom works fine (chart is zoomed to the last point)
Tooltip seems to be working fine
Starting the zoom by drag inside the chart and ending it outside the chart container also works fine: https://jsfiddle.net/BlackLabel/fnxm17ty/

The only issue that remains is zooming when two charts are side by side: https://jsfiddle.net/BlackLabel/vwrb1u8y/

Is it the only issue that we have to fix, or do you see any other problems on code from the aforementioned branch?

@kamil-musialowski kamil-musialowski moved this from Review in progress to In progress in Development-Flow Jun 5, 2023
@rdp1414
Copy link

rdp1414 commented Jun 7, 2023

Hello @kamil-musialowski,

There are few issues with our setup of Highcharts. Let me explain our situation:

  1. We don't have a fiddle-like setup with provisions for <script> tag. In our package.json, we import "highcharts": "10.3.3" and use highcharts's constructor to create a chart new Highcharts.Chart(chartOptions). So, we cannot use <script src="https://github.highcharts.com/bugfix/15569-zoom-by-drag-improvement/stock/highstock.js"></script> to test out the fix made.
  2. Alpha:
    • The other option is that we can request you to release an alpha from that PR/branch. We pick that alpha version in our package.json file, test it, find that things are working fine which we tell you. Now, you'll merge that PR to your master branch, and you'll release an upcoming v11.1.1 which contains the fix (currently you're at v11.1.0). This is my understanding, please correct me in case of discrepancy.
    • To accept your patch release in our code, first of all, we've to make our djs-hc compatible with v11. We tried migrating to v11 and large number of tests are failing. This particular v11 of Highcharts, has large number of breaking changes for us - both UX and property for which we're seeking solutions at Topic#50953. It is highly unlikely, that we would be able to handle / accept v11 within 2 months.
  3. The other option can be, if possible, you can give as a code-snippet that we can plugin into our code-base. Is it possible to give something like this which is a plugin equivalent of the fix made in PR#19083?

Can you please give some advice and suggestions?

Regards,
Rajdeep

@TorsteinHonsi
Copy link
Collaborator

Hi Rajdeep,

I think option 3 is most practical for both parties. @kamil-musialowski what if we create snippets that contain all the changes, then give them version numbers to keep track of it until the various facets of the issue are fixed?

@kamil-musialowski
Copy link
Contributor

@TorsteinHonsi I also think that creating code snippets with the changes will be the best option. I'll prepare them :)

@TorsteinHonsi
Copy link
Collaborator

Working in parallel here... Anyway, here's the drop-in snippet for the current state of the fix, also tested on v10:

/*
Drop-in fix for #15569, drag-zoom issues.
Version: 2023-06-07
*/
(function(H) {
	const { pick, Pointer, wrap } = H;
    wrap(
        Pointer.prototype,
        'onContainerMouseLeave',
        function(proceed, e) {
            this.onContainerMouseMove(e);
            return proceed.apply(this, Array.prototype.slice.call(arguments, 1)); // missing "1" in arguments
        }
    );
    wrap(
        Pointer.prototype,
        'onContainerMouseMove',
        function(proceed, e) {
            proceed.apply(this, Array.prototype.slice.call(arguments, 1));
            this.setHoverChartIndex(e);
        }
    );
    
    Pointer.prototype.setHoverChartIndex = function (e) {
        const chart = this.chart;
        const hoverChart = H.charts[pick(Pointer.hoverChartIndex, -1)];

        if (
            hoverChart &&
            hoverChart !== chart
        ) {
            hoverChart.pointer.onContainerMouseLeave(
                e || { relatedTarget: chart.container }
            );
        }

        if (
            !hoverChart ||
            !hoverChart.mouseIsDown
        ) {
            Pointer.hoverChartIndex = chart.index;
        }
    };

})(Highcharts);

https://jsfiddle.net/highcharts/Loqywrv4/

@rdp1414 Please test and advice

@TorsteinHonsi
Copy link
Collaborator

@rdp1414

This particular v11 of Highcharts, has large number of breaking changes for us - both UX and property for which we're seeking solutions at Topic#50953

We made a legacy theme for this exact situation, see https://www.highcharts.com/samples/highcharts/members/theme-v10 . It was announced in the changelog. It is not a perfect representation of the v10 design, but close.

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

Successfully merging a pull request may close this issue.

8 participants