Same-page anchor: Account for <a> nested in <svg>

[seanpdoyle]

Follow-up to #1285

While an <a> element is typically represented by an HTMLAnchorElement, an <a> nested inside an <svg> element is instead an SVGAElement.

Similarly, while HTMLAnchorElement.href returns a String, the SVGAElement.href returns an instance of SVGAnimatedString, which cannot be supported by neither calls to String.startsWith nor RegExp.test.

This commit adds explicit test coverage for a same-page navigation from an <a> nested within an <svg>. To ensure that the value is a String, replace .href with getAttribute("href"). This restores the implementation to be closer to what was defined in e591ea9e.

Note

It's worth mentioning that both of these new tests pass without the page.on("pageerror") parts are omitted from the coverage. That signals to me that either:

1. the intended behavior still functions when JavaScript errors are raised,
2. the test coverage does not genuinely reflect real-world use cases

I'm hopeful that it's 1., and that the test coverage with the explicit JavaScript error tracking sufficiently guards against both 1. and 2. being true.

[domchristie]

Related: Astro's ClientRouter approach: https://github.com/withastro/astro/blob/dbf71c021e3adf060c34e6f268cf1b5cd480233d/packages/astro/components/ClientRouter.astro#L82-L83

[seanpdoyle]

@domchristie interesting! Thanks for sharing that context. I'll copy-paste it here to keep those details as part of this conversation:

if (
  !(link instanceof HTMLAnchorElement) &&
  !(link instanceof SVGAElement) &&
  !(link instanceof HTMLAreaElement)
)
  return

// …

const linkTarget = link instanceof HTMLElement ? link.target : link.target.baseVal;
const href = link instanceof HTMLElement ? link.href : link.href.baseVal;

I like that approach in that there are some class-level checks. However, I worry about a future need of adding additional supported classes (in case we miss one now, or in case the "link" is an <a>-like custom HTML element of some kind).

For now, I think I prefer the getAttribute-based approach for two reasons: 1) it's closer to the original code, 2) it doesn't limit the classes supported.

Am I missing out on something about the alternative?

[domchristie]

Am I missing out on something about the alternative?

No I don't think so. The use of baseVal was interesting to me, but I think getAttribute works well for both (as it does for the target attribute. I feel like some of the Astro code is to satisfy TypeScript, particularly the instanceof guards.

[packagethief]

Thanks for looking into this @seanpdoyle 🙏

[fschwahn]

@seanpdoyle I tried to test this, but it seems JS regexp does not support \A as a start of string anchor, that's a ruby thing. (I think tests are failing because of this too).

/\A#/.test("#foo") // false
/\A#/.test("A#") // true

[fschwahn]

I'm hopeful that it's 1., and that the test coverage with the explicit JavaScript error tracking sufficiently guards against both 1. and 2. being true.

I went back and tested this, and the claim that this broke links in SVGs was wrong, I'm sorry about that. In fact, the links still behave as intended, there's just noise from the errors in the browser console.

So I think (at least in my case) it is indeed 1.