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

sets main-content id outside of main element #131

Merged
merged 2 commits into from
Sep 10, 2021

Conversation

markconroy
Copy link
Member

PR for #130

@danchamp
Copy link
Contributor

danchamp commented Sep 8, 2021

@markconroy I think we want to keep the id on the main element, since it's the landmark that we want to be the target of the skip link.

The issue is the focus outline that Safari displays, so it may be a better approach to remove that in CSS?

Also paging @msayoung for a sense-check.

@markconroy
Copy link
Member Author

@danchamp

Does it need to be the landmark? Drupal core does not link to a landmark, it links to a a tag, exactly like I have done. You can see it in the bartik theme's /templates/page.html.twig

@danchamp
Copy link
Contributor

danchamp commented Sep 8, 2021

The screen reader experience in Safari seems to be much better with the main element as the target. On VoiceOver, the target gets announced as:

Main, you are currently on a heading level one.

With the empty anchor element:

You are currently on a link, inside web content

GovUK and NHS use the main element as the target, but interestingly don't have the tabindex set for the element.

Going to dig further...

Hmm, I understood that without the tabindex there was an issue with the element reliably receiving focus, but that doesn't appear to be the case. GovUK removed it some time ago:

alphagov/govuk_elements#534

And have found this article which confirms there's no longer a need for that negative tabindex:

https://knowbility.org/blog/2019/skip-links

So, I think we can fix this by just removing tabindex="-1" from main, and leaving it as the target of the skip link.

@markconroy
Copy link
Member Author

Cool stuff. I'll get that fixed later so. Thanks Dan.

@markconroy
Copy link
Member Author

Hi @danchamp

This is ready again now. Thanks.

@danchamp danchamp merged commit e24f53e into 1.x Sep 10, 2021
jsnmrs pushed a commit to jsnmrs/jasonmorris that referenced this pull request Apr 24, 2023
jsnmrs pushed a commit to jsnmrs/jasonmorris that referenced this pull request Apr 24, 2023
* Update bio format

* Adjust 404 output

* Removing `tabindex="-1"` from `<main>`

From localgovdrupal/localgov_base#131 (comment) and alphagov/govuk_elements#534

* More readable HTML

* concise for better fit
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.

2 participants