knockout / knockout

Knockout makes it easier to create rich, responsive UIs with JavaScript
http://knockoutjs.com/
Other
10.43k stars 1.52k forks source link

anchor with data-bound href attr requires a click binding returning true with a clickBubble set to false #2565

Closed mig8447 closed 2 years ago

mig8447 commented 3 years ago

This is rather an enhancement request but I came across this SO question: https://stackoverflow.com/questions/18414398/knockout-js-dynamic-links-do-not-click-through because I found the same issue described.

When a link is clicked, and the href is data-bound via the attr binding, it doesn't respond to clicks unless I add a click: () => true, clickBubble: false bind, which is weird because according to the documentation:

By default, Knockout will prevent the click event from taking any default action. This means that if you use the click binding on an a tag (a link), for example, the browser will only call your handler function and will not navigate to the link’s href. This is a useful default because when you use the click binding, it’s normally because you’re using the link as part of a UI that manipulates your view model, not as a regular hyperlink to another web page.

Which from my interpretation means: If and only if the click binding is applied, then KO prevents the default action of the anchor. But for some reason this is not working as I'd expect and needs the rather convoluted markup:

<a data-bind="attr: { href: url }, click: () => true, clickBubble: false">
   <!-- ... -->
</a>

This is not critical since there's a workaround but I'd like to be able to do

<a data-bind="attr: { href: url }">
   <!-- ... -->
</a>

instead. I tested with several permutations of options and indeed that was the only one I came across that worked

karimayachi commented 3 years ago

Hi @mig8447 ,

Which from my interpretation means: If and only if the click binding is applied, then KO prevents the default action of the anchor.

This interpretation is correct (I use this all the time). See this JSFiddle: https://jsfiddle.net/odrem64y/8/

<a data-bind="attr: { href: url }">
   <!-- ... -->
</a>

should just work. The SO-post you referenced is from 2013. Are you using a recent version of KO? If so, do you have a Codepen or JSFiddle example that doesn't work?

mig8447 commented 3 years ago

Hi @karimayachi, I'm using 3.5.1, let me create a jsfiddle with an example, I'm using components though, so maybe something with templates might be the culprit, but will send the jsfiddle a little bit later.

mbest commented 3 years ago

Perhaps it's a browser quirk. I tried the examples in the SO question with the old version of Knockout, and they work fine for me with just the attr binding using Chrome.

webketje commented 2 years ago

@mbest maybe this does warrant some extra thought? I forked @karimayachi 's fiddle: https://jsfiddle.net/kevinvanlierde/qkx6c4jr/.

As you can see the issue is that links nested inside a parent element with a click binding will not trigger due to the event being prevented by default. The only way to solve this is by returning false in the click-bound parent listener, or, as @mig8447 observed, by adding a useless clickhandler, and a clickBubble to the anchor.

Perhaps line 34 in the event binding should be amended? Maybe the assumption in the comment was useful in the past, but it doesn't / no longer does ring true for me. Like, if I want to prevent the default, let me decide if I want to e.preventDefault. https://github.com/knockout/knockout/blob/2bec689a9a7fcaaed37e6abb9fdec648f86c4f81/src/binding/defaultBindings/event.js#L34

E.g.

var hasLogicalDefaultAction = element.nodeName === 'A' || (element.nodeName === 'BUTTON' && element.type === 'submit');
if (handlerReturnValue !== true && !hasLogicalDefaultAction)

Ofc., potentially not backward-compatible.

mbest commented 2 years ago

Thanks for the additional information. It seems that the current functionality is consistent with the documentation. You have two options:

  1. Return true from the click handler in the parent element. This allows the default action of the click event to happen. This logic could even be conditional depending on the target element.
  2. Add a binding to the child element that will stop propagation. As pointed out, you can use the built-in click and clickBubble bindings. You can also create a custom binding for this if it's a common occurrence in your code.