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.

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

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

Open
jbescoyez opened this issue May 14, 2021 · 9 comments
Open

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

jbescoyez opened this issue May 14, 2021 · 9 comments

Comments

@jbescoyez
Copy link

🐞 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

  • Put a <LinkTo> inside an element that stopPropagation on click.

https://ember-twiddle.com/bac43dc0c91d12e7a9afb1d4adfcaabd

😕 Actual Behavior

  • No transition
  • Default <a> behavior

🤔 Expected Behavior

  • An Ember transition to the route.

🌍 Environment

  • Ember: 3.26.1
  • Node.js/npm: 12.19.0 / 6.14.8
  • OS: MacOS 10.15.5
  • Browser: Chrome 90.0.4430.212
@chancancode
Copy link
Member

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
Copy link
Sponsor Contributor

@chancancode Did you mean to link to emberjs/rfcs#671 instead of #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
Copy link
Member

chancancode commented May 27, 2021

@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
Copy link

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
Copy link

ghost commented Jun 10, 2021

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
Copy link

ghost commented Jun 10, 2021

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
Copy link
Sponsor Contributor

@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 added a commit that referenced this issue Jun 10, 2021
During bubbling, `event.target` may point to a child element whereas
`event.currentTarget` always points to the element where the handler
was attached, which is what we want here.

Reported in a comment on #19546, though this may be a distinct issue
from the original report as it was reported as a default-cancelling
parent element interfering with the nested `<LinkTo>`, and this is
the other way around.
@chancancode
Copy link
Member

@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 added a commit that referenced this issue Jun 10, 2021
During bubbling, `event.target` may point to a child element whereas
`event.currentTarget` always points to the element where the handler
was attached, which is what we want here.

Reported in a comment on #19546, though this may be a distinct issue
from the original report as it was reported as a default-cancelling
parent element interfering with the nested `<LinkTo>`, and this is
the other way around.
chancancode added a commit that referenced this issue Jun 10, 2021
During bubbling, `event.target` may point to a child element whereas
`event.currentTarget` always points to the element where the handler
was attached, which is what we want here.

Reported in a comment on #19546, though this may be a distinct issue
from the original report as it was reported as a default-cancelling
parent element interfering with the nested `<LinkTo>`, and this is
the other way around.

(cherry picked from commit 57907a4)
chancancode added a commit that referenced this issue Jun 10, 2021
During bubbling, `event.target` may point to a child element whereas
`event.currentTarget` always points to the element where the handler
was attached, which is what we want here.

Reported in a comment on #19546, though this may be a distinct issue
from the original report as it was reported as a default-cancelling
parent element interfering with the nested `<LinkTo>`, and this is
the other way around.

(cherry picked from commit 57907a4)
@chancancode
Copy link
Member

That fix was released in 3.27.5

btecu pushed a commit to btecu/ember.js that referenced this issue Aug 18, 2021
During bubbling, `event.target` may point to a child element whereas
`event.currentTarget` always points to the element where the handler
was attached, which is what we want here.

Reported in a comment on emberjs#19546, though this may be a distinct issue
from the original report as it was reported as a default-cancelling
parent element interfering with the nested `<LinkTo>`, and this is
the other way around.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants