Lighthouse SEO

Crawling and IndexingTo appear in search results, crawlers need access to your app.

Links are not crawlable

Search engines may use `href` attributes on links to crawl websites. Ensure that the `href` attribute of anchor elements links to an appropriate destination, so more pages of the site can be discovered. Learn More

Uncrawlable Link

a#main-content

Issue fork drupal-3222236

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

hop created an issue. See original summary.

larowlan’s picture

Status: Active » Postponed (maintainer needs more info)
Issue tags: +Bug Smash Initiative

Are you using claro as your front end theme?

hop’s picture

Yes, why not?
Is it forbidden or is it not allowed?

larowlan’s picture

Status: Postponed (maintainer needs more info) » Active

No, however its not common - as its an Admin theme

jeroent’s picture

jeroent’s picture

Also, this is not only limited to Claro but is a problem in page.html.twig in the following themes:

  • Bartik
  • Claro
  • Classy
  • Olivero
  • Seven
  • Stable
  • Stable9
  • Demo Unami

And the page.html.twig template provided by the system module.

So this:
<a id="main-content" tabindex="-1"></a>

should be updated to:
<span id="main-content" tabindex="-1"></span>

On the webform issue, tabindex was also removed. But I'm not sure if that is necessary.

jeroent’s picture

Component: Claro theme » markup
Issue tags: +Novice

saranyamariappan made their first commit to this issue’s fork.

saranyamariappan’s picture

Assigned: Unassigned » saranyamariappan
Status: Active » Needs work

I am working on it.

saranyamariappan’s picture

Assigned: saranyamariappan » Unassigned
Status: Needs work » Needs review
StatusFileSize
new623 bytes

Below patch is fixes provided only for Claro Theme.

jeroent’s picture

Status: Needs review » Needs work

Back to needs work for the other themes.

longwave’s picture

+++ b/core/themes/claro/templates/page.html.twig
@@ -49,7 +49,7 @@
     <main class="page-content clearfix" role="main">
-      <div class="visually-hidden"><a id="main-content" tabindex="-1"></a></div>
+      <div class="visually-hidden"><span id="main-content" tabindex="-1"></span></div>

The point of this is so we have an ID attribute for the browser to scroll or tab to, right? So if we're dropping the <a> here, can we just put this directly on the <main> element?

     <main class="page-content clearfix" role="main" id="main-content" tabindex="-1">

I'm unsure of the accessibility implications of any of this, but if we can replace an anchor with a span, I'm not sure why we need the span at all.

saranyamariappan’s picture

Assigned: Unassigned » saranyamariappan

I am working on it, for other core themes.

longwave’s picture

https://webaim.org/techniques/skipnav/ suggests that <main id="main-content"> is perfectly OK.

saranyamariappan’s picture

Assigned: saranyamariappan » Unassigned
Status: Needs work » Needs review
StatusFileSize
new4.04 KB
new2.72 KB

Updated the "a" tag to "span" in all page.html.twig files on all the core themes. This patch includes the changes made on #10 patch as well. This going to be the consolidated one.

Status: Needs review » Needs work

The last submitted patch, 15: 3222236-14.patch, failed testing. View results

meenakshi_j’s picture

Status: Needs work » Needs review
StatusFileSize
new4.98 KB
new804 bytes

Fixed the fails of #15

longwave’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs accessibility review

Tentatively marking RTBC as the patch solves the problem at hand, but would like an accessibility review so we can see if #12 if viable as I feel that is cleaner than using spans.

alexpott’s picture

A links without an href are valid html - see http://w3c.github.io/html-reference/a.html - is this really a bug?

JeremyFrench’s picture

A links without an href are valid html - see http://w3c.github.io/html-reference/a.html - is this really a bug?

The HTML is valid, but it has a negative SEO impact, according to lighthouse (made by google so they should know). This means potentially every site which uses one of these themes will have an inbuilt negative SEO impact on every page.

larowlan’s picture

Status: Reviewed & tested by the community » Needs work

According to https://css-tricks.com/a-deep-dive-on-skipping-to-content/ we should keep it as an anchor tag, but add a href that links back to itself

vsujeetkumar’s picture

Status: Needs work » Needs review
StatusFileSize
new5.08 KB
new4.77 KB

Addressed #21, Patch created.

@larowlan I have added href="#main-content" in anchor tag, I have bit confuse about it is the correct way to call back to itself?, Please have a look and advise.

longwave’s picture

Version: 9.2.x-dev » 9.3.x-dev
Status: Needs review » Needs work

According to https://css-tricks.com/a-deep-dive-on-skipping-to-content/ #22 is the right approach, however that link also suggests including text inside the skip link target, we currently have no text. Unsure if that is in scope for this issue or not.

After applying #22 there are still a few places that use <a id> with no href, these should all be fixed together:

core/modules/system/templates/page.html.twig:    <a id="main-content" tabindex="-1"></a>{# link is in html.html.twig #}
core/profiles/demo_umami/themes/umami/templates/layout/page.html.twig:      <a id="main-content" tabindex="-1"></a>{# link is in html.html.twig #}
core/themes/starterkit_theme/templates/layout/page.html.twig:    <a id="main-content" tabindex="-1"></a>{# link is in html.html.twig #}
core/themes/bartik/templates/maintenance-page.html.twig:            <a id="main-content"></a>
core/themes/olivero/templates/maintenance-page.html.twig:            <a id="main-content"></a>
vsujeetkumar’s picture

Status: Needs work » Needs review
StatusFileSize
new8.14 KB
new2.37 KB

As mentioned in #23, Added changes in some more places.

larowlan’s picture

I think we should add text in the anchor whilst we're here.

alexpott’s picture

@larowlan won't we then need to add the visibility-hidden class? I think maybe discussing the BC impacts of doing that is worth it and so making that part of the change in follow-up seems a good idea.

larowlan’s picture

StatusFileSize
new230.15 KB

Yeah, I hadn't considered that - and its even more involved than adding visibility-hidden

Here's a screenshot from the example in that article of when the anchor has focus

So that will be something that takes time.

I also found this resource https://developer.mozilla.org/en-US/docs/Web/HTML/Element/a#skip_links which has it linking to the main element instead of an a

longwave’s picture

@larowlan I raised the possibility of putting the ID on <main> instead of <a> in #12/14 but am unsure of the accessibility implications of doing so, so tagged for that in #18. There is also a minor issue that <main> already has a different ID in some templates, although there are child elements we could put the attribute on instead.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

noonoos’s picture

<a href="#main-content" id="main-content" tabindex="-1"></a> as per #24
Works for Bootstrap Barrio. SEO 93=>100

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

dpagini’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

Just noticing this patch is only for 9.3.x, so it should be updated. If this DOES get re-rolled however, is this direction the right approach to get this accepted to core? This makes sense to me at least as far as addressing the OP.

dpagini’s picture

Status: Needs work » Reviewed & tested by the community

Re-rolled for 10.1.x. I am using the previous patch, and now this new re-roll, and it's fixing my Google PageSpeed complaint, so I'm going to move this right to RTBC. Hopefully that will get someone from the core team to look at this again and give some direction on what's needed here to solve this problem...?

dpagini’s picture

Issue tags: -Needs reroll
dpagini’s picture

I should add that I am getting this Lighthouse "ding" now:

Links do not have a discernible name

Which I think is what is mentioned in #25.

xjm’s picture

Status: Reviewed & tested by the community » Needs review

There doesn't seem to be a specific reason that the earlier approach with <span> was abandoned, other than that the markup of <a> without href is valid and used to be very common.

I think we need that accessibility review to go forward here. I wouldn't want to degrade the navigational experience for SEO if there's a better way to improve the markup.

dpagini’s picture

@xjm - who would do that review? Can we request that review from those individuals?

Putting up an alternate merge request which I believe uses the <span> method you referred to.

smustgrave’s picture

Status: Needs review » Needs work
Issue tags: +Needs change record

Posted to the #accessibility channel to see if they can take a look

But if we are changing all the theme templates won't we need a change record if any thing base themed off one of these?

dgwolf’s picture

Hi, Lighthouse complains that links are not crawlable in our new site on D9.5.7 with the default Olivero theme that doesn't show up in Google search so far. It specifies
"Uncrawlable Link", and

  div#main-wrapper > div#main > div.main-content > a#main-content
<a id="main-content" tabindex="-1">

I am a bit lost as to which patches have been applied in 9.5.7, if any, and what workaround could currently actually be applied with the least damage resulting elsewhere. Or is an upgrade to D10 the best solution?

pradipmodh13’s picture

Hello Folks,
In Drupal 10.1.x-dev anchor tag comes with href attribute. So no need to work.
For ref attached screenshot. So we can move this bug to needs review.

pradipmodh13’s picture

StatusFileSize
new566.8 KB

Hello Folks,
In Drupal 10.1.x-dev anchor tag comes with href attribute. So no need to work.
For ref attached screenshot. So we can move this bug to needs review.

pradipmodh13’s picture

Status: Needs work » Needs review
StatusFileSize
new4.28 KB
new309.49 KB
new239.95 KB

After looking further, it was discovered that the anchor tag's href property was missing, thus href was added in accordance with usual practise.
After applying the fix, I verified that the lighthouse reports were error-free.
Please review

longwave’s picture

Status: Needs review » Needs work

#47 is wrong, now the anchor is a link to itself.

I still think the simplest solution is to put the ID directly on the <main> element.

WebAIM suggests that this would be OK: https://webaim.org/techniques/skipnav/

<a href="#main-content" class="visually-hidden focusable skip-link">
...
<main id="main-content">

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

gauravvvv’s picture

Status: Needs work » Needs review
StatusFileSize
new4.61 KB
new4.75 KB

Addressed feedback from #48, Attached interdiff with 47, please review

dpagini’s picture

I came across this other core issue regarding these same links, and I think a solution should probably consider both of these issues...?

smustgrave’s picture

Status: Needs review » Needs work

Seems latest change caused failures.

dpagini’s picture

What do you mean by that, @smustgrave?

smustgrave’s picture

#50 didn't fully pass. Caused some nightwatch errors I believe.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.