Bug-Fix: Modified click trigger on form elements prevent default behaviour of clicked element

[pfiadDi]

Description

The shouldCancle function prevents the default behaviour for form elements when clicked. The problem is, when a dynamically added element should trigger the form and the user adds something like: hx-trigger="click[event.target.matches('button#dynamicallyAddedElement')] from:body" the default behaviour of other elements are prevented as well.

A checkbox can't be checked anymore for example.

I added a check so that we only cancel the event when the node with the trigger is a form AND the event target is also one:
Old:
if (elt.tagName === 'FORM') { return true }

New:
if (elt.tagName === 'FORM' && asElement(evt.target)?.tagName === 'FORM') { return true }

Corresponding issue: #2755

Testing

Manually:

• Added an input field type date and a form with hx-trigger click from:body
• Clicked on the date picker symbol and got the expected datepicker flyout

New tests:

• added a test with the same setup and checked in the event object if property "defaultPrevented" equals false (tbh wasn't sure where so I added it to re
• adapted the existing tests in internal to work with the adapted function
• added one test for a missing case in function "shouldCancel"

Checklist

• I have read the contribution guidelines
• I have targeted this PR against the correct branch (master for website changes, dev for
  source changes)
• This is either a bugfix, a documentation update, or a new feature that has been explicitly
  approved via an issue
• [] I ran the test suite locally (npm run test) and verified that it succeeded -> YES and NO:

I had troubles with the test suite also before working on the bug, I always got this result:
641 passing (9s)
3 pending
2 failing

1. Core htmx Parameter Handling
   form includes last focused button:
   Error: Timeout of 2000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves.

2. Core htmx Parameter Handling
   form includes last focused submit:
   Error: Timeout of 2000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves.

After adding my fix and the new tests, my new tests were successful and the same two failed...

[Telroshan]

Hey,

• Could you rebase against dev? Seems like you are bringing some unrelated commits along, which makes the diff harder to review.
• I'm worried about this sentence

adapted the existing tests in internal to work with the adapted function

As we don't want to introduce breaking changes to the lib, and as shouldCancel is exposed to the internal API (that any of the official, community of custom extensions could use), existing tests shouldn't need to be modified to use the new syntax. Ideally, the fallback should work as it did before in these tests, as if I understood correctly, this is about a bugged behavior that hadn't been identified since now, but the tested ones are fine

As for the failing tests, these timeouts are sometimes annoying, maybe try increasing the timeout values if you have a not-so-great internet connection? As those tests are making requests to an external domain to test the security features

[pfiadDi]

Hi @Telroshan thank you very much for your feedback. It's my first PR I highly appreciate it :)
I'll work on that later today or tomorrow
Br

[pfiadDi]

Hi, @Telroshan I rebased it against dev.
About your other comments:

I'm worried about this sentence
adapted the existing tests in internal to work with the adapted function

You are absolutely right but I think in this case it's acceptable.

What did I change?

• Goal: The def. behav. of a form should be prevent when the FORM is clicked
• Solution: To achieve that I check the evt object for it's tagname:

if (elt.tagName === 'FORM' **&& asElement(evt.target)?.tagName === 'FORM'**) {

Why do I had to change existing tests?
The existing tests don't pass a full evt object the just an object with the evt type:

htmx/test/core/internals.js

Line 97 in 45d4bec

htmx._('shouldCancel')({ type: 'submit' }, form).should.equal(true)

So I had to add to those tests the a target object:
https://github.com/pfiadDi/htmx/blob/5389f07c55803a9864806be5100dfb49c7ac6a2e/test/core/internals.js#L97

In short: Yes I had to change existing tests BUT I only added a object property that in reality is present.

About the failing tests:
(timeout and pending)
I tried to increase timeout without success.

I assume it has something to do with the mocha-chrome lib. When I start the tests I get this error:

[chrome-exception] { timestamp: 1722514436511.944, exceptionDetails: { exceptionId: 2, text: 'Uncaught', lineNumber: 0, columnNumber: 0, scriptId: '15', url: 'file:///media/thomas/externe/Projekte/htmx/node_modules/mocha-webdriver/dist/index.js', exception: { type: 'object', subtype: 'error', className: 'SyntaxError', description: "SyntaxError: Identifier 'chrome' has already been declared", objectId: '-5334836499612074012.2.3', preview: [Object] }, executionContextId: 2 } }

So I don't know how to proceed - I am confident the failing and pendig tests have nothing todo with my changes because the same fail when I run them on master or dev of the main repo.

Thanks for your help
Br

[Telroshan]

Hey @pfiadDi , seems the rebase went wrong as other unrelated commits are still here 😬

Got it, you're right, didn't notice these tests were incorrectly mocking an event's properties.

[Telroshan]

Shouldn't we use the input var here though instead of creating a dummy element on the fly? As input is the clicked element, the event's target would be that very element in a real situation right?

Same goes for the form above, and the button after

[pfiadDi]

Yes you're right

[Telroshan]

I'm wondering ; couldn't we simply check if (asElement(evt.target) || elt).tagName === 'FORM' in this case?

So that, if defined, evt.target takes precedence, otherwise we keep the existing behavior. You would probably not even need to add the target property to mock the events of the test suite in this case? As elt is the fallback

Btw, should we use the same technique for the other checks in this same function? Anchors for ex. Maybe we should simply change toconst elt = asElement(evt.target) || asElement(node) at the start of the function and not need to modify anything else inside?
Just some thoughts, completely untested, let me know!

[pfiadDi]

@Telroshan the problem is, the event target is an EventTarget and an EventTarget can be a Dom element but doesn't have to be. So the property tagname can be undefined. (It was my first approach but the type checker complaint). That's why I chose this way to ensure we only check properties which exist and don't run into any errors when trying to access undefined properties

[Telroshan]

an EventTarget can be a Dom element but doesn't have to be. So the property tagname can be undefined.

Hence my suggestion @pfiadDi : const elt = asElement(evt.target) || asElement(node)
Unless I'm mistaked (could totally be, didn't test this 😆 ), this will get the evt.target if it's a DOM element, but if it's not an element, will fallback to node.
Then we have the already present if statement that returns early, in case neither evt.target nor node were elements.
I believe you shouldn't get any type checker complaint with that approach as we cast to Element thanks to asElement?