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

Hash links to new pages don't focus the correct element #3770

Open
Rich-Harris opened this issue Feb 7, 2022 · 14 comments · May be fixed by #10856
Open

Hash links to new pages don't focus the correct element #3770

Rich-Harris opened this issue Feb 7, 2022 · 14 comments · May be fixed by #10856
Labels
bug Something isn't working help wanted PRs welcomed. The implementation details are unlikely to cause debate p2-nice-to-have SvelteKit cannot be used by a small number of people, quality of life improvements, etc.
Milestone

Comments

@Rich-Harris
Copy link
Member

Describe the bug

If you click a link like <a href="#foo">, the browser will focus an element like <h2 id="foo">, which in effect (since <h2> elements aren't typically focusable) actually means that pressing Tab will focus the next focusable element after the <h2>.

But if you click a link like <a href="/other#foo">, SvelteKit will navigate to /other and scroll to #foo, but it won't focus the element.

Reproduction

https://stackblitz.com/edit/sveltejs-kit-template-default-nrjdx2

Logs

No response

System Info

System:
    OS: macOS 12.0.1
    CPU: (10) arm64 Apple M1 Max
    Memory: 79.22 MB / 32.00 GB
    Shell: 5.8 - /bin/zsh
  Binaries:
    Node: 16.13.1 - ~/.nvm/versions/node/v16.13.1/bin/node
    npm: 8.1.2 - ~/.nvm/versions/node/v16.13.1/bin/npm
  Browsers:
    Chrome: 98.0.4758.80
    Firefox: 96.0.3
    Safari: 15.1
  npmPackages:
    @sveltejs/adapter-auto: workspace:* => 1.0.0-next.18 
    @sveltejs/kit: workspace:* => 1.0.0-next.260 
    @sveltejs/site-kit: ^2.0.2 => 2.0.2 
    svelte: ^3.43.0 => 3.44.2

Severity

serious, but I can work around it

Additional Information

No response

@Rich-Harris Rich-Harris added the bug Something isn't working label Feb 7, 2022
@Rich-Harris Rich-Harris mentioned this issue Feb 7, 2022
30 tasks
@bluwy
Copy link
Member

bluwy commented Feb 8, 2022

Maybe related #3636

@jasonlyu123
Copy link
Member

jasonlyu123 commented Feb 10, 2022

Still believe it's related to the problem I mentioned in #3105. And I don't think it's covered together with the scrolling.

As for what we can manage focus. We can't just focus() the target element as it might not be focusable. And it's not the same behaviour as traditional hash links in modern browsers. There's a Sequential Focus Navigation Start Point concept. I think this blog from google explains it quite well.

Unfortunately, there seems to be no way to programmatically set the Sequential Focus Navigation Start Point. These are the possible workaround I can think of:

  1. Make the target focusable, focus it and revert it. the downside is it would cause the focus ring to show up. And it's not the same as the normal hash link behaviour. But it doesn't create any dummy element.
  2. Append a dummy element before the target element if the target element isn't focusable. focus it and remove it.
  3. Append a <a href="#target" ></a> tag to the document body and call the click() method. This would probably be the most consistent with a traditional hash link. But we might need to prevent event propagation on it?

@Rich-Harris Rich-Harris added this to the 1.0 milestone Mar 5, 2022
@Rich-Harris
Copy link
Member Author

I came here to post an article about SFNSP I just stumbled upon — https://hiddedevries.nl/en/blog/2017-04-24-where-focus-goes-when-following-in-page-links — but it basically just reiterates what @jasonlyu123 posted.

All three strategies seem viable:

  • Demo 1. The one caveat I can think of is that if for some reason the target element had a focus handler, it would be activated incorrectly.
  • Demo 2.
  • Demo 3. I don't think we need to worry about navigation handlers refiring since the click would navigate to the current link, and the router would discard the navigation, but we would want to check thoroughly

@mrkishi
Copy link
Member

mrkishi commented Mar 11, 2022

Just spitballing, but would it be possible for us to manually set the hash after navigation? Something like:

  • User clicks href="/route#target-hash", so we capture the link and route
  • Instead of history.pushState(state, '', '/route#target-hash') we do history.pushState(state, '', '/route')
  • Render route
  • hash_navigating = true; location.replace('#target-hash'); (not actually hash_navigating but something similar)

@mrkishi
Copy link
Member

mrkishi commented Mar 12, 2022

Apparently even a simple location.replace('#'); location.replace('#component-format-script-context-module'); instead of focus() works on desktop Chromium and Firefox, but tests on other browsers are needed. Are there known downsides to this?

@jasonlyu123
Copy link
Member

Yeah. seems like only calling location.replace without deferring the hash navigation would also work. Look at the spec, It seems like it's mostly going to be the same behaviour as link navigation. location.hash setter won't work though. It's explicitly mentioned in the spec that it should return early. Maybe that's why I thought the location API won't work before.

I found some downside with navigation approaches. The history entry would be replaced. So the new history entry won't have a PopStateEvent.state. The navigation will be aborted. We can probably call history.replaceState again. Any downside with this?

If this turns out to work in safari and we can solve the history problems. It seems like a good approach to do.

@benmccann benmccann added the p2-nice-to-have SvelteKit cannot be used by a small number of people, quality of life improvements, etc. label Mar 17, 2022
@benmccann benmccann added help wanted PRs welcomed. The implementation details are unlikely to cause debate and removed help wanted labels Apr 4, 2022
@whataboutpereira
Copy link
Contributor

location.replace will also make CSS :target work after navigation. Currently :target doesn't work when navigated to a new route with a hash.

@Rich-Harris Rich-Harris modified the milestones: 1.0, whenever Jul 20, 2022
@Bruce-Hopkins
Copy link

Is this issue open for contributors? Can I open a PR for this?

@mefaba
Copy link

mefaba commented Oct 29, 2022

The reproduction link is outdated, todos section of it is throwing error. @Rich-Harris , is this bug still exists?

@hrueger
Copy link

hrueger commented Feb 5, 2023

For anyone interested, this works for me as a temporary workaround:

onMount(() => {
    const hash = window.location.hash.slice(1);
    if (hash) {
        window.location.hash = hash;
    }
});

Edit: apparently that's not how the spec says it behaves, however, it works fine for me.

@whataboutpereira
Copy link
Contributor

location.replace will also make CSS :target work after navigation. Currently :target doesn't work when navigated to a new route with a hash.

I noticed there is data-sveltekit-replacestate now, but :target still doesn't work when using it.

Previous code:

const state = history.state;
location.replace(navigation.to.url);
history.replaceState(state, '', location.href);

Using data-sveltekit-replacestate I still need location.replace(navigation.to.url) for :target to work.

@whataboutpereira
Copy link
Contributor

location.replace will also make CSS :target work after navigation. Currently :target doesn't work when navigated to a new route with a hash.

I noticed there is data-sveltekit-replacestate now, but :target still doesn't work when using it.

Previous code:

const state = history.state;
location.replace(navigation.to.url);
history.replaceState(state, '', location.href);

Using data-sveltekit-replacestate I still need location.replace(navigation.to.url) for :target to work.

After more testing it looks like there's no need for neither data-sveltekit-replacestate nor history.replaceState(state, '', location.href);. Just location.replace(navigation.to.url); suffices to make :target work.

@whataboutpereira
Copy link
Contributor

After more testing it looks like there's no need for neither data-sveltekit-replacestate nor history.replaceState(state, '', location.href);. Just location.replace(navigation.to.url); suffices to make :target work.

Alas, Firefox still needs manual replaceState(). Chrome seemed fine without.

@eltigerchino
Copy link
Member

eltigerchino commented Oct 11, 2023

  1. Append a tag to the document body and call the click() method. This would probably be the most consistent with a traditional hash link. But we might need to prevent event propagation on it?

This doesn't seem to work on macOS Safari. Might be because of Safari's behaviour of waiting for the actual user's first click and not recognising programmatic clicks. Unless we leave Safari out, this option does pretty well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted PRs welcomed. The implementation details are unlikely to cause debate p2-nice-to-have SvelteKit cannot be used by a small number of people, quality of life improvements, etc.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants