Skip to content

Feature: More customizable pointer events for advanced use cases #498

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

Open
wants to merge 1 commit into
base: v6
Choose a base branch
from

Conversation

hmpthz
Copy link

@hmpthz hmpthz commented Oct 2, 2024

It's always been hard if there are interactive childs inside Viewport containers.
Devs have to be careful with all those event bubbling and capturing stuff to make things work as they expect.

Recently, I was adding a few childs with 'pointertap' events into Viewport, hoping the child events would be emitted before Viewport events. Then I found out 'pointertap' event was not added by InputManager, therefore event bubbling didn't work.

This makes me think if it's a good idea to provide additional options for devs to fully control Viewport's pointer events. I believe it might be useful in some rare use cases.

This commit adds a new property to IViewportOption. People won't notice any difference if this prop is not specified.

@danielbarion danielbarion changed the base branch from master to v6 November 13, 2024 18:08
@danielbarion
Copy link
Collaborator

danielbarion commented Nov 13, 2024

@hmpthz we'll be taking a look as soon as possible, thanks!

@danielbarion
Copy link
Collaborator

@hmpthz can you please check the conflicts?

@danielbarion
Copy link
Collaborator

@hmpthz @davidfig I believe we should release v6 and after it, check this PR and release a v6.1.0 after confirming everything is working as expected :)

@hmpthz
Copy link
Author

hmpthz commented Nov 14, 2024

@danielbarion Thanks, I'm looking into it. It seems the conflicts are just about imports.

@hmpthz
Copy link
Author

hmpthz commented Nov 14, 2024

@hmpthz @davidfig I believe we should release v6 and after it, check this PR and release a v6.1.0 after confirming everything is working as expected :)

Conflict resolved, please check :)

@danielbarion
Copy link
Collaborator

the code looks good to me, but I would like to have feedback from @lunarraid too.

@lunarraid can you please take a look and let me know your thoughts about it? thanks in advance

@lunarraid
Copy link
Contributor

This seems to add a lot of complexity just to change out whether or not the pointer events use the capture phase or not. Is all that per-event logic really necessary? There's nothing preventing devs from simply adding their own capture phase events for any reason they'd need to without any API changes.

@hmpthz
Copy link
Author

hmpthz commented Dec 2, 2024

Hi @lunarraid the main reason for this pr is because you can't stop the original InputManager from adding all default listeners in constructor.
Therefore, if you want to change to a different event, you must remove the current one first. I just feel like this "remove then add again" can be avoided by a new config option.
But you have the point that this would be an overkill as devs are free to add events they want anyway.

An alternative solution could be a simple boolean option such as "noDefaultListeners" so that InputManager won't add anything in contructor. Then it'll be devs jobs to add things on their demand. Do you think this sounds ok?

if (!this.viewport.options.noDefaultListeners)
{
    this.addPointerListeners();
}

Frankly speaking, writing some "remove then add again" code doesn't hurt anything. It's just my personal motive to get rid of it at the cost of a small API change.

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

Successfully merging this pull request may close these issues.

3 participants