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

Fix focus in create resource #2367

Closed
2 tasks
Macroz opened this issue Sep 29, 2020 · 19 comments
Closed
2 tasks

Fix focus in create resource #2367

Macroz opened this issue Sep 29, 2020 · 19 comments

Comments

@Macroz
Copy link
Collaborator

Macroz commented Sep 29, 2020

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

  • fix focus to go to the first field
  • bonus: check the other create navigations too
@BadAlgorithm
Copy link
Contributor

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
Copy link
Collaborator Author

Macroz commented Oct 1, 2020

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
Copy link
Contributor

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
Copy link
Collaborator Author

Macroz commented Oct 6, 2020

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
Copy link
Collaborator Author

Macroz commented Oct 7, 2020

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
Copy link
Collaborator Author

Macroz commented Oct 23, 2020

Anything we can help you with @BadAlgorithm?

@BadAlgorithm
Copy link
Contributor

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
Copy link
Collaborator Author

Macroz commented Oct 25, 2020

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
Copy link
Collaborator Author

Macroz commented Nov 19, 2020

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

@marharyta
Copy link
Contributor

marharyta commented Dec 9, 2020

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
Copy link
Contributor

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
Copy link
Contributor

right, thanks for clarification @opqdonut

@marharyta
Copy link
Contributor

I assume my surrent solution with #2367 does not work for you then?

@opqdonut
Copy link
Contributor

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
Copy link
Contributor

marharyta commented Dec 10, 2020

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
Copy link
Contributor

opqdonut commented Dec 10, 2020

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
Copy link
Contributor

ok, so I understood the task correctly then

@opqdonut
Copy link
Contributor

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

  • Focus is initially in the h1
  • Tab moves to first input field

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

@Macroz
Copy link
Collaborator Author

Macroz commented Dec 14, 2020

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

@Macroz Macroz closed this as completed Dec 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

No branches or pull requests

4 participants