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

feat: Use composedPath to find anchor elements #2769

Merged
merged 4 commits into from
Nov 23, 2021

Conversation

andreavaccari
Copy link
Contributor

@andreavaccari andreavaccari commented Nov 10, 2021

Closes #2768

Changes

  • The client-side router registers event handlers on mouse movements and clicks that search anchor elements in an event's target's ancestors to perform prefetching or routing. The current implementation uses parentNode which "skips" anchor elements in shadow doms.
  • This change searches instead the event.composedPath(), which includes nodes in open shadow trees.

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests

  • Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

  • If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpx changeset and following the prompts. All changesets should be patch until SvelteKit 1.0

@changeset-bot
Copy link

changeset-bot bot commented Nov 10, 2021

🦋 Changeset detected

Latest commit: 254e3f5

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@sveltejs/kit Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

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

It would be great if you can add a test for web components as well!

packages/kit/src/runtime/client/router.js Outdated Show resolved Hide resolved
@andreavaccari
Copy link
Contributor Author

@bluwy Could you help me figure out where/how to add the test?

@bluwy
Copy link
Member

bluwy commented Nov 10, 2021

I think under packages/kit/test/apps/basics/src/routes/routing with a new directory called shadow-dom would be good. You can reference the other test structure as well. Ideally, we want to test the exact scenario you've mentioned in your issue with a custom web component and anchor tag in it.

@andreavaccari
Copy link
Contributor Author

Here is a first pass. I based the test on this. Please let me know if I can improve it.

@Rich-Harris
Copy link
Member

thank you! this looks good. any chance you could add a changeset, so that this triggers a version bump? for future reference it's useful to keep the pull request template in the comment as it includes a checklist/instructions for stuff like this

The client-side router registers event handlers on mouse movements and
clicks that search anchor elements in an event's target's ancestors to
perform prefetching or routing. The current implementation uses
`parentNode` which "skips" anchor elements in shadow doms.

This change searches instead the `event.composedPath()`, which includes
nodes in open shadow trees.
@andreavaccari
Copy link
Contributor Author

I included a changeset and added back the PR template. Excited for my first contribution to a project a love so dearly!

Also, congrats on the big news from yesterday! 👏

@Conduitry
Copy link
Member

https://developer.mozilla.org/en-US/docs/Web/API/Event/composedPath Using APIs like this would prevent us from (eventually) supporting IE11 without a prototype-modifying polyfill. (I haven't checked whether one exists for this particular API.) Would it be worth it to include a fallback here so that when we do support IE11 and other differential legacy build stuff, all those builds will have to do is transpile JS and include a couple of polyfills for things like Map?

It's also possible that that ship has already sailed and we're already using a bunch of APIs like this all over the place, and we'll just have to cross that bridge when we get to #12.

@bluwy
Copy link
Member

bluwy commented Nov 13, 2021

Looks like a polyfill is available but yeah it's modifying the prototype. I'd personally drop IE11 support and direct users to the polyfill though. We can document this in either #12 or in code to not forget about it.

@Rich-Harris
Copy link
Member

I'd personally lean towards documenting the polyfill requirement. Or better still, injecting polyfills for stuff like this if a legacy: true or target: 'ie11' option or whatever is set. Given that we don't yet have a good answer for IE11 support anyway, I reckon we can merge this without worrying about it too much for now

@andreavaccari
Copy link
Contributor Author

For this particular situation, given that IE11 doesn't support the Shadow DOM, we could also check if composedPath is not defined and fall back to the old behavior in that case.

@bluwy
Copy link
Member

bluwy commented Nov 15, 2021

@andreavaccari I think that's the discussion we're having now, whether to add those extra bytes of code or not. Seems like the consensus is to document it, so I'm putting my approval on this. Thanks for adding this feature!

@Rich-Harris Rich-Harris merged commit c90c8aa into sveltejs:master Nov 23, 2021
@Rich-Harris
Copy link
Member

thanks!

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

Successfully merging this pull request may close these issues.

Client-side router should check event.composedPath() to intercept link clicks
4 participants