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

ISSUE-4115 - triggers in/out events for sub targets #6013

Merged

Conversation

jakedowns
Copy link
Contributor

  • and calls _setCursorFromEvent for sub targets

…ls _setCursorFromEvent for sub targets

pass linting
@jakedowns jakedowns force-pushed the ISSUE-4115-mouse-over-out-sub-targets branch from ec0d03a to 54c5923 Compare December 3, 2019 17:56
@jakedowns
Copy link
Contributor Author

Shout-out to #4115 Here's a little preview of this PR in action to get you excited! :D

Screen Recording 2019-12-03 at 10 02 AM

@jakedowns
Copy link
Contributor Author

@asturur I am noticing something weird with this implementation, when I scroll (via canvas.viewportTransform[5]) the hover Over seems to work ok, but the hover Out isn't

I'll try to put together a reduced test case scenario tomorrow.

https://share.getcloudapp.com/o0uQylBp

@asturur
Copy link
Member

asturur commented Dec 6, 2019

i need some time to read it carefully.

@asturur
Copy link
Member

asturur commented Dec 6, 2019

i need some time to read it carefully.
You wrote lot of code, 0 comments. :)
takes time to review.

Try to explain what is the approach you took, there are difficulties in handling hover and out, on a stack of objects.

I'm happy to help you trough this and merge it, as long as you are ok accepting that is a mid long process of understanding and refining.

@jakedowns
Copy link
Contributor Author

Oh yes, I totally understand. Thanks for being receptive & responsive to this PR.

My initial approach followed your original suggestion to the letter, I simply added a targetName argument, and used the key of the .targets[] array for naming, and tracking ._hoveredTargetN

however, this didn't seem sufficient, as, I'd roll across from one subtarget to the next, and sometimes, depending on the nesting, ._hoveredTarget0 would match (signaling an "oldTarget") but, really, there was a new ._hoveredTarget0 trying to be set. So it would sort of do an outFire & overFire, leading to it sometimes working, sometimes not, as it would get false-positives for "targetChanged"

My second attempt was to switch from a list of targets, keyed by their target array index to a dictionary keyed by a guid.

  1. i added a pseudo-private .__guid getter to the base Object class (similar to the .__uid property already present)
  2. canvas.hoveredTarget|N becomes => canvas._hoveredTargets = {}
    • i also added canvas._hoveredTargetsOrdered = [string_guid1, string_guid2] in case a developer needed to reference the exact stacking order of the hovered objects, but maybe that was overkill.
  3. I updated fireSyntheticInOutEvents to accept an array of Objects as it's first targets argument
  4. Since _fireEnterLeaveEvents also uses fireSyntheticInOutEvents, i modified it to match, using targets instead of target and adding a corresponding canvas._draggedoverTargets<dictionary>, canvas._draggedoverTargetsOrdered<array>
  5. I did my best to null out / delete lingering Object references in these two new places to avoid memory leaks as you suggested
  6. I updated all the related tests I could find, but I have not yet added any new tests for checking that triggering in/out events for sub targets works as expected😳

@asturur
Copy link
Member

asturur commented Dec 8, 2019

So i have some requests:
I would leave the _hoveredTargetsOrdered out for this PR.
Let's implement the basic functionality first and test it.

I would also like to avoid to introduce the getter for the _guid.
I do not like the concept that _guid is related to graphical interfaces ( maybe i m wrong? ). I would like to understand if we can relate to the group structure to find the objects rather than adding an identifier for this.

@asturur
Copy link
Member

asturur commented Dec 8, 2019

I would image somehow the group structure is enough.
The targets we find and we put in the targets array should change only when some target change.
At that point every item in the target array from the change on is no more hovered, and all the one before the changed one are still hovered.

I would like to see this boiled down to the minimum amount of changes we need to support the basic feature.

@jakedowns
Copy link
Contributor Author

Totally understand. Ok, I'll revert back to my first pass at implementing this and squash it into this PR

…rgets and draggedoverTargets tracking"

This reverts commit b6fa464.

# Conflicts:
#	src/mixins/canvas_events.mixin.js
# Conflicts:
#	src/mixins/canvas_events.mixin.js
src/canvas.class.js Outdated Show resolved Hide resolved
@asturur
Copy link
Member

asturur commented Dec 14, 2019

Today i ll sit down and help with this.

@asturur
Copy link
Member

asturur commented Dec 14, 2019

i m actually on a local setup validating an idea. I will share asap

@jakedowns
Copy link
Contributor Author

jakedowns commented Dec 14, 2019 via email

@asturur
Copy link
Member

asturur commented Dec 14, 2019

{"version":"3.5.1","objects":[{"type":"activeSelection","version":"3.5.1","left":-152,"top":656.25,"width":356.5,"height":356.5,"scaleX":0.45,"scaleY":0.45,"objects":[]},{"type":"group","version":"3.5.1","left":11,"top":6,"width":511.5,"height":511.5,"objects":[{"type":"rect","version":"3.5.1","left":-255.75,"top":-255.75,"width":50,"height":50,"fill":"#6ce798","scaleX":10.03,"scaleY":10.03,"opacity":0.8},{"type":"group","version":"3.5.1","left":-179.75,"top":22,"width":356.5,"height":356.5,"scaleX":0.54,"scaleY":0.54,"objects":[{"type":"rect","version":"3.5.1","left":-178.25,"top":-178.25,"width":50,"height":50,"fill":"#4862cc","scaleX":6.99,"scaleY":6.99,"opacity":0.8},{"type":"group","version":"3.5.1","left":-163.25,"top":-161.25,"width":177.5,"height":177.5,"objects":[{"type":"rect","version":"3.5.1","left":-88.75,"top":-88.75,"width":50,"height":50,"fill":"#5fe909","scaleX":3.48,"scaleY":3.48,"opacity":0.8},{"type":"rect","version":"3.5.1","left":-59.75,"top":-68.75,"width":50,"height":50,"fill":"#f3529c","opacity":0.8},{"type":"triangle","version":"3.5.1","left":36.03,"top":-38.12,"width":50,"height":50,"fill":"#c1124e","angle":39.07,"opacity":0.8},{"type":"rect","version":"3.5.1","left":-65.75,"top":17.25,"width":50,"height":50,"fill":"#9c5120","opacity":0.8}]},{"type":"group","version":"3.5.1","left":-34.25,"top":-31.25,"width":177.5,"height":177.5,"scaleX":1.08,"scaleY":1.08,"objects":[{"type":"rect","version":"3.5.1","left":-88.75,"top":-88.75,"width":50,"height":50,"fill":"#5fe909","scaleX":3.48,"scaleY":3.48,"opacity":0.8},{"type":"rect","version":"3.5.1","left":-59.75,"top":-68.75,"width":50,"height":50,"fill":"#f3529c","opacity":0.8},{"type":"triangle","version":"3.5.1","left":36.03,"top":-38.12,"width":50,"height":50,"fill":"#c1124e","angle":39.07,"opacity":0.8},{"type":"rect","version":"3.5.1","left":-65.75,"top":17.25,"width":50,"height":50,"fill":"#9c5120","opacity":0.8}]}]},{"type":"group","version":"3.5.1","left":-202.75,"top":-228.5,"width":356.5,"height":356.5,"scaleX":0.61,"scaleY":0.61,"objects":[{"type":"rect","version":"3.5.1","left":-178.25,"top":-178.25,"width":50,"height":50,"fill":"#4862cc","scaleX":6.99,"scaleY":6.99,"opacity":0.8},{"type":"group","version":"3.5.1","left":-163.25,"top":-161.25,"width":177.5,"height":177.5,"objects":[{"type":"rect","version":"3.5.1","left":-88.75,"top":-88.75,"width":50,"height":50,"fill":"#5fe909","scaleX":3.48,"scaleY":3.48,"opacity":0.8},{"type":"rect","version":"3.5.1","left":-59.75,"top":-68.75,"width":50,"height":50,"fill":"#f3529c","opacity":0.8},{"type":"triangle","version":"3.5.1","left":36.03,"top":-38.12,"width":50,"height":50,"fill":"#c1124e","angle":39.07,"opacity":0.8},{"type":"rect","version":"3.5.1","left":-65.75,"top":17.25,"width":50,"height":50,"fill":"#9c5120","opacity":0.8}]},{"type":"group","version":"3.5.1","left":-34.25,"top":-31.25,"width":177.5,"height":177.5,"scaleX":1.08,"scaleY":1.08,"objects":[{"type":"rect","version":"3.5.1","left":-88.75,"top":-88.75,"width":50,"height":50,"fill":"#5fe909","scaleX":3.48,"scaleY":3.48,"opacity":0.8},{"type":"rect","version":"3.5.1","left":-59.75,"top":-68.75,"width":50,"height":50,"fill":"#f3529c","opacity":0.8},{"type":"triangle","version":"3.5.1","left":36.03,"top":-38.12,"width":50,"height":50,"fill":"#c1124e","angle":39.07,"opacity":0.8},{"type":"rect","version":"3.5.1","left":-65.75,"top":17.25,"width":50,"height":50,"fill":"#9c5120","opacity":0.8}]}]},{"type":"group","version":"3.5.1","left":138.3,"top":-90.22,"width":356.5,"height":356.5,"scaleX":0.42,"scaleY":0.42,"angle":62.73,"objects":[{"type":"rect","version":"3.5.1","left":-178.25,"top":-178.25,"width":50,"height":50,"fill":"#4862cc","scaleX":6.99,"scaleY":6.99,"opacity":0.8},{"type":"group","version":"3.5.1","left":-163.25,"top":-161.25,"width":177.5,"height":177.5,"objects":[{"type":"rect","version":"3.5.1","left":-88.75,"top":-88.75,"width":50,"height":50,"fill":"#5fe909","scaleX":3.48,"scaleY":3.48,"opacity":0.8},{"type":"rect","version":"3.5.1","left":-59.75,"top":-68.75,"width":50,"height":50,"fill":"#f3529c","opacity":0.8},{"type":"triangle","version":"3.5.1","left":36.03,"top":-38.12,"width":50,"height":50,"fill":"#c1124e","angle":39.07,"opacity":0.8},{"type":"rect","version":"3.5.1","left":-65.75,"top":17.25,"width":50,"height":50,"fill":"#9c5120","opacity":0.8}]},{"type":"group","version":"3.5.1","left":-34.25,"top":-31.25,"width":177.5,"height":177.5,"scaleX":1.08,"scaleY":1.08,"objects":[{"type":"rect","version":"3.5.1","left":-88.75,"top":-88.75,"width":50,"height":50,"fill":"#5fe909","scaleX":3.48,"scaleY":3.48,"opacity":0.8},{"type":"rect","version":"3.5.1","left":-59.75,"top":-68.75,"width":50,"height":50,"fill":"#f3529c","opacity":0.8},{"type":"triangle","version":"3.5.1","left":36.03,"top":-38.12,"width":50,"height":50,"fill":"#c1124e","angle":39.07,"opacity":0.8},{"type":"rect","version":"3.5.1","left":-65.75,"top":17.25,"width":50,"height":50,"fill":"#9c5120","opacity":0.8}]}]}]}]}

Storing here a json loadable nested group that is usefull for tests

@asturur
Copy link
Member

asturur commented Dec 14, 2019

@jakedowns i made #6032 a POC i did not even have time to run. I'm going to bed is midnight.
Please have a look at the idea, and think if it can work. I'm sure we can do something on those lines.

+ added a _this var for targets.forEach loop closure
+ changed targetName to oldTarget in _fireEnterLeaveEvents config object
+ added additional loop to call `fireSyntheticInOutEvents` when `this._hoveredTargets.length` is larger than `this.targets.length`
@jakedowns jakedowns changed the title Fixes #4115 - triggers in/out events for sub targets ISSUE-4115 - triggers in/out events for sub targets Dec 15, 2019
…out-sub-targets

# Conflicts:
#	src/mixins/canvas_events.mixin.js
@jakedowns
Copy link
Contributor Author

jakedowns commented Dec 15, 2019

Thank you for your assistance with this. It's always great to have a second set of eyes looking at a problem to come up with the most elegant & straight-forward solution. Also great to have someone to bounce ideas back & forth with.

I've implemented the changes from your POC and your comments from your POC PR.
Thank you for allowing me to take ownership/credit for this PR as well! 🙇

Next step, I believe, is just a matter of making sure the unit tests account for these changes, correct? Any suggestions as to what you'd like to see in terms of test coverage?

I've tested on my local using the following setup and it appears to be working as expected:

https://gist.github.com/jakedowns/0fbca45fd8796e109e94635c2f221980

(^ you can ignore the zoom/pan bindings in there, I was trying to recreate the issue I have in my project repo where the hovering becomes mis-aligned after panning, but i believe it is a bug i'm somehow introducing somewhere else in my code, because sub-target hovering appears to work fine in this example, even when zoomed and/or panned. I'll have to narrow down that other bug some more before I can reproduce it in a simple test case, and so that I can ask for more assistance with it.)

+ reference new ._hoveredTargets array instead of Canvas._hoveredTargetN properties
src/canvas.class.js Outdated Show resolved Hide resolved
src/shapes/object.class.js Outdated Show resolved Hide resolved
@asturur
Copy link
Member

asturur commented Dec 15, 2019

I think we are nearly there, i would happy with even just a new test case using that json string and mocking up some event data. i can try to write one.

Thanks for being patient and collaborative.

@jakedowns
Copy link
Contributor Author

jakedowns commented Dec 15, 2019

@asturur Thanks again for all your help and attention with this PR. I stubbed in a new unit test:

7483142

Not sure of your preferred method of simulating the mouseMove events though. couldn't find an existing example of that.

I also adjusted _setCursorFromEvent to account for subTargets. I hope that's ok, my project relies on it, so i'm hoping it can be included when this finally releases. 1909d1f

test/unit/canvas_events.js Outdated Show resolved Hide resolved
+ _fireOverOutEvents: fill diff array with nulls
+ _onMouseOut: remove debug console.log
+ updated description for subTargetCheck property to expand what it affects
+ updated subTargetCheck mouseover/mouseout event test case
+ added TODO notes for new tests for "mousemove: subTargetCheck: setCursorFromEvent considers subTargets"
@asturur
Copy link
Member

asturur commented Dec 18, 2019

I would proceed with a merge and then eventually fix if we broke anything. I can add more tests later.

@jakedowns
Copy link
Contributor Author

let me know if there's anything else you need from me before proceeding with the merge :D

@asturur
Copy link
Member

asturur commented Dec 18, 2019

No, i want just to add more testing, but i realize we have not the infrastructure. Is easier for me to look at master than working on your branch, the build pass i think is fine.

var _this = this, _hoveredTarget = this._hoveredTarget,
_hoveredTargets = this._hoveredTargets, targets = this.targets,
diff = _hoveredTargets.length - targets.length;
[target].concat(targets, new Array(diff > 0 ? diff : 0).fill(null)).forEach(function(_target, index) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jakedowns sorry i did not notice this.
We still support internet explorer and fill is not an option there.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

replaced with a for loop: dbaf781

test/unit/canvas_events.js Outdated Show resolved Hide resolved
@asturur
Copy link
Member

asturur commented Dec 18, 2019 via email

diffArray = [];
for (var i = 0; i < diffArrayLength; i++){
diffArray.push(null);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh i see, the array with 3 undefined will not iterate. Well ok this is fine, if will find away to have it more terse, it can be a follow up.

image

the length is 7 but forEach will not care

Copy link
Contributor Author

@jakedowns jakedowns Dec 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah forEach ignores the empties :/ interested to see what you come up with that's shorter than that. I looked at an Array.prototype.fill polyfill, but it was way more lines than a 3-line for loop.

@asturur
Copy link
Member

asturur commented Dec 18, 2019

to be honest we have a fabric.util.array.fill, but since i'm planning to remove those generic utils from the library ( non standard and out of scope ) i did not want to use it.
Probably there isn't a readable shorter version. Whatever i can think of makes the code unreadable for a couple of bytes of savings. not worth.

@asturur asturur merged commit f876769 into fabricjs:master Dec 18, 2019
@asturur asturur mentioned this pull request Dec 28, 2019
ickata pushed a commit to ickata/fabric.js that referenced this pull request Jul 27, 2021
* Fixes fabricjs#4115 - triggers in/out events for sub targets; and calls _setCursorFromEvent for sub targets
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants