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

Fix x-on with both self and once #4152

Merged
merged 2 commits into from
Apr 18, 2024
Merged

Fix x-on with both self and once #4152

merged 2 commits into from
Apr 18, 2024

Conversation

bb
Copy link
Contributor

@bb bb commented Apr 17, 2024

I want to use the once and self modifiers together, like @something.self.once, but the handler never runs if the first event is not from self. I think the reason is that once is evaluated before self.

This PR fixes the order of evaluating the modifiers so the two work together.

I think the failing test is unrelated.

Copy link
Contributor

@ekwoka ekwoka left a comment

Choose a reason for hiding this comment

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

the once modifier should be moved to be the final modifier handled to also also handle keydown and keyup once usages.

@bb
Copy link
Contributor Author

bb commented Apr 18, 2024

I'm not sure if I understand your request correctly. Keydown and keydown are already lexically last, i.e. evaluated first.

But maybe we want to move once up a bit, so e.g. clicking outside (once) will actually work after a click inside.

@bb
Copy link
Contributor Author

bb commented Apr 18, 2024

I added a test for .away.once which was failing and fixed it by moving once lexically higher, i.e. evaluated later than the inside/outside check.

Tests for keyup.once already existed and passed before this PR.

@Tim-Wils
Copy link

Shouldn't 'once' be part of the options passed to the addEventListener though?
That way the eventListener destroy itself after and the removeEventListener part could be skipped/removed.

Once option docs:
https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/addEventListener#once

@bb
Copy link
Contributor Author

bb commented Apr 18, 2024

That wouldn't work with the filtering modifiers, @Tim-Wils. Let's take keydown as an example: with plain event listeners, you can only listen for keydown, but not for a specific key. Try this:

window.addEventListener('keydown', function(event) {
  if (event.key === 'b') {
    console.log('pressed b once');
  }
}, { once: true });

Press first "a". Then press "b". Nothing will be logged on the console, because after pressing a, the listener would be gone.

@Tim-Wils
Copy link

@bb Seems fair, you're right :) Thanks for the example!

@ekwoka
Copy link
Contributor

ekwoka commented Apr 18, 2024

Yes last or first. Once should do it's thing only once all the others have passed, no?

If once is after self, it should also be after the keyup modifiers. Since they should both only do the once if they pass.

@bb
Copy link
Contributor Author

bb commented Apr 18, 2024

Exactly. That's how I did it from the beginning and how it still is.

Order of processing in as of the current state of this PR:

  1. keys
  2. self
  3. away/outside
  4. once

So, what's your request?

@calebporzio
Copy link
Collaborator

Looks great and makes perfect sense. Thanks @bb!

@calebporzio calebporzio merged commit 95ae590 into alpinejs:main Apr 18, 2024
1 check passed
@bb
Copy link
Contributor Author

bb commented Apr 18, 2024

Thanks for merging and for AlpineJS in general 😊

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.

4 participants