test(): add event pointer caching failing test

[ShaMan123]

Description

Caching pointers the way fabric does is bad design.
A very simple and common case will hit a severe bug.

Reproduce

From an event handler you need to change the viewport transform and then try to use Canvas#getScenePoint. That will return the cached value which is wrong.
I added a simple test showing that.

In fact you would need to call Canvas#_resetTransformEventData to fix getScenePoint which doesn't make sense.
If the perf opt is really that significant (and that is not clear) then we can assign the values to the pointer event so that e.scenePoint could be accessed instead of calling getScenePoint.
This is also non breaking for rc. But I would say that these method are new anyways so no matter what the dev needs to visit them so we can say - if you wish to save on some computation replace canvas.getPointer with e.viewportPoint/e.scenePoint

Regardless, caching Canvas#_target has proved to be very limiting and hard to override by apps to a point that I am departing for all the event handlers and re implementing myself from scratch.

In Action

[codesandbox]

Review or Edit in CodeSandbox

Open the branch in Web Editor • VS Code • Insiders

Open Preview

• Run Next Template
• Run Node Template
• Run Vanilla Template
• Run Vue Template

[github-actions]

Build Stats

file / KB (diff)	bundled	minified
fabric	916.740 (0)	305.648 (0)

[ShaMan123]

uncomment this line and the test passes

[asturur]

the reason why those values were cached is because traversing the dom was expensive.
In a mouse move situation was painful in the moment we started to fire more events or check more often the pointer
We could just reset the cache before firing the event probably and fire the event as late as it makes sense, or we could just stop caching the one affected by the viewport port so that is just a matrix multiplication and is not expensive.

Meaning that the actual point you click on the canvasElement the non scene one is not going to change, and that is the expensive one to calculate.

Is non breaking anyway is just performances

[asturur]

I ll say one more time, leaving comments in description of PRs like this one:

Regardless, caching Canvas#_target has proved to be very limiting and hard to override by apps to a point that I am departing for all the event handlers and re implementing myself from scratch.

is not useful.
Don't do that.
If something is limiting open a discussion, explain it, make a proposal, explain alternatives you have considered, weight pros and cons of caching vs not caching, measure the impact. If you don't have time for that don't do it, but you don't need to leave the comment either.
If you leave a comment like that in a PR for a test for events, you think you have communicated an issue while you didn't, because that is not the place for those informations.

[asturur]

This PR contains only a failing test, so i don't think we want to merge it. What is it for?

[ShaMan123]

To prove that we need to address it and to discuss.
I have worked on removing event caching: https://github.com/fabricjs/fabric.js/compare/fix/rm-event-caching?expand=1

[ShaMan123]

I have worked on removing event caching: https://github.com/fabricjs/fabric.js/compare/fix/rm-event-caching?expand=1

I do not think you will want to do that so I am forking

[asturur]

My message is super clear, is there you can re-read it as many time as you want.

I wrote you a bunch of messages with reminders of things said/planned but i don't think this is the place for it and then i felt like i was defending myself, thing i don't really need to do.

You need a fork for how you want to work, but don't put it on me because is just not correct.

I invited you a couple of months ago to fork because i noticed that they way you say you want to do things does not match the way you need to do things and you actually do things.
I think is the right decision for you.

[ShaMan123]

Let me recap the professional side of our discussion.

Why does fabric use event caching?

the reason why those values were cached is because traversing the dom was expensive.
In a mouse move situation was painful in the moment we started to fire more events or check more often the pointer

Originally posted by @asturur in #9850 (comment)

Why do I think fabric uses event caching?

Fabric creates event context repeatedly when it fires events and uses that context in many cases.
However, the main canvas methods do not use it and leaf consumers at the very edge do not, e.g.

fabric.js/src/shapes/IText/ITextClickBehavior.ts

Lines 246 to 247 in a055616

	getSelectionStartFromPointer(e: TPointerEvent): number {
	const mouseOffset = this.canvas!.getScenePoint(e)

It seems that it is not used for historical reasons. The code was written and wasn't refactored to limit changes.
I would argue that caching was done ONLY because the context didn't reach all consumers. So instead of breaking signatures of those consumers in order to provide better context, caching was introduced.

My Approach: Fixing without Degrading Performance

My Motivation

I prefer refactoring than workarounds.

I hate gotchas and even more fabric gotchas.
They are time sinks.
I have been working with fabric for a number of years as a contributor and before that as a consumer.
The fact that I still bump into gotchas makes me 😡.
It states to me that fabric is immature for PROD.

I want to fix this issue without degrading performance.
This means refactoring the code.

A Different Design: context vs. caching

Providing context is the better way because:
It is stateless and declarative.
It is easy to test, maintain and reason about.
It is easy to override, subclass and customize. e.g. a dev could control the context and pass whatever they want as long as it follows the context convention and control fabric with ease.

Fabric creates event context repeatedly when it fires events and uses that context in many cases.
Fabric should use that context and declare it at the beginning of each event handler and pass it down to where it didn't reach until now.
This means that instead of recreating the context in _handleEvent we pass it down from the caller (the top level event hander) and use in the caller as well.

I have worked on removing event caching: https://github.com/fabricjs/fabric.js/compare/fix/rm-event-caching?expand=1

Originally posted by @ShaMan123 in #9850 (comment)

Other Approach: Fix the Mentioned Bug

Motivation

We are at rc, maybe there is no option for refactors. Most likely no option for breaking changes.

Suggestions

Remove caching

We could just reset the cache before firing the event probably and fire the event as late as it makes sense, or we could just stop caching the one affected by the viewport port so that is just a matrix multiplication and is not expensive.
Meaning that the actual point you click on the canvasElement the non scene one is not going to change, and that is the expensive one to calculate.

Originally posted by @asturur in #9850 (comment)

Though this sounds legit as a fix for the mentioned bug it doesn't fix other bugs caused by the same issue.

Consider a RTL css layout app with the following case:
During an event, when caching is active, an event handler triggers a canvas resize.
Since pointers are measured from the left, the viewport point will also become invalid, making the proposal to keep part of pointer caching as is invalid.

Is non breaking anyway is just performances

Originally posted by @asturur in #9850 (comment)

I do not want to degrade performance so removing caching entirely sounds bad if indeed the dom calculations are heavy.
However, I do prefer something correct than something that is fast and is correct most of the time. That is the exact cause of a gotcha.
Regardless, PROD accepts only something that is correct and fast.
So if fabric is meant for PROD removing caching entirely is not legit.

Assign the cached values to the DOM event itself

If the perf opt is really that significant (and that is not clear) then we can assign the values to the pointer event so that e.scenePoint could be accessed instead of calling getScenePoint.

Originally posted by @ShaMan123 in #9850 (comment)

This is valid and does not break anything apart for a ton of tests.
This solution also follows event conventions, meaning, that the data is correct at the moment of the event being fired. If something changed during the time that has passed from firing the event, the dev understands that they can't trust the values any longer and resort to calling getScenePoint or getViewportPoint

Posted in: #9853

[ShaMan123]

This PR contains only a failing test, so i don't think we want to merge it. What is it for?

A test in jest can be marked as test.failing so it can be merged as a TODO