emberjs / ember.js

Ember.js - A JavaScript framework for creating ambitious web applications
https://emberjs.com
MIT License
22.45k stars 4.21k forks source link

[Bug] Unable to prevent default <LinkTo> behavior without blocking all event handlers #19861

Open zzzzBov opened 2 years ago

zzzzBov commented 2 years ago

🐞 Describe the Bug

When using the <LinkTo> component, it's occasionally helpful to override the default link behavior, such as by opening a modal instead of navigating to a page of the same content.

Ideally this would be handled via a click handler using event.preventDefault(), but unfortunately LinkTo doesn't check event.defaultPrevented before performing its default click behavior.

This leads to some complications because calling stopPropagation/stopImmediatePropagation prevent all event handlers from executing, which can be problematic if other handlers—such as analytics tracking—are also bound to the element or any of its ancestors.

πŸ”¬ Minimal Reproduction

https://ember-twiddle.com/c5143970c9a673adff023dc106faedcc is an example that shows two links that have click handlers.

πŸ˜• Actual Behavior

The first uses preventDefault which is taken to the /example route, but still allows the parent event handler to execute. The second uses stopPropagation which does not navigate to /example, but also doesn't allow the parent event handler to execute.

πŸ€” Expected Behavior

In the case of the first link, I'd expect clicking on the link to call both handlers and not navigate to the /example route.

🌍 Environment

βž• Additional Context

MelSumner commented 2 years ago

Should Ember support this? Opening a modal should be done with a button, not a link.

zzzzBov commented 2 years ago

Navigating to a document is a link behavior. Not all modals are stand-alone documents, but in my very specific case it is, and I need to perform some non-default functionality for visitors that use the link directly.

What I don't want to have to do is build a custom component to render a link to a route when all that's needed is an extra check in LinkTo:

if (!isSimpleClick(event) || event.defaultPrevented) {
  return;
}

Someone middle- or control-clicking on the link ought to be taken to the correct document, without the special handling (I'm aware my example code doesn't include the isSimpleClick check necessary for that behavior).

NullVoxPopuli commented 2 years ago

Instead of using a click handler to open the Modal, what if you you use the router service and derived data to open the modal?

like:

@service router;

get isModalActive() {
  return this.router.isActive('whatever route you need');
}
{{#if this.isModalActive}}
  <MyModal />
{{/if}}
zzzzBov commented 2 years ago

Please don't pin on the specifics of opening a modal for this issue. It's just a single example of why I feel <LinkTo> is not well behaved.

There are a variety of niche reasons why it's useful to prevent the default linking behavior (such as a custom scroll handler for a link with a document fragment identifier) and the issue here is that <LinkTo> doesn't allow for those sorts of scenarios.

NullVoxPopuli commented 2 years ago

Fair point, I shouldn't assume you're trying to solve a specific problem -- because I mean, given the constraints of our routing environment, even routing to a fragment can be done with: https://github.com/CrowdStrike/ember-url-hash-polyfill which is -- again, interacting with the router rather than the links, which works everywhere, rather than for specific links.

idk, my point is that I personally don't think anchor tags should be used for anything other than routing and navigation. But, I'm not on w3c, so :shrug: -- I do find things easier when I work around that assumption tho

rlivsey commented 1 year ago

An example of us running into this recently is displaying buttons / other UI inside links, overly simplified example:

<LinkTo>
  primary action

  <button {{on "click" (prevent-default)}}>sub action</button>
</LinkTo>

Real-world example, we display a list of conversations which we have implemented as links because we want to be a good citizen and support cmd+click opening in new tabs etc… Later on we want to also display some secondary actions in each item in a … button:

Screenshot 2023-01-04 at 11 40 35

Clicking the … shouldn't navigate to the item, but we're now having to resort to all sorts of workarounds to avoid this. Calling stopPropagation works in some situations, but is a hammer to crack a nut as it then breaks things like pop-overs.

zzzzBov commented 1 year ago

@rlivsey per the spec, anchors may not have interactive descendants, so I'd recommend taking a different approach for that particular issue.

Years ago I used to be a proponent of nesting controls, but it's proven to be bad for accessibility.

The ARIA in HTML spec has an entire section on correct nesting when making use of ARIA so the right approach is to not nest interactive elements.

For example, in your case, I would recommend a grouping container using grid layout to position the button over the link visually.

robbytx commented 1 year ago

I vote for the event.defaultPrevented condition to be checked. There are various places where I'd like to render a link to a route, but where a simple click on that route does not automatically transition to the route.

My current use case is to facilitate copying a link to the clipboard for sending in email or sharing in some other way. Using a <LinkTo> component is the most straightforward way to render the link URL, incorporating model & query parameters, and then I'd like to do {{on "click" (prevent-default (fn this.copyLinkToClipboard))}}. Using a <LinkTo> here additional shows the link on hover, and allows the user to right-click and copy it themselves if they aren't sure what the app will do.

Another use case is for nav menu items, where the menu item itself is a default link, but upon hover or click, it actually expands to a sub-menu containing additional links.

Honestly, I don't see the argument for not honoring defaultPrevented. Clearly the behavior of <LinkTo> is to replace a standard <a> so why shouldn't it behave the same in this way?

kategengler commented 4 months ago

We are very unlikely to change the behavior of at this point. I expect the framework to at some point suggest using anchors directly, as we originally discussed in emberjs/rfcs@master/text/0391-router-helpers.md

For now, you can make your own url-for helper using RouterService#urlFor or the ember-router-helpers addon.