search

emberjs / ember.js

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

[Bug] LinkTo won't transition if a parent element stopPropagation #19546

Open jbescoyez opened 3 years ago

jbescoyez commented 3 years ago

🐞 Describe the Bug

When a <LinkTo> is inserted inside an element that stopPropagation of click's, it won't transition but rather fallback on a default <a> behavior.

🔬 Minimal Reproduction

https://ember-twiddle.com/bac43dc0c91d12e7a9afb1d4adfcaabd

😕 Actual Behavior

🤔 Expected Behavior

🌍 Environment

chancancode commented 3 years ago

This is one of those hairy things that should be fixed in 3.27 / RFC #671. This is true for all classic components – because they use event delegation and listens on the rootElement (body by default), if the event is canceled before it makes it to the top then it will not trigger on the component. This is something to generally pay attention to when mixing classic components and modifiers. In the long run it will be less of an issue as components are migrated, and in the case of <LinkTo> specifically, 3.27 moves away from the classic implementation.

kpfefferle commented 3 years ago

@chancancode Did you mean to link to https://github.com/emberjs/rfcs/pull/671 instead of https://github.com/emberjs/ember.js/issues/671?

We are now experiencing an issue in our Production builds after upgrading to 3.27 where LinkTos will (seemingly) randomly decide to act as standard <a> tags instead of as Ember route transitions. The work related to the RFC seems to be a likely culprit 😬

chancancode commented 3 years ago

@kpfefferle hmm, that is strange: I would think in 3.27 you would be less likely to run into issues like that, because on the modernized version the handler is attached directly on the <a> element using the {{on}} modifier, whereas before it is using event delegation on the <body>, so if the event get canceled before it reaches the top things would not work as expected.

One thing you can try is to call LinkComponent.reopen(). This forces ember to fallback to the old implementation (you will get a deprecation), so you can use that to test if the issues you are expecting is related to the change or not.

sinankeskin commented 3 years ago

We are now experiencing an issue in our Production builds after upgrading to 3.27 where LinkTos will (seemingly) randomly decide to act as standard <a> tags instead of as Ember route transitions. The work related to the RFC seems to be a likely culprit 😬

Same here with 3.27. I don't now how linkTo decides. As @kpfefferle said some behaves like an a tag and creates a request some uses the router. No issue on 3.26.x.

ghost commented 3 years ago

Something very similar here: upgrading to 3.27+ would make linkTos that contain any other tag, anything more than just text, behave like regular <a>s and reload the app instead of transitioning. It happens for production builds only. In dev they retain the usual behaviour.

ghost commented 3 years ago

This (from -internals/glimmer/lib/components/link-to.ts):

    let element = event.target;
    assert('[BUG] must be an <a> element', element instanceof HTMLAnchorElement);

    let isSelf = element.target === '' || element.target === '_self';

    if (isSelf) {
      this.preventDefault(event);
    } else {
      return;
    }

fails when the LinkTo contains any other element, and the click goes to the contained element. The element is the contained element, assert is skipped in production, and isSelf is false.

kpfefferle commented 3 years ago

@lele-melis This is a great find! It seems consistent with the issues we're seeing too - as the links that exhibit this behavior for us often contain something like an SVG icon 🕵️‍♀️

@chancancode Does this additional info give you any ideas why the recent refactor may have caused this regression? Did removing the body event delegation somehow break the event delegation from possible LinkTo child elements? 🤔

chancancode commented 3 years ago

@lele-melis @kpfefferle Good find, thanks for reporting and sorry it wasn't caught sooner (the assertions do run in tests, as well as inside the test suite in apps). I proposed a fix in #19597, but I am not sure if this is actually the same issue @jbescoyez reported as this parent-child relationship is the other way around here.

chancancode commented 3 years ago

That fix was released in 3.27.5