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

Using ViewTransitions API from SVG element #9001

Closed
1 task done
adrianAzoitei opened this issue Nov 5, 2023 · 8 comments · Fixed by #9140
Closed
1 task done

Using ViewTransitions API from SVG element #9001

adrianAzoitei opened this issue Nov 5, 2023 · 8 comments · Fixed by #9140
Labels
- P2: nice to have Not breaking anything but nice to have (priority) feat: view transitions Related to the View Transitions feature (scope)

Comments

@adrianAzoitei
Copy link

Astro Info

Astro                    v3.4.0
Node                     v18.16.0
System                   Linux (x64)
Package Manager          npm
Output                   static
Adapter                  none
Integrations             @astrojs/mdx
                         @astrojs/sitemap
                         @astrojs/tailwind

If this issue only occurs in one browser, which browser is a problem?

No response

Describe the Bug

I have an SVG element wrapped with <a href="..."></a>. It seems that, in this case, a full page refresh happens all the time, instead of Astro intercepting the click event and transitioning between the two pages.

Reproducible code:

<div class="p-5 min-w-[100%] md:min-w-[40%] overflow-visible">
  <svg>
    <a href=<some_page>>
      <circle cx="50" cy="50" r="40" stroke="black" stroke-width="3" fill="red" />
    </a>
  </svg>
</div>  

What does fix it is if I intercept the onclick event and call navigate().

<script>
import { navigate } from "astro:transitions/client";
document.addEventListener(`click`, e => {
  e.preventDefault();
  const origin = e.target.closest(`a`);
  
  if (origin) {
    navigate(<some_page>);
  }
});
</script>

What's the expected result?

When clicking an <a> element, Astro is able to intercept the event and perform the transition between pages.

Link to Minimal Reproducible Example

https://github.com/adrianAzoitei/svg-transitions

Participation

  • I am willing to submit a pull request for this issue.
@github-actions github-actions bot added the needs triage Issue needs to be triaged label Nov 5, 2023
@bluwy
Copy link
Member

bluwy commented Nov 6, 2023

Looks like we need to handle anchor tags inside SVG specifically to support this, which is always tricky to handle:

!(link instanceof HTMLAnchorElement) ||

Example handlers:

https://github.com/visionmedia/page.js/blob/4f9991658f9b9e3de9b6059bade93693af24d6bd/page.js#L768-L801

https://github.com/sveltejs/kit/blob/8724fa9a119f0cf6bb7fc779110c92278efada01/packages/kit/src/runtime/client/utils.js#L118-L141

@bluwy bluwy added - P2: nice to have Not breaking anything but nice to have (priority) feat: view transitions Related to the View Transitions feature (scope) and removed needs triage Issue needs to be triaged labels Nov 6, 2023
@martrapp
Copy link
Member

martrapp commented Nov 9, 2023

Hi @adrianAzoitei, thank you for pointing out that issue and for providing the minimal example. In deed the current implementation already calls closest() as you suggested:

document.addEventListener('click', (ev) => {
   let link = ev.target;
   if (link instanceof Element && link.tagName !== 'A') {
     link = link.closest('a');
   }
   if (
     !link ||
     !(link instanceof HTMLAnchorElement) ||

As @bluwy said, the problem is that the check for HTMLAnchorElement ignores SVGAElements. Thanks @bluwy for the quick and enlightening analysis!

@adrianAzoitei do you want to submit the pull request or should i fix?

@martrapp martrapp added the needs response Issue needs response from OP label Nov 9, 2023
@adrianAzoitei
Copy link
Author

Thanks both for the insights! I’ll submit a PR.

@martrapp martrapp removed the needs response Issue needs response from OP label Nov 9, 2023
@rishi-raj-jain
Copy link
Contributor

rishi-raj-jain commented Nov 10, 2023

@adrianAzoitei @martrapp

I just tried the repo and it does seem to be working for me.

Steps I followed:

@martrapp
Copy link
Member

Thanks both for the insights! I’ll submit a PR.

Hi @adrianAzoitei, do you need any assistance with your PR? I would be very happy if we could deliver this with the next minor release in the middle of next week.

@adrianAzoitei
Copy link
Author

@martrapp can only get to it the week after that. If anyone else has the bandwidth & and wants to pick it up, by all means.

@martrapp
Copy link
Member

Hi @adrianAzoitei, thanks for your quick reply. I will keep you updated.

@martrapp
Copy link
Member

@adrianAzoitei, links inside <svg> elements should work as soon the #9140 PR is released.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
- P2: nice to have Not breaking anything but nice to have (priority) feat: view transitions Related to the View Transitions feature (scope)
Projects
None yet
4 participants