bugfix/13353-handling-passive-events

[raf18seb]

Fixed #11353, added support for passive events.

This PR is a continuation of #13933 (can be closed). I couldn't resolve some issues again so had to install a fresh repo.

[highsoft-bot]

Visual test results - No difference found

[highsoft-bot]

File size comparison

	master	candidate	difference
highcharts	268.2 kB	268.5 kB	344 B
highcharts, gzipped	92.4 kB	92.4 kB	-25 B
highstock	347.4 kB	347.7 kB	344 B
highstock, gzipped	118.8 kB	118.9 kB	153 B
highmaps	304.6 kB	304.9 kB	344 B
highmaps, gzipped	104.1 kB	104.3 kB	211 B
highcharts-gantt	363.8 kB	364.2 kB	344 B
highcharts-gantt, gzipped	123.8 kB	123.9 kB	59 B

[TorsteinHonsi]

Thanks!

As I noted in the original PR, I was concerned about the added complexity when allowing an event handler to be a config object instead of a function:

If the current suggestion works for our internal cases, I'd say we start with that. But obviously we need to check if we break something, by identifying if there are places we are currently calling e.preventDefault on touch events.

Did we check this out? Have we concluded that we actually need the added complexity?

[raf18seb]

I tested all the cases that I could think of where we are calling e.preventDefault and made those events to be passive: false.

Did we check this out? Have we concluded that we actually need the added complexity?

We don't need it internally. Internally, we can decide whether an event should be passive or not before adding an event (as I did).
The main reason for adding that complexity is to let the users decide about their events before they're added.

We can get rid of it and wait for users to request, however, it's already implemented - it's your call whether you want to keep it or add it later/never.

[TorsteinHonsi]

In my experience it's better to be minimalistic and tight on the features than to add features and complexity that is rarely used. @pawelfus , @oysteinmoseng what do you think?

[pawelfus]

Given the popularity of the issue, I would say fix it all

[oysteinmoseng]

How much overhead is it to support the event config objects? If it is a lot, I suppose the alternative is to make user-provided touch events passive by default. That should cover most cases without having to add complexity.

[TorsteinHonsi]

How much overhead is it to support the event config objects?

@oysteinmoseng You can read that from the code in this PR, as it is implemented here.

[TorsteinHonsi]

If we decide to go all in, we should refactor out the part where we objectEach over event options and add either a function event or an event object.

It is currently implemented in Chart.ts and Interaction.ts.

But should also be applied in Axis.ts and possibly PlotLineOrBand.ts

[oysteinmoseng]

If I am reading it correctly that's just 8 or so lines of extra code so far? That being said, there is the refactor mentioned by Torstein, and it looks like it is not added to the API documentation yet, and it is extra complexity to explain to users.

Unless I am missing something, the only value we are adding here is that users can add touch events that call preventDefault from the config options. Since we have the pattern of returning false to prevent the default Highcharts action, I am not sure how much value there is in the extra preventDefault. Except to actually prevent scrolling, I suppose. Users could also perhaps use addEvent directly in many cases if they want to control the passive option.

I am leaning towards not implementing this for now.

[TorsteinHonsi]

I am leaning towards not implementing this for now.

Me too... So far, "it may come in handy sometime in the future" has not been a valid reason to add something to Highcharts.

Given the popularity of the issue, I would say fix it all

@pawelfus Don't you think that's mainly because of the error message in the console, that will be fixed under any circumstance?

[pawelfus]

Don't you think that's mainly because of the error message in the console, that will be fixed under any circumstance?

Hmm.. good point. I just realized, after Oystein's comment, that only touch events need this change (and a few of click events too, because those are translated to touch events sometimes). And those are not that popular afaik. I haven't seen anyone reporting this issue for anything except our internal events.

If I am reading it correctly that's just 8 or so lines of extra code so far? That being said, there is the refactor mentioned by Torstein, and it looks like it is not added to the API documentation yet, and it is extra complexity to explain to users.

Actually, instead of refactoring, we could change addEvent(element, eventName, callback, options) to allow callback or config: addEvent(element, eventName, { callback, options } | callback, options). That would simplify code in the core too. But readability.. Just thinking out loud, nevermind me ;)

[TorsteinHonsi]

Okay, we need a decision...

I hereby decide that we implement passive events for our internal events, but not for custom events added through chart config. That way we solve the initial reported issue, and we don't overcomplicate our API options structure.

In the future, if frequently requested, we can consider support for passive events through the API options structure.

[TorsteinHonsi]

Thanks!