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.

Sign up for GitHub

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

Jump to bottom

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

Open
wants to merge 14 commits into
base: dev
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
6c53c61
fix: example buttons in darkmode (#2719)
Rooyca Jul 13, 2024
e73831b
Bump ws from 8.14.2 to 8.17.1 (#2718)
dependabot[bot] Jul 13, 2024
b71bc93
Fix default `scrollBehavior` value in docs (#2729)
bencroker Jul 16, 2024
6dd555e
Update README.md - fix url to htmx extensions (#2732)
ASMag Jul 17, 2024
5f27c78
Stop center aligning docs nav on small screen (#2740)
bobaekang Jul 18, 2024
80c0856
Bump Zola version to 0.19.1 (#2739)
danieljsummers Jul 18, 2024
24944a3
fixed typo in hx-trigger.js (#2728)
7flash Jul 26, 2024
5f57fcd
docs: update jetbrains sponsor logo (#2551)
niklas-wortmann Jul 26, 2024
1c773df
test case for uncovered case added
Jul 29, 2024
93e48ce
test added
Jul 29, 2024
86a53ca
form clicks only prevented when click came from form
Jul 29, 2024
f010924
old tests regarding shouldCancle adapted
Jul 29, 2024
8817374
target can be null
Jul 29, 2024
5389f07
manuall test added, mvd test to eof
Jul 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ directly in HTML, using [attributes](https://htmx.org/reference#attributes), so

htmx is small ([~14k min.gz'd](https://unpkg.com/htmx.org/dist/)),
[dependency-free](https://github.com/bigskysoftware/htmx/blob/master/package.json) &
[extendable](https://htmx.org/extensions)
[extendable](https://extensions.htmx.org/)

## motivation

Expand Down
2 changes: 1 addition & 1 deletion netlify.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ publish = "public"
command = "zola build"

[build.environment]
ZOLA_VERSION = "0.17.1"
ZOLA_VERSION = "0.19.1"

[context.deploy-preview]
command = "zola build --base-url $DEPLOY_PRIME_URL"
31 changes: 5 additions & 26 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions src/htmx.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,7 +206,7 @@ var htmx = (function() {
disableSelector: '[hx-disable], [data-hx-disable]',
/**
* @type {'auto' | 'instant' | 'smooth'}
* @default 'smooth'
* @default 'instant'
*/
scrollBehavior: 'instant',
/**
Expand Down Expand Up @@ -2313,7 +2313,7 @@ var htmx = (function() {
return false
}
if (evt.type === 'submit' || evt.type === 'click') {
if (elt.tagName === 'FORM') {
if (elt.tagName === 'FORM' && asElement(evt.target)?.tagName === 'FORM') {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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!

Copy link
Author

Choose a reason for hiding this comment

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

@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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

return true
}
if (matches(elt, 'input[type="submit"], button') && closest(elt, 'form') !== null) {
Expand Down
2 changes: 1 addition & 1 deletion test/attributes/hx-trigger.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -657,7 +657,7 @@ describe('hx-trigger attribute', function() {
}, 50)
})

it('A throttle of 0 does not multiple requests from happening', function(done) {
it('A throttle of 0 does not prevent multiple requests from happening', function(done) {
var requests = 0
var server = this.server
server.respondWith('GET', '/test', function(xhr) {
Expand Down
11 changes: 8 additions & 3 deletions test/core/internals.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,15 +94,20 @@ describe('Core htmx internals Tests', function() {
htmx._('shouldCancel')({ type: 'click' }, anchorThatShouldNotCancel).should.equal(false)

var form = make('<form></form>')
htmx._('shouldCancel')({ type: 'submit' }, form).should.equal(true)
htmx._('shouldCancel')({ type: 'submit', target: document.createElement('form') }, form).should.equal(true)

htmx._('shouldCancel')({ type: 'click', target: document.createElement('form') }, form).should.equal(true)

// function works also when target isn't an Element
htmx._('shouldCancel')({ type: 'click', target: null }, form).should.equal(false)

var form = make("<form><input id='i1' type='submit'></form>")
var input = byId('i1')
htmx._('shouldCancel')({ type: 'click' }, input).should.equal(true)
htmx._('shouldCancel')({ type: 'click', target: document.createElement('input') }, input).should.equal(true)
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

Yes you're right


var form = make("<form><button id='b1' type='submit'></form>")
var button = byId('b1')
htmx._('shouldCancel')({ type: 'click' }, button).should.equal(true)
htmx._('shouldCancel')({ type: 'click', target: document.createElement('button') }, button).should.equal(true)
})

it('unset properly unsets a given attribute', function() {
Expand Down
21 changes: 21 additions & 0 deletions test/core/regressions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -271,3 +271,24 @@ describe('Core htmx Regression Tests', function() {
}, 50)
})
})

it('a modified click trigger on a form does not prevent the default behaviour of other elements - https://github.com/bigskysoftware/htmx/issues/2755', function(done) {
var defaultPrevented = 'unset'
make('<input type="date" id="datefield">')
make('<form hx-trigger="click from:body"></form>')

htmx.on('#datefield', 'click', function(evt) {
// we need to wait so the state of the evt is finalized
setTimeout(() => {
defaultPrevented = evt.defaultPrevented
try {
defaultPrevented.should.equal(false)
done()
} catch (err) {
done(err)
}
}, 0)
})

byId('datefield').click()
})
29 changes: 29 additions & 0 deletions test/manual/form-trigger.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
<html lang="en">
<head>
<meta charset="utf-8" />
<title>Mocha Tests</title>
<meta name="viewport" content="width=device-width, initial-scale=1.0" />
<link rel="stylesheet" href="../node_modules/mocha/mocha.css" />
<meta http-equiv="cache-control" content="no-cache, must-revalidate, post-check=0, pre-check=0" />
<meta http-equiv="cache-control" content="max-age=0" />
<meta http-equiv="expires" content="0" />
<meta http-equiv="expires" content="Tue, 01 Jan 1980 1:00:00 GMT" />
<meta http-equiv="pragma" content="no-cache" />
<meta name="htmx-config" content='{"historyEnabled":false,"defaultSettleDelay":0}'>
</head>
<body style="padding:20px;font-family: sans-serif">

<h1 style="margin-top: 40px">Working Checkbox and Datepicker</h1>
<p>A modified click trigger on a form, doesn't prevent the default behaviour of other elements</p>
<p>The checkbox can be checked, a click on the datepicker symbol opens the datepicker flyout</p>

<script src="../src/htmx.js"></script>

<input type="date">
<input type="checkbox">
<form hx-trigger="click from:body">
<input type="hidden" value="foo">
</form>
</div>
</body>
</html>
2 changes: 1 addition & 1 deletion www/content/_index.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,7 @@ Thank you to all our generous <a href="https://github.com/sponsors/bigskysoftwar
</tr>
<tr>
<td>
<a data-github-account="JetBrainsOfficial" href="https://www.jetbrains.com"><img src="/img/jetbrains.png" alt="Jetbrains" style="max-width:30%;min-width:100px;"></a>
<a data-github-account="JetBrainsOfficial" href="https://www.jetbrains.com"><img src="/img/jetbrains.svg" alt="Jetbrains" style="max-width:30%;min-width:100px;"></a>
</td>
<td>
<a data-github-account="commspace" href="https://www.commspace.co.za">
Expand Down
4 changes: 2 additions & 2 deletions www/content/examples/click-to-edit.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,8 +86,8 @@ return `<form hx-put="/contact/1" hx-target="this" hx-swap="outerHTML">
<label for="email">Email Address</label>
<input type="email" id="email" name="email" value="${contact.email}">
</div>
<button class="btn" type="submit">Submit</button>
<button class="btn" hx-get="/contact/1">Cancel</button>
<button class="btn primary" type="submit">Submit</button>
<button class="btn danger" hx-get="/contact/1">Cancel</button>
</form>`
}

Expand Down
2 changes: 1 addition & 1 deletion www/content/reference.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,7 +240,7 @@ listed below:
| `htmx.config.disableSelector` | defaults to `[hx-disable], [data-hx-disable]`, htmx will not process elements with this attribute on it or a parent |
| `htmx.config.withCredentials` | defaults to `false`, allow cross-site Access-Control requests using credentials such as cookies, authorization headers or TLS client certificates |
| `htmx.config.timeout` | defaults to 0, the number of milliseconds a request can take before automatically being terminated |
| `htmx.config.scrollBehavior` | defaults to 'smooth', the behavior for a boosted link on page transitions. The allowed values are `auto` and `smooth`. Smooth will smoothscroll to the top of the page while auto will behave like a vanilla link. |
| `htmx.config.scrollBehavior` | defaults to 'instant', the behavior for a boosted link on page transitions. The allowed values are `auto`, `instant` and `smooth`. Instant will scroll instantly in a single jump, smooth will scroll smoothly, while auto will behave like a vanilla link. |
| `htmx.config.defaultFocusScroll` | if the focused element should be scrolled into view, defaults to false and can be overridden using the [focus-scroll](@/attributes/hx-swap.md#focus-scroll) swap modifier. |
| `htmx.config.getCacheBusterParam` | defaults to false, if set to true htmx will append the target element to the `GET` request in the format `org.htmx.cache-buster=targetElementId` |
| `htmx.config.globalViewTransitions` | if set to `true`, htmx will use the [View Transition](https://developer.mozilla.org/en-US/docs/Web/API/View_Transitions_API) API when swapping in new content. |
Expand Down
Binary file removed www/static/img/jetbrains.png
View file
Open in desktop
Binary file not shown.
17 changes: 17 additions & 0 deletions www/static/img/jetbrains.svg
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
1 change: 0 additions & 1 deletion www/themes/htmx-theme/static/css/site.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -270,7 +270,6 @@ ol li {
float: right;
}
.nav {
text-align: center;
font-size: 1.2em;
line-height: 1.3em;
}
Expand Down