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

Skip extra listener for non-Flash platforms #3023

Closed
wants to merge 0 commits into from

Conversation

DigiEggz
Copy link
Contributor

@DigiEggz DigiEggz commented Feb 3, 2024

Adding an extra listener can cause callbacks to fire twice in rapid succession, causing errors. The Flash version may still require this, so the listener is retained for that platform.

@DigiEggz DigiEggz closed this Feb 3, 2024
@DigiEggz DigiEggz reopened this Feb 3, 2024
@DigiEggz DigiEggz closed this Feb 4, 2024
@DigiEggz DigiEggz reopened this Feb 5, 2024
@DigiEggz
Copy link
Contributor Author

DigiEggz commented Feb 5, 2024

Checking input.pressed instead of overlapFound allows the button to fire "onUp" events when triggered by elements other than the pointer.

@Geokureli
Copy link
Member

can you provide small, complete test case that highlights the benefit of this change?

@DigiEggz
Copy link
Contributor Author

DigiEggz commented Feb 5, 2024

can you provide small, complete test case that highlights the benefit of this change?

Sure thing!

@DigiEggz
Copy link
Contributor Author

DigiEggz commented Feb 6, 2024

I made another change to make sure it functions correctly when the allowSwiping property is set.

Demo.zip

Here's a simple project that includes a FlxButton and a FlxUIButton. Clicking on any button should print text to the console. Pressing the X key or Enter will trigger the UI button. Pressing the S key will disable swiping on both buttons.

When you swap between the current the new code, everything should properly react. The primary benefit is that an extra event listener is no longer added to every button instance. I've noted some occasions where the event listener can cause the "onUp" event to be fired twice, which can cause issues. Please let me know if you encounter any differences or have any concerns.

Because of the order of handling, this may resolve #1370.

@Geokureli
Copy link
Member

Geokureli commented Feb 9, 2024

I haven't checked the demo yet, but one fear i have pertains to this:

/**
* Using an event listener is necessary for security reasons on flash -
* certain things like opening a new window are only allowed when they are user-initiated.
*/

I've also experienced similar restrictions on browsers

This also seems like its 2 or 3 changes rolled up into one and I'm not sure what each change is trying to fix

Edit: I looked at the demo, I do not see any issues that need fixing

@DigiEggz
Copy link
Contributor Author

DigiEggz commented Feb 9, 2024

I left the onUp check for Flash since it likely still requires it, so Flash should function the same. The extra code added to updateStatus() was to fix a dragging issue introduced by the listener-less code.

The end goal is for buttons to function as they currently do, but without an extra event listener, freeing up memory and preventing execution order errors.

@Geokureli
Copy link
Member

I left the onUp check for Flash since it likely still requires it, so Flash should function the same

Again, I've noticed the same requirements on browsers meaning html5 should not have this removed

@DigiEggz
Copy link
Contributor Author

I left the onUp check for Flash since it likely still requires it, so Flash should function the same

Again, I've noticed the same requirements on browsers meaning html5 should not have this removed

Oh, I see what you're saying! I thought you meant browsers as in Flash within a browser. I can continue to test HTML5 functionality between different browsers.

What would be a good test case for this? When it says open a new window, does it mean something like FlxG.openURL()?

@Geokureli
Copy link
Member

What would be a good test case for this? When it says open a new window, does it mean something like FlxG.openURL()?

Yeah i've specifically seen this error when opening a url, I'd also like to know more about this issue about events firing twice before I make a change like this for all projects. perhaps this should be an opt-in setting?

@DigiEggz
Copy link
Contributor Author

DigiEggz commented Feb 10, 2024

I understand the cause for concern! So far I've had no issues on Firefox or Chrome when using openURL(). It's been hard to replicate, but there are times when a button callback changes a graphic and opens a substate, the graphic is null for a second and an OpenFL null graphic crash occurs.

I noticed that not adding the additional onUp callback listener stopped this from happening. Other than for Flash, it made sense to not add an extra callback if it was no longer necessary. Let me know if you have any issues with HTML5.

What would be the best way to make this opt-in?

Edit: I need to look into #2201 because it may be tangentially related.

@Geokureli
Copy link
Member

Geokureli commented Feb 10, 2024

TBH the open url issue is something that has come and gone once or twice with various security updates for browsers, at different times for different browsers.

Chrome has a domain whitelist and blacklist for this setting, I believe a window opening without being called from a user input event is considered a "pop-up"
Screenshot 2024-02-10 at 12 06 19 PM

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