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

WIP fix(): searchPossibleTargets targets value #5

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

ShaMan123
Copy link
Owner

This code was not FF correctly, needs work

Motivation

Trying to impl a fix for the problem presented in fabricjs#9329
Being that the group rewrite made searchPossibleTargets return a sub target instead of the found taregt to allow deep selection. This surfaced an issue that the use of Canvas#targets aka subTargets is wrong and confusing.

closes fabricjs#9329

Description

Rethinking and Reconsidering subTargets

If I go by the name and from what I understand was the original intention I believe subTargets should populate the sub targets of the found target.
Parents of the found target should not be part of subTargets, that is what makes the name confusing. They should not be part mainly because it is clear that they are in the hit region or else the found target would not be found. If anyone including fabric needs the parents it is very simple to walk up the tree.

The main use in fabric of subTargets is for events. So we need to fix that.
I think it is unclear what is expected from the synthetic events.

Changes

  • _searchPossibleTargets => findTargetsTraversal + added a flag to allow searching for all hits
  • fix searchPossibleTargets return value to consider an case where subTargetCheck: true, interactive: false group were travesred, so returning the first hit is wrong
  • dep searchPossibleTargets => findTargets
  • move setting targets to findTarget because it varies on the case and is a step towards refactoring the use of Canvas#targets

Gist

The tests came from fabricjs#9333 if you wish to merge that first, conflicts should resolve to ours

In Action

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.

1 participant