CSCfi / rems

Resource Entitlement Management System is a tool for managing access rights to resources, such as research datasets.
MIT License
51 stars 22 forks source link

Fix focus in create resource #2367

Closed Macroz closed 3 years ago

Macroz commented 3 years ago

When choosing Administration, then Resources and finally Create, the focus should be in the first field but it seems to disappear.

BadAlgorithm commented 3 years ago

Hi @Macroz I'm happy to tackle this one.

I've had a quick look at the codebase and can see where the focus is going. Did you have a specific solution in mind or should I just try open a PR and get your thoughts?

Macroz commented 3 years ago

Great! No specific solution but I urge you to have a look at what we have done elsewhere by looking for focus in the code. Please, ask us if you have any questions!

BadAlgorithm commented 3 years ago

Hi @Macroz,

I've got a few questions about this issue, it seems to be a little trickier than I originally anticipated.

I've identified a few problems which makes this a little challenging.

Firstly, I noticed that there is a root on-update function that calls focus on the #main-content element. I did remove this since it would often steal focus from the dropdowns. However, I want to ask why was this being focused by default? I'm hoping this wouldn't break existing functionality. My current solution is to extend the dropdown component to take in an auto-focus? flag that will forward to the Select component. This works, but I would've loved to be able to use the centralised on-update function.

My initial solution was to add custom focus classes to each of the first children in the forms and query for that class (via document.getElementsBySelector). This solution was nice since it allowed me to use the centralised on-update function and implement it in a backwards-compatible way. However it seems like the lifecycle methods don't allow me to do this. It would call the update function before the children had a chance to mount.

What are your thoughts?

Macroz commented 3 years ago

Good questions and thinking.

I think the idea was to move the focus initially to a relevant element. However, it is not working correctly. Typically forms would/should(?) focus the first input element, and obviously here it didn't work so. I think for pages where there are no inputs, the idea was to focus something else instead. However I know that there should be the "skip to main content link" that you can navigate to backwards, so I wonder why the autojump is there.

These are all related to accessibility requirements. I'll dig around for the answer a bit. I couldn't find anything quickly that would specify how the initial focus should be set. Only that e.g. navigation and focus must be logical. The logic can really only be tested with NVDA on Windows or such setup and I can do that out once we have some solution to try.

I'll get back to you!

Macroz commented 3 years ago

I had a look at some accessibility documents and I think that although it is not exactly specified, the best practice seems to be to focus the <h1>. So the task should rather be to make sure the focus is in the first <h1> and that pressing tab should take the user to the first input field. I just tested and it doesn't do that so maybe we can fix it to be like this instead?

I think all the editors in the admin pages have the exact same problem that the focus is not in the <h1> after pressing the create buttons. What do you think?

Macroz commented 3 years ago

Anything we can help you with @BadAlgorithm?

BadAlgorithm commented 3 years ago

Hey @Macroz Sorry, I've been out of action for a couple of weeks.

I think the solution that you've proposed works and I've just tried it out. Seems to work well and will navigate to the correct input fields in the forms by default (as far as my testing has seen). One thing that I did notice in my testing is that it does not capture focus when the page is refreshed, is this something we would like to support? If this is the case a solution I can think of is to change the lifecycle on when we call the function to focus the h1 element (since it's not mounted when main app is mounted).

What are your thoughts?

I can refer you to my fork if you wanted more information

Macroz commented 3 years ago

Hopefully nothing serious!

Sounds good. I think perhaps the first PR should just fix the focus when navigating and then if you have the chance a second PR could solve the page refresh. I'm happy to review a draft PR too if you want early comments or specific help but sounds like you have the <h1> focus part covered.

Macroz commented 3 years ago

@BadAlgorithm do you want to continue on this issue or shall we finish it?

marharyta commented 3 years ago

https://developers.google.com/web/fundamentals/accessibility/focus May I ask about H1 focus? What was the resource that points out it should be focused? https://developers.google.com/web/fundamentals/accessibility/focus/using-tabindex Another clarification would be why do we want to remove tabindex in the focus function? I mean, could we just add tabindex="0" to h1?

To continue the discussion, I do not see explicitly :focus state being styled / triggered with re-frisk.

opqdonut commented 3 years ago

This is my understanding: we use tabindex=-1 to make the element focusable from JS with .focus(), but not focusable with tabbing. This way, we don't break keyboard navigation (tab should take you to the first input element), but we can still help screen reader users by focusing the start of content from JS.

marharyta commented 3 years ago

right, thanks for clarification @opqdonut

marharyta commented 3 years ago

I assume my surrent solution with https://github.com/CSCfi/rems/issues/2367 does not work for you then?

opqdonut commented 3 years ago

Yeah at least it's not in line with the current approach we have. We can have a discussion about this focus stuff with the team and figure out a new approach of course if there are problems with the old one.

marharyta commented 3 years ago

well, the way I see it it currently had a focus on h1 element according to previous implementation, so adding focus on the first field in form is not relevant anymore?

opqdonut commented 3 years ago

I've not kept up with the developments in this ticket, but this comment from @Macroz above seems to be the current spec:

So the task should rather be to make sure the focus is in the first <h1> and that pressing tab should take the user to the first input field.

marharyta commented 3 years ago

ok, so I understood the task correctly then

opqdonut commented 3 years ago

I just checked all the Create catalogue item / resource / form / workflow / organization pages and they all work correctly:

So I think this issue is done, or am I missing something @Macroz ?

Macroz commented 3 years ago

Maybe just the idea task for the page refresh behavior (that should be the same).