bigskysoftware
Bug-Fix: Modified click trigger on form elements prevent default behaviour of clicked element
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: https://github.com/bigskysoftware/htmx/issues/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
- [x] I have read the contribution guidelines
- [x] I have targeted this PR against the correct branch (
masterfor website changes,devfor source changes) - [x] 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
-
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.
-
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...
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
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
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: https://github.com/bigskysoftware/htmx/blob/45d4bec43fc226ec4fd22bc4790550d67d84e019/test/core/internals.js#L97
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
