Skip to content

Conversation

wkillerud
Copy link
Contributor

Description

Thanks for making it possible to use anchor positioning while supporting older browsers ❤️

I ran into one of the limitations of the current polyfill while writing a web component where both the anchor and target are inside the same shadow root. The reliance on document.querySelectorAll to find the elements to measure makes it so the current version can't work in shadow roots.

Open for other approaches here, but the gist of it is to pass in a shadow root if there is one to run querySelectorAll on that instead of document. I went for an approach binding the call to the component instance, but it could also be an extra root option to the function call that defaults to document or something like that.

Related Issue(s)

Steps to test/reproduce

npm run serve

Then visit http://localhost:3000/shadow-root.html

Show me

2025-10-01.at.11.49.58.mp4

Copy link

netlify bot commented Oct 1, 2025

Deploy Preview for anchor-polyfill ready!

Name Link
🔨 Latest commit cd5bb5e
🔍 Latest deploy log https://app.netlify.com/projects/anchor-polyfill/deploys/68e4ce38e52938000810d787
😎 Deploy Preview https://deploy-preview-353--anchor-polyfill.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

Copy link

netlify bot commented Oct 1, 2025

Deploy Preview for anchor-position-wpt canceled.

Name Link
🔨 Latest commit cd5bb5e
🔍 Latest deploy log https://app.netlify.com/projects/anchor-position-wpt/deploys/68e4ce387f9ceb0008ef4ad6

@jamesnw
Copy link
Contributor

jamesnw commented Oct 1, 2025

Thanks for this contribution! I'd definitely love to improve how the polyfill works with the shadow DOM. I don't necessarily think a PR needs to solve the entire problem to be helpful or merged, but am thinking as I type about how to make sure this is headed in the correct direction.

From the spec-

Positioned elements in shadow trees can reference anchor names defined in “higher” trees. Currently, they cannot reference anchor names defined in “lower” shadow trees, though.

Would this be possible with this approach?

The other portion of the spec related to shadow DOM is about using ::part, which I think is somewhat orthogonal to this PR.

I went for an approach binding the call to the component instance, but it could also be an extra root option to the function call that defaults to document or something like that.

Hmm, this is interesting. I see two uses for a root option-

  1. As an additive property- "here are additional roots to look at, in addition to document". This would address the use case in this PR. It would perhaps allow for tethering to an anchor in a higher tree, although we may need to consider the higher/lower relationships somehow
  2. As a scoping property- "All my positioned elements are in my site's navbar, so my root is that element". There might be some performance gain from limiting the search, although that may be minimal in practice.

I think both use cases would be possible by allowing an array, something like {root: [document, shadowRoot1, shadowRoot2]}. My sense is that a root option would be more flexible than binding the call.

@wkillerud
Copy link
Contributor Author

From the spec-

Positioned elements in shadow trees can reference anchor names defined in “higher” trees. Currently, they cannot reference anchor names defined in “lower” shadow trees, though.

Would this be possible with this approach?

It should be possible with some adjustments. I double-checked that it's possible for a child component to navigate up through a parent shadow root and into light DOM. We can use the presence of a host to know if we're entering a new shadow tree vs light DOM, for scoping.

We'll need an element locator that does this type of navigation up and into light DOM, querying along the way. Once it reaches light DOM it could skip straight to document unless we want to have that scoping property you mention with the root option.

The other portion of the spec related to shadow DOM is about using ::part, which I think is somewhat orthogonal to this PR.

Have to admit I'm not that familiar with ::part and how that would play out with the polyfill 😅 The selectors seem to be in the parent shadow tree or light DOM, but the anchor element itself is inside the shadow root, so measuring it for positioning would be tricky?

My sense is that a root option would be more flexible than binding the call.

That makes sense, let's do that instead then.

Just the one for now to make it work like before
Lets users skip querying for link and style tags inside a shadow root.
@wkillerud
Copy link
Contributor Author

wkillerud commented Oct 2, 2025

Positioned elements in shadow trees can reference anchor names defined in “higher” trees.

Do you know if this is implemented anywhere yet? The spec update was pretty recent, so maybe not. I'm trying to set up an example and control that it works in Chrome (checked Stable and Canary and the Chromium issue tracker). I might very well be doing something wrong, would love to not "fly blind" 😅

@jamesnw
Copy link
Contributor

jamesnw commented Oct 2, 2025

Positioned elements in shadow trees can reference anchor names defined in “higher” trees.

Do you know if this is implemented anywhere yet? The spec update was pretty recent, so maybe not. I'm trying to set up an example and control that it works in Chrome (checked Stable and Canary and the Chromium issue tracker). I might very well be doing something wrong, would love to not "fly blind" 😅

I'm not aware of this being implemented anywhere, I didn't see any relevant WPTs for this. I'd be content with opening a separate issue to support that, and knowing that this change doesn't make that change harder.

@wkillerud
Copy link
Contributor Author

The last test run failed with a diff above a threshold. Set up a devcontainer to see if it was an OS specific thing, but the tests pass there too. Maybe OK with a retry?

I'd be content with opening a separate issue to support that, and knowing that this change doesn't make that change harder.

Sounds good.

I think the root option is a good first step to add support. If a root has a host property it's a shadow tree and would need some extra steps.

Copy link
Contributor

@jamesnw jamesnw left a comment

Choose a reason for hiding this comment

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

This is looking great! I have some nits and questions. The tests did seem to pass on re-run.

@jgerigmeyer Do you have any objection to adding the .devcontainer?

One more area that would be useful to update would be the README.md. Could you document the root(s) option under Configuration, and also update the line about shadow-dom (L153) to reflect your changes?

Thanks so much for this contribution!

Copy link
Member

@jgerigmeyer jgerigmeyer left a comment

Choose a reason for hiding this comment

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

I like this approach -- and thanks for the contribution!

I'd probably prefer to remove the .devcontainer/ files, unless we add more documentation around them.

@wkillerud
Copy link
Contributor Author

I'd probably prefer to remove the .devcontainer/ files, unless we add more documentation around them.

I'll remove it then, it's quick to set up should anyone want it for themselves 👍

Copy link
Contributor

@jamesnw jamesnw left a comment

Choose a reason for hiding this comment

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

This looks and works great! Thanks for your contribution.

Some ponderings for future exploration (not really a review, but things I thought of while reviewing)-

  1. Could the custom properties be set on the new root instead of body?
  2. If so, what happens when multiple roots are passed?
  3. Could this be a recommended pattern for non-shadow root situations? By specifying a navbar as the root, for instance, there might be some performance enhancements over parsing unrelated DOM.

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.

3 participants