"Skip to main content" should link to main tag or similar

This was added in 2009, in #386462: Skip Navigation' should be in all core themes
AFAIK that was before the MAIN tag was in the official HTML spec, and using an A tag was the recommended approach.

That has changed since then, and an A tag is no longer preferred: http://webaim.org/techniques/skipnav/

See https://www.w3.org/TR/WCAG20/#navigation-mechanisms-skip for the official spec.

This extra a tag that's missing an alt and href attribute isn't needed and should be removed really.

The alt attribute doesn't belong to <a> elements, only <area>, <img> and some <input> types.

The href attribute isn't mandatory for an <a> element when we use it in this way.

I have no objection to the proposed change (in principal) but it must have manual testing in all the browsers we support to avoid a regression for keyboard operation.

This should be fixed in the system module's copy of the page template first.

Tagging for Stable and Classy maintainer to consider. We avoid touching that markup unless there's a bug that really needs to be fixed. I'm not sure this qualifies, but @cottser can say otherwise.

The patch in #6 removes a documentation comment. I think we ought to keep that - it describes a relationship between two templates that isn't immediately obvious (there's a corresponding comment in html.html.twig).

@James: thanks for the patch! Can you make the change to the copy of page.html.twig in the system module instead? We should wait to see what the maintainers of stable + classy say about this, too.

We're not proposing a new class, removing class="l-content" from the issue summary.

No need to remain assigned while it needs review.

Motivation is good, reasoning is good. Improvement is good. Removing Anorexic Anchors and linking to the *full* element being referred to (and not just a point at the top of it) is good.
Code is fine.

As the ID is being retained, and (AFAIK) all known browsers support the ID as a target, I can't see or imagine any regression issues.
I've run it through my memory of accessibility reviews and tests, including the keyboard shortcuts we expect to work and haven't been able to identify any possible issues.

The only medium UI change is that for text-to-speech-accessibility tools that highlight the current element that has been switched to, the highlight (eg, a thick orange box) flashes around the whole 'main' area ... instead of a little blob of orange before the heading. Overall, that's a change, but an improvement.

When you check your Drupal website using the Wave toolbar this also generates an error because the anchor is an 'empty link'.

The suggested patch also fix this wave toolbar error.

Changed to Reviewed & tested as it appears to have been by dman and DomoSpiens.

Thanks for the review and testing in #17 and #18. Sounds like the manual testing is covered and this does indeed resolve an accessibility bug.

The issue is still tagged "needs accessibility review" -- let's have one of the accessibility topic maintainers sign off for it to be RTBC. I've also left @Cottser a message to take a look at the patch. In terms of BC policy, I think it is okay for 8.3.x since it is making a change to a module template, which is allowed to change in minors, without changing one of the stable base themes.

Patch looks good, can be committed once we have approval from an accessibility topic maintainer.

I think it's too risky to change Stable or Classy templates for this.

@cottser: ah yes, thanks for the reminder. I tagged this back in comment #5.

I am very close to signing off on this!

I've already done manual testing with LOTS of desktop OS/browsers (+ screen reader) combinations, before and after the patch. Some interesting differences, but none so far that would be a problem for signing off on this.

The reason I want to satisfy manual testing is that the skip-link is probably the single most important accommodation we have for sighted keyboard users (and by extension anyone whose assistive tech is set up to emulate a keyboard).

However, I still want to test iOS behaviour with a keyboard. I need to go to badger an iPhone user...

Patch applied cleanly and thus doesn't need reroll.

root@masky-VirtualBox:/home/drupal/cap/drupal# git apply -v  skip_to_main_content-2784311-12.patch 
Checking patch core/modules/system/templates/page.html.twig...
Applied patch core/modules/system/templates/page.html.twig cleanly.
root@masky-VirtualBox:/home/drupal/cap/drupal# git branch
* 8.3.x

Hmm.. It failed when I tried it a week ago. Maybe it was a glitch on my end. Thanks!

I re-queued the patch for 8.4.x. It passed. I then manually applied the patch to an install of Drupal-8.4.x with PHP 7.1 and MySQL 5.7.17. The patch installed and appeared to do what it was supposed to do. I don't have the facility or knowledge to test against accessibility.

I did some testing with this already, with a number of browers, with and with out a screenreader. I'll post the results in the next few comments.

Screenreaders still needing tests:

• Jaws
• iOS Voiceover
• Android Talkback

There was no appreciable difference before + after the patch withe the following set-ups:

• Internet Explorer 9, Windows 7
• Internet Explorer 9 + NVDA screen reader v2016.3, Windows 7
• Firefox 49, Linux
• Firefox 49, macOs Sierra
• Firefox 49, Windows 7
• Firefox 50, Windows 7

Since this patch only changes the CSS in the System module, testing should be done against with the Stark theme.
Not sure if that was obvious in the earlier comments.

The following set-ups had a minor difference, which is acceptable IMO. No screen reader was used here.

• Chrome 54.0 Linux
• Chrome 54.0 Windows 7
• Chrome 54.0 macOS 10.12 Sierra
• Chromium 53.0 Linux
• Chromium 56.0 macOS 10.12 Sierra
• Opera 41.0 Linux
• Opera 41.0 Windows 7
• Opera 41 macOS 10.12 Sierra
• Safari 10 macOS 10.12 Sierra

BEFORE the patch, the focus went to the empty <a>and no outline appeared.

AFTER the patch, focus outline appears on the entire <main> element (faint grey on Chrome/linux and Opera/linux, clearer blue on Chromium/linux and Safari, Opera/mac, Chrome/mac, Chromium/mac)

This is presumably the default Blink browser engine behaviour. We could try to override it, but I don't think it's worth the effort.

I discovered a BIG difference with these set-ups, all the same beheviour.

• Chrome 54, VoiceOver, macOS 10.12 Sierra
• Chromium 56, VoiceOver, macOS 10.12 Sierra
• Opera 41, VoiceOver, macOS 10.12 Sierra

BEFORE the patch:

• Follow the skip link, VoiceOver announces "Main", but does not read the heading.
• The landmarks menu in VoiceOver includes a landmark called "main" (with no other label).
• Choosing the main landmark, VoiceOver announces the level-one heading (i.e. the first speakable element inside <main>).

AFTER the patch, a big difference:

• The main landmark seems to be labelled by the entire content of the <main> element - quite a difference!
• In VoiceOver's landmark list, the main landmark has this insanely long label.
• When following the skip-to-main-content link, this "label" is read out (yes, the entire content of <main>). It's obviously mitigated by the fact that you can press the Control key to make VoiceOver stop talking.

Given that Chrome, Chromium, and Opera all exhibit the same behaviour, and Safari doesn't, I'm going to assume this is down to a difference between what the Blink and WebKit engines send to VoiceOver via the Mac accessibility API. Drupal isn't giving an explicit label to the main landmark, so I'm going to put this down to bad behaviour on Blink's part, and say it's not a blocker for this issue. (We might want to mitigate this by giving our main landmark an explicit ARIA label, but that would be a separate (controversial) issue for later.

Since Safari + VoiceOver + macOS behaves nicely (the default applications for Macs), and none of these Blink browsers appear in combination with VoiceOver on the most recent WebAIM Screen Reader User Survey Results, then I reckon we don't treat this as a blocker for this issue.

Android 6.x + Chrome + Talkback (latest versions, I think...) and a bluetooth keyboard.

No appreciable difference before and after the patch when using Talkback.

When using Android 6 + Chrome, without talkback, the same minor differnce as desktop browsers in #33. An outline is seen around the <main> after using the skip link.

I'm happy with this.

@andrewmacpherson sounds like you are ready to mark this RTBC, right?

iOS v10.2 + Safari tests and a bluetooth keyboard.
OK - no appreciable difference before/after the patch, with and without voiceover.

@mgifford - I'm happy with this so far. The only significant gaps in my manual testing have been with:

• JAWS, with IE, Firefox or others browsers (I can't get the JAWS demo version to work inside a Windows VM. Meh. I should get a small hard drive for dual booting...)
• Windows 10/Edge/Narrator (which I haven't become familiar with yet)

Do you have a JAWS set-up available?

I'm tempted to just say RTBC for this anyway. <main id="main-content"> as a skip-link target is a widespread enough in the wild...

I am happy to RTBC it at this point. As Andrew noted, this is a pretty common process at this time.

I don't have JAWS set up, but we don't test all accessibility patches with JAWS unfortunately. It would be better if we did.

If folks do want to test it again, I would remind folks that the theme needs to be set to Stark to see the changes.

We should create a follow-up issue for Stable and Classy maintainers that is either in the 8.4 or 9.x branch. @Cottser what is your view on where the follow-up issues to this should be?

Maybe this can be a followup, but what about the following in Bartik and Seven templates?

core/themes/bartik/templates/maintenance-page.html.twig:            <a id="main-content"></a>
core/themes/bartik/templates/page.html.twig:        <main id="content" class="column main-content" role="main">
core/themes/bartik/templates/page.html.twig:            <a id="main-content" tabindex="-1"></a>
core/themes/seven/templates/page.html.twig:      <div class="visually-hidden"><a id="main-content" tabindex="-1"></a></div>

As @Cottser said I think we probably can't change this in Stable or Classy in 8.x.

Earlier on, I imagined the Bartik and Seven change would be follow-up, depending on whether it was eligible for Stable/Classy. However since we heard from @cottser in #21, we may as well do the Seven and Bartik changes here.

@xjm what do you think of @Cottser's concerns about it being too risky to change at this time?

@mgifford, @Cottser said "I think it's too risky to change Stable or Classy templates for this." As far as I understand he is referring only to those, not to System module nor Bartik and Seven. Module and non-base theme templates can change in a minor release. This is covered by our BC policy: https://www.drupal.org/core/d8-bc-policy#themes

So as far as I know this patch is fine for 8.4.x, as would be one for Bartik and Seven.

Here's some code with the other themes. I haven't tested it yet, but putting it up for the bots.

@mgifford, Cottser's comment in #21 says not to change classy but the patch contains changes to a classy file. Is this intended?

I misread what @xjm wrote & yes Classy shouldn't be included as "non-base theme templates can change in a minor release." So Bartik and Seven is fine but Stable & Classy aren't.

Easy fix though as the bots liked it.

Re-roll without Classy.

#48 Looks good to me. Will RTBC once the test is green.

RTBC for #48

The scope in #48 looks good to me.

@Cottser said:

Patch looks good, can be committed once we have approval from an accessibility topic maintainer.

That sounds like a frontend framework manager signoff to me, and we also have the requested accessibility signoff, including very thorough accessibility testing.

Now is a great time to commit this to 8.4.x, because that way there is plenty of time for it to be tested.

+++ b/core/themes/bartik/templates/maintenance-page.html.twig
@@ -32,9 +32,8 @@
-        <main id="content" class="column" role="main">
-          <section class="section">
-            <a id="main-content"></a>
+        <main id="content" class="column" role="main" tabindex="-1">
+          <section class="section" id="main-content">

+++ b/core/themes/bartik/templates/page.html.twig
@@ -76,9 +76,8 @@
-        <main id="content" class="column main-content" role="main">
-          <section class="section">
-            <a id="main-content" tabindex="-1"></a>
+        <main id="content" class="column main-content" role="main" tabindex="-1">
+          <section class="section" id="main-content">

In these two, the <section> is marked as the main-content rather than the <main>. That seems correct to me, since it retains the hierarchy. I also checked that (as far as I can tell) there is no CSS that will be affected by these changes.

Finally, I manually tested Bartik, Seven, and the Bartik maintenance page. In each case, the "Skip to main content" link properly took me to the main content, and nothing visually looked broken to my untrained eye.

Thanks so much for the careful review of this change! Committed to 8.4.x.

Oh, one more thing. I guess we should have a followup issue to update the Stable and Classy templates in 9.x, but I have no idea how we will handle deprecations for the frontend framework, and I imagine there will be many changes that we made internally that should go into the base themes for 9.x, not just this small fix.

For now, let's file a followup for 9.x for this issue and then we can split a broader discussion off of that if appropriate. Thanks!

Does anyone like the outline around main tag?

oops.. sorry for re-testing that...

@Chi, can you clarify what you mean or post a before/after screenshot illustrating it? Is this something I missed in my manual testing, and does the issue need to be reverted so we can discuss it?

I think I see what @Chi might be referring to. When the <main> receives focus, in some browsers, it becomes surrounded with a border (not a CSS border; one added by the client). When it was an empty <a> tag, no such border would have been visible. Is this what you meant?

To me, that seems correct, and also more accessible for users of a graphical browser anyway. I ask to "skip to main content" and the browser's default behavior highlights for me where that content is, which helps me understand the information on the page. I don't think we should try to circumvent the client's behavior there.

If there is a different border that is caused by the template changes in this patch, one on the frontend rather than client-side, let's post a screenshot. If it also exists in HEAD, then a followup issue is probably better. Thanks!

@xjm, right, the question was whether that outline looks good for you.
It's not a big deal to disable it in a custom theme but for core and contributed themes you need to find a way to inject the CSS code.

main {
    outline: none;
}

One more thing I noticed, when clicking a checbox or radio button inside main area the outline is blinking.

@Chi I can replicate the focus in Seven. Basically any of the admin pages I tested, such as: /admin/config/people/accounts

I can see the outline in Chrome but not Firefox (just testing on a Mac).

I'm not sure this wouldn't be see as a feature rather than a bug. It's definitely different, but much easier to see where your focus is.

If we're going to address the inconsistency between browsers (possibly with main { outline: none; }) then I suggest we do this in a new issue.

Re: #54 The outline around the #main-content element was noticed in #33.

It doesn't appear in all browsers; it seems to be just Webkit + Blink browsers. Firefox and IE don't show it. I have a suspicion it may be down to the fact we also have a tabindex="-1", which was put in place to fix #1529814: Fix skiplink behavior for Webkit browsers.

Normally, page-internal links to an ID #fragment would NOT produce a visible focus if the ID was on an element which isn't operable (e.g. the table-of-contents links on Wikipedia pages, which point to heading elements). Visible focus would normally only be indicated for interactive elements (links, buttons, etc.).

If we wanted to suppress it, we could try a CSS rule like #main-content[tabindex="-1"] { outline: transparent; }. If so, we could do that as follow-up - I don't think it warrants reverting this commit.

Firefox and IE don't show it.

Firefox does show it with different outline color, at least on my Linux installation (FF 42.0).

The style comes from Blink (and Webkit, presumably) browsers which have a user agent style like this:

:focus {
    outline: -webkit-focus-ring-color auto 5px;
}

This is present in Chrome 56, Chromium 55, Opera 43.0, and Vivaldi 1.7 (Linux versions). I don't have a copy of Safari available right now.

I noticed something else: it doesn't affect Bartik.

It turns out that Bartik's tabindex=-1 and id="main-content" are on separate elements. See commit ed82b95. This goes some way to confirming my hunch that it is down to the tabindex fix for webkit browsers.

Given that the #main-content element itself is not intended to be an operable element, and other page-internal fragment links (e.g. links to heading elements) do not gain a visible outline, I'm inclined to suppress the focus outline we are seeing here.

it doesn't affect Bartik.

@andrewmacpherson, it does affect for me.

Wrong screenshot.

Re: #65 - Thanks @Chi!

I recently closed #2852693: 508 Testing Issues - Skip to main content loses focus, works as designed. For consistency I think we ought to suppress the outline around <main> which was introduced here.

@xjm - since this is only in 8.4.x, and we aren't targeting it for 8..3.x backport, there's plenty of time to address the focus outline in a follow-up. Hopefully we don't need to revert this issue's commit?

@andrewnacpherson Well, if we do consider the outlines a regression, we should revert it. The template markup changes are an improvement, whereas (based on your information) the borders are a small accessibility and visual regression (if they are miscommunicating that the element is interoperable when it's not).

Fortunately, same as we would have plenty of time to do followups, we also have plenty of time to add that fix to this patch if needed as well.

Edit: Less "commenting from an iPhone" run-on.

Alright, I reverted this for the reasons above. If we agree that the focus border is not correct, then let's add the needed CSS along with the template changes. I think we'll probably also want a frontend framework manager's review before we recommit with that.

Thanks everyone!

I forgot this was still open.

There's another approach we could take. Instead of removing <a id="main-content" tabindex="-1"></span> tag and putting the ID directly on <main>, we could replace it with <span id="main-content" tabindex="-1"></span>.

I expect this would solve the link-without-a-href semantic problem, without having to add CSS to avoid showing outlines on the main element. I hope it would avoid some of the other problems I found too, but these would need a repeat of manual testing.

Needs a reroll.

Updated per the suggestion in #73.

I noticed that VoiceOver on Safari behaves differently depending upon whether you type the "enter" key after bringing up the "skip to main content" button, or you use its recommended control-option-space combination. Simply pressing enter results in voiceover simply saying there's a link, with instructions on how to open the link, whereas if you use control-option-space it reads out the label of the link. Is this by design, or is there something else that can be done to improve the "enter" scenario?

A related article that cehfisher suggested: https://axesslab.com/skip-links

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Adding for Bypass Blocks SC.

With #77 in the custom theme in my project, it solves issue from continuum scan error. The alternative approach tried was to add role="none", which also solves the same issue from continuum scan.

I rerolled the patch from #77.

Please review.

Was tagged for a followup 7 years ago is that still needed?

Started the IS but it will need some updating too.

Reroled the last successful patch #77

I'm just mainly following, I need a theme that does ARIA landmark role="main" correctly; working on a university project. I was surprised to see this doesn't yet exist in D10.2 Olivero. I guess I can override this somehow until patches are in core.

<!-- END OUTPUT from 'core/themes/olivero/templates/layout/region--secondary-menu.html.twig' -->

   </div>
     </div>
       </div>
         </div>
         </header>
    <div id="main-wrapper" class="layout-main-wrapper layout-container">
      <div id="main" class="layout-main">
        <div class="main-content">
          <a id="main-content" tabindex="-1"></a> 
          <div class="main-content__container container">
            

Discussed within Accessibility office hours February 15th (today) with several folks including A11y maintainers Mike Gifford and Ben Mullins.

This is a very low priority, and we're struggling to figure out the problem that this is solving. A potential problem, that's not in the IS, is that maybe the named anchor link will get styled unnecessarily, but I haven't seen this.

According to Ben Mullins, we need evidence that this is a problem, which we do not have. Closing and if anyone can post evidence that this is a real problem, please re-open.