Skip to content

Don't preventDefault on events that are filtered#1572

Closed
HeyHugo wants to merge 2 commits intobigskysoftware:masterfrom
HeyHugo:HeyHugo-patch-1
Closed

Don't preventDefault on events that are filtered#1572
HeyHugo wants to merge 2 commits intobigskysoftware:masterfrom
HeyHugo:HeyHugo-patch-1

Conversation

@HeyHugo
Copy link

@HeyHugo HeyHugo commented Jul 14, 2023

Run the maybeFilterEvent() check before evt.preventDefault() so that events excluded via hx-trigger filters are ignored and not prevented.

Prior to this change <a href="/some/url" hx-get="/some/url" hx-trigger="click[!ctrlKey && !metaKey]"> would not let the browser use default ctrl + click behaviour due to the event being prevented.

This PR makes the suggestion here viable without adding another event listener on your own.

For a bit of context I have a use case where it makes sense to use hx-get on a <a> tag. It's a modal/overlay thing using hx-push-url and hx-history-elt. Direct browsing to the url makes sense and I'd really like to keep <a> ctrl/meta + click behaviour for the specific link. My current solution is to add another event listener specifically for click + modifier and then do window.open("/some/url", "_blank").

My workaround is to add something like this:

  const link = document.querySelector("#some-container a");
  const url = link.getAttribute('href');
  link.addEventListener("click", function (e) {
    if (e.metaKey || e.ctrlKey) {
      e.preventDefault();
      window.open(url, "_blank");
    }
  });

It works but requires me to reimplement the default browser behaviour with another event listener. I think it'd make sense in the general case that if you filter an event out explicitly it keeps whatever default there is.

@HeyHugo HeyHugo changed the title maybeFilterEvent prior to preventDefault Move maybeFilterEvent prior to preventDefault Jul 14, 2023
@HeyHugo
Copy link
Author

HeyHugo commented Jul 18, 2023

@alexpetros Aha a test had to be updated. Did so now, can you trigger workflow again 🙏

@HeyHugo HeyHugo force-pushed the HeyHugo-patch-1 branch 2 times, most recently from 7b931d3 to b0ac87a Compare July 18, 2023 08:30
HeyHugo added 2 commits July 18, 2023 11:45
Don't run `evt.preventDefault()` for the event before checking for event exclude filter.

Prior to this change `<a href="/some/url"` hx-get="/some/url" hx-trigger="click[!ctrlKey && !metaKey]"> would not let the browser use default ctrl + click behaviour due to the event being prevented.
@alexpetros alexpetros added the bug Something isn't working label Jul 18, 2023
@alexpetros
Copy link
Collaborator

Labeling this a bug because I think this is how it should work anyway. Want to be careful there's no unintended regression but I personally am pro this change.

@alexpetros alexpetros changed the title Move maybeFilterEvent prior to preventDefault Don't preventDefault on events that are filtered Jul 18, 2023
Copy link

@judyburgett3 judyburgett3 left a comment

Choose a reason for hiding this comment

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

thank you

@HeyHugo
Copy link
Author

HeyHugo commented Aug 19, 2023

@judyburgett3 Spamming won't help.

Meanwhile you can use the workaround I posted above.

@alexpetros
Copy link
Collaborator

Thanks Hugo. This is going to be looked at for 1.9.6 btw

Copy link

@judyburgett3 judyburgett3 left a comment

Choose a reason for hiding this comment

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

please make the changes. thank you.

Copy link

@judyburgett3 judyburgett3 left a comment

Choose a reason for hiding this comment

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

make the changes

@judyburgett3
Copy link

thank you

@bigskysoftware bigskysoftware deleted a comment from judyburgett3 Aug 27, 2023
@bigskysoftware bigskysoftware deleted a comment from judyburgett3 Aug 27, 2023
@bigskysoftware bigskysoftware deleted a comment from judyburgett3 Aug 27, 2023
@bigskysoftware bigskysoftware deleted a comment from judyburgett3 Aug 27, 2023
@bigskysoftware bigskysoftware deleted a comment from judyburgett3 Aug 27, 2023
@bigskysoftware bigskysoftware deleted a comment from judyburgett3 Aug 27, 2023
@bigskysoftware bigskysoftware deleted a comment from judyburgett3 Aug 27, 2023
@bigskysoftware bigskysoftware deleted a comment from judyburgett3 Aug 27, 2023
});

it('does not submit with a false condition on a form', function() {
it('submission is aborted but event default is not prevented when event is excluded via filter', function() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this a chance in behavior that could cause possible regressions?

Copy link
Author

@HeyHugo HeyHugo Aug 27, 2023

Choose a reason for hiding this comment

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

I would say no. If by any chance you rely on the event excluded by the filter preventing the default of that event you rely on a "bug" behavior.

There could be a case where someone is using a filter on a <a> element and relying on the behavior that any clicks even those excluded by filter does not navigate the user away. In this case one could say it's a regression, but I'd argue:

Why are you using an <a> element with href attribute if you don't intend to navigate the user? Perhaps replace it with a button.

It'd be an extremely rare regression worth the change to allow ctrl/meta + click links by only adding a filter.

Copy link
Contributor

Choose a reason for hiding this comment

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

what about someone who has an event filter on a form that has an hx-post on it?

if this change is accepted, the form would submit if the filter was false, no?

i see what you are saying here, but I think that this change might be a little too aggressive.

Copy link
Author

Choose a reason for hiding this comment

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

Hmm yes the filter on form / submit button case would have changed behaviour 🤔
I see this might be too breaking then.

There's probably no smoother way to support ctrl/meta-click default other than doing what I already did with a custom event-listener reimplement target="_blank"

@alexpetros alexpetros added the ready for review Issues that are ready to be considered for merging label Aug 27, 2023
@alexpetros
Copy link
Collaborator

Related: #1456

@1cg
Copy link
Contributor

1cg commented Sep 15, 2023

OK, closing this out for now although we should probably make this use case addressable some way, either via additional syntax in the trigger, events, etc. Thank you for your efforts here!

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

Labels

bug Something isn't working ready for review Issues that are ready to be considered for merging

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants