mefechoel / svelte-navigator

Simple, accessible routing for Svelte
Other
504 stars 39 forks source link

Focusing `h1` on route breaks in page / fragment links #3

Closed hidde closed 4 years ago

hidde commented 4 years ago

Describe the bug

Upon route, focus is placed on the h1. This is good. But when my route contains a hash in its path, that breaks browser default behavior, which would be to scroll to relevant element and move the sequential focus starting point.

To Reproduce

Steps to reproduce the behavior:

  1. Follow a link with a fragment identifier

Expected behavior

Focus of heading behavior is skipped, because the hash specifies a more specific location.

Actual behavior

Heading is focused.

Screenshots

Desktop (please complete the following information):

n/a

Smartphone (please complete the following information):

n/a

Additional context

mefechoel commented 4 years ago

Yes, in that case we would need to prevent focusing of the h1 element.

I also noticed while checking the bug, that a Link with a to="#someHash" treats #someHash as a path segment, rather than a fragment. It will redirect to /currentRoute/#someHash, although it should be /currentPath#someHash.

It should a rather easy fix though.

hidde commented 4 years ago

I also noticed while checking the bug, that a Link with a to="#someHash" treats #someHash as a path segment, rather than a fragment. It will redirect to /currentRoute/#someHash, although it should be /currentPath#someHash.

Ah, good find, yes that would be unexpected too!

It should a rather easy fix though.

Yay! I have had a look if I could submit something, but was not sure how to do it sensibly… many thanks for taking a look!

mefechoel commented 4 years ago

I've already had a look and just disabling focus management on a navigation with a fragment seems to work well.

I am now wondering if the screen reader announcement should be disabled as well.

Also, the default browser behaviour seems to be to focus the body element after an in page jump. Since this is the standard behaviour it should work with screen readers, but it seems more intuitive to focus the jump mark to me.

hidde commented 4 years ago

What is the best way to disable focus management on a specific navigation? I've been able to do it on a Route component with primary={false}, but sometimes that route is accessed without a fragment identifier. In those cases I would want focus management.

I am now wondering if the screen reader announcement should be disabled as well.

Good question, I'd say it depends on whether you go to a fragment in the same page (probably don't announce), or in another page (probably do announce).

To give a practical example, on this page I have a bunch of links to other pages with fragment (the edit links in the table). They navigate to a different page, and on that page, the fragment. Announcing the name of that page would make sense to me.

Also, the default browser behaviour seems to be to focus the body element after an in page jump. Since this is the standard behaviour it should work with screen readers, but it seems more intuitive to focus the jump mark to me.

As far as I understand it does blur the element that had focus, so body registers as the activeElement, but the “sequential focus navigation starting point” moves, so when a user would press tab, the next focus element would not be the first in the body, but the first in / after (?) the identified fragment. I believe that also means that part of the page would get read out in browse mode, but I should test.

mefechoel commented 4 years ago

The way it is implemented now, you cannot disable focus management on a specific navigation. A navigation can be caused by multiple interactions, such as Links or navigate, but also by hitting the browsers back button. So we don't always know where a navigation was caused and therefore cannot disable focus management on a per navigation basis. Instead when location changes, all Routes that match that location register with the Router, which then picks the most specific match and focuses it.

In your case you should not need to manually disable focus management when using a fragment navigation. That should be the Routers concern.

Good question, I'd say it depends on whether you go to a fragment in the same page (probably don't announce), or in another page (probably do announce).

Yes, that seems to be the way to go!

As far as I understand it does blur the element that had focus, so body registers as the activeElement, but the “sequential focus navigation starting point” moves, so when a user would press tab, the next focus element would not be the first in the body, but the first in / after (?) the identified fragment.

You're probably right. I will test it as well.

Thanks a lot for the feedback! I'll probably get to working on a fix later today.

mefechoel commented 4 years ago

Ok, I've investigated the issue further and there is unfortunately more to it than anticipated.

First of all, I fixed the incorrect focusing, that was the core issue. So when clicking an <a href="#hash" /> the page will indeed jump to the specified element and skip focusing any headers or other focus targets. It will also deal with screen reader announcements, as discussed above.

@hidde I can publish a patch with the fix now, if that helps. There is however more to be done to completly resolve this issue.

As far as I understand it does blur the element that had focus, so body registers as the activeElement, but the “sequential focus navigation starting point” moves, so when a user would press tab, the next focus element would not be the first in the body, but the first in / after (?) the identified fragment.

It turns out that Chrome and Edge behave exactly as you described, IE however will actually reset focus to the html body, so you'd need to tab through the whole page anyways, and the in-page jump does not really help you. Firefox does it in yet another way, in that it works as expected, when the element, that is skipped to comes before the next focusable element. When the next focusable element is inside the linked element (e.g. the main element), Firefox skips the first focusable element and immediately goes to the second focusable element. This seems like a really weird bug in Firefox to me, but I didn't find any information on it...

I think it's enough to recommend putting the jump mark before the next focusable elements somewhere in the README, with a note, that for IE support, the developer will need to do some extra work.

The bigger problem is, that when navigating to a Route, that is not already rendered, the browser will check for a jump mark immediately, but of course rendering the Route takes some time, so the browser won't find the element and won't scroll the page. I think it's best to tackle this issue in a new ticket, because there is some more work to be done there.

I think there are two possible ways to solve this. Either, when calling navigate, e.g. by clicking a Link, we need to update the Routers internal location first, wait for the Routes to render, and then call history.pushState. But there is probably a lot of tricky work to be done. The second possibility is to assign window.location.href back to itself, after all Routes have rendered. This however triggers a popstate event in some browsers, which the Router will interpret as a new navigation, so we need to make sure, not to cause infinite loops by accident.

hidde commented 4 years ago

Thanks so much for investigating this!

@hidde I can publish a patch with the fix now, if that helps. There is however more to be done to completely resolve this issue.

This would be super helpful, if you'd like to do that, thanks!

This seems like a really weird bug in Firefox to me, but I didn't find any information on it...

Hm, that's odd! I'm sure the folks in Bugzilla would be happy if you could file it, they might not be aware.

The bigger problem is, that when navigating to a Route, that is not already rendered, the browser will check for a jump mark immediately, but of course rendering the Route takes some time, so the browser won't find the element and won't scroll the page.

Yes!

Incidentally, in my app, I've do location.hash = location.hash in the onMount hook of the route so that the :target in CSS gets evaluated as expected. I started doing this based on this comment on Twitter which may be helpful to this problem, too?

mefechoel commented 4 years ago

Alright, I will release a patch soon, and add a note to the README.

I'll have a look if there already is a bug report in Bugzilla and report it otherwise 👍

Incidentally, in my app, I've do location.hash = location.hash in the onMount hook of the route so that the :target in CSS gets evaluated as expected. I started doing this based on [this comment on Twitter] which may be helpful to this problem, too?

Great thanks! That's probably better than re-asigning location.href. Could you please provide the twitter link again? It seems to be missing...

hidde commented 4 years ago

Thanks, and woops, just added link.

mefechoel commented 4 years ago

I just released 3.0.8 with the patch.

Progress on the discussed related issues will be tracked in #5.

mefechoel commented 4 years ago

This seems like a really weird bug in Firefox to me, but I didn't find any information on it...

I could not reproduce the bug using plain html, so the bug actually does not seem to be in Firefox, but somewhere in Svelte Navigator (or maybe Svelte?). I'll need to look into this more, probably also in #5.