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

Internal anchors with hashes not working #25778

Closed
ediblecode opened this issue Jul 15, 2020 · 28 comments · Fixed by #28555
Closed

Internal anchors with hashes not working #25778

ediblecode opened this issue Jul 15, 2020 · 28 comments · Fixed by #28555
Labels
topic: core Relates to Gatsby's core (e.g. page loading, reporter, state machine) type: bug An issue or pull request relating to a bug in Gatsby

Comments

@ediblecode
Copy link
Contributor

Description

Internal anchors with hashes stop working after the first click. See the minimal test case repo at https://github.com/ediblecode/gatsby-anchors-issue and demo site at https://frosty-brown-de3460.netlify.app/. Click into page 2 and click the various internal anchors on that page.

Note: this is using simple anchor tags as per the docs for gatsby link:

Neither <Link> nor navigate can be used for in-route navigation with a hash or query parameter. If you need this behavior, you should either use an anchor tag...

I thought this was fixed by #25749 and released in 2.24.3 but it still seems to be an issue. I think it's related to, if not the same as, #25554, #25745 and #25522. I wonder if it was caused by #24306 (or at least related in some way).

The first anchor seems to be OK but it's definitely something to do with subsquent clicks. Weirdly though it's non-determinate: sometimes it will work fine and sometimes it won't and I can't seem to work out the pattern. Looking into it, session storage is used under the hood for the scroll position, so I wonder if it's something to do with that - maybe the key used isn't including the anchor hash or something? Anyway, that's as far as I got with my digging, sorry. Hoping @blainekasten you might be able to take a look as you've looked at the other related issues. Thank you very much.

Steps to reproduce

  1. Clone the gatsby starter
  2. Upgrade dependencies to latest via npx npm-check-updates -u
    1. This includes Gatsby 2.24.3
  3. Run npm i to install these dependencies
  4. Create a page with hash links to elements on a page with matching ids (see example repo and site)
  5. Click the internal hash anchors to try to jump up and down the page.

Expected result

The URL is updated with the hash, and the page is scrolled to the element with an id matching the hash in the URL.

Actual result

The first click scrolls to the target element and updates the URL but subsequent clicks only update the URL without moving down the page.

Environment

System:
OS: Windows 10 10.0.19041
CPU: (4) x64 Intel(R) Core(TM) i7-5500U CPU @ 2.40GHz
Binaries:
Node: 12.16.3 - C:\Program Files\nodejs\node.EXE
Yarn: 1.16.0 - C:\Program Files (x86)\Yarn\bin\yarn.CMD
npm: 6.14.4 - C:\Program Files\nodejs\npm.CMD
Languages:
Python: 2.7.11 - C:\Python27\python.EXE
Browsers:
Edge: 44.19041.1.0
npmPackages:
gatsby: ^2.24.3 => 2.24.3
gatsby-image: ^2.4.13 => 2.4.13
gatsby-plugin-manifest: ^2.4.18 => 2.4.18
gatsby-plugin-offline: ^3.2.18 => 3.2.18
gatsby-plugin-react-helmet: ^3.3.10 => 3.3.10
gatsby-plugin-sharp: ^2.6.19 => 2.6.19
gatsby-source-filesystem: ^2.3.19 => 2.3.19
gatsby-transformer-sharp: ^2.5.11 => 2.5.11

@ediblecode ediblecode added the type: bug An issue or pull request relating to a bug in Gatsby label Jul 15, 2020
@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Jul 15, 2020
@blainekasten blainekasten removed the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Jul 15, 2020
@blainekasten blainekasten self-assigned this Jul 15, 2020
@annmirosh
Copy link

I'm experiencing the same issue and the solution from #25554 doesn't work for me. Any ideas on how this can be fixed?

@dalxds
Copy link

dalxds commented Jul 16, 2020

exact same issue here!
Sometimes it works fine, sometimes it doesn't.
I think on iOS works correctly most times. Also, if you don't scroll between hitting two anchors (using the menu for example) also works correct most times.
Also, I think the problem is something that came up last 15 days (maybe less) and didn't existed before.
Can't think of anything else that might help.

@ediblecode
Copy link
Contributor Author

I think I have a workaround that seems to be OK (if hacky). There were various comments on #25554 about downgrading gatsby (e.g. to 2.24.18) or downgrading gatsby-react-router-scroll to 3.0.3. Downgrading gatsby seemed to introduce other issues, for example I came across #20082 and I wasn't a fan of downgrading the whole of gatsby just to fix this issue. So I opted for the specific version of gatsby-react-router-scroll, like the comments.

I checked our existing package-lock.json and it was using [email protected], so I went with that version to be safe, although people do seem to suggest 3.0.3 would be OK. (I haven't actually reviewed which version the issue was introduced in).

Unfortunately, the idea of using resolutions in package.json, although great if you're using yarn, didn't work for us as we're using npm, which doesn't support resolutions natively (I wonder if that's the reason for this comment). So instead I used the npm-force-resolutions package to replicate this yarn functionality. So my package.json looks like this:

{
  "scripts": {
+    "preinstall": "npx npm-force-resolutions",
  },
"devDependencies": {
  "gatsby": "2.24.3",
+  "gatsby-react-router-scroll": "3.0.0",
},
+"resolutions": {
+    "gatsby-react-router-scroll": "3.0.0"
+  }

Notice I needed gatsby-react-router-scroll in both devdeps and resolutions to avoid getting "Can't resolve 'gatsby-react-router-scroll' in '.cache'" errors, even though I'm not using it as a dependency directly.

To be honest, it's a bit hacky but it does mean it works as I was being blocked from any new releases because of the functional regression and the trouble I was having downgrading. I can easily back out the line in the diff above when the issue is fixed.

@josephmarkus maybe this could fix your issue?

ediblecode added a commit to nice-digital/cks-gatsby that referenced this issue Jul 17, 2020
@josephmarkus
Copy link
Contributor

josephmarkus commented Jul 17, 2020

@ediblecode thanks for the insight. I'll try your suggestion on the latest version of gatsby. In the meantime, my issue was addressed by this fix here - #25749 (and here's the original issue I raised - #25745), namely on a downgraded version of gatsby.

@dalxds
Copy link

dalxds commented Jul 17, 2020

I am using gatsby themes! should I use the resolution solution to the theme's package.json or the app's (that uses the theme) package.json or both?
thanks!

@ediblecode
Copy link
Contributor Author

@dalxds not used themes, but probably makes sense to be in the app's I think

kavithanice pushed a commit to nice-digital/cks-gatsby that referenced this issue Jul 22, 2020
* CKS-334 Scaffold chapter pages

* CKS-334 Remove extra topic page content

* CKS-334 Flatten topic menu until nested stacked nav

* CKS-334 Set correct chapter heading

* CKS-334 Rewrite chater content links

* CKS-334 Remove HTML processing

* CKS-334 Add comment for scss declaration

* CKS-334 Refactors

* CKS-334 Refactor chapter types

* CKS-334 Fix test

* CKS-334 Fix test

* CKS-334 Update sub chapters sub nav indentation

* CKS-334 Add key to topic menu fragment

* CKS-334 Refactor chapter body

* CKS-334 Add HTML utils tests

* CKS-334 Ignore graphql fragments from coverage

* CKS-334 Add chapter body tests

* CKS-334 Add insertId tests

* CKS-334 Add placeholder tests for the chapter template

* CKS-334 Refactor chapter pages

* CKS-334 Refactor chapter headings to correct level

* CKS-334 Change home breadcrumb to NICE

* CKS-334 Add level 1 chapter tests

* CKS-334 Add chapter body tests

* CKS-334 Remove unusde fragments

* CKS-334 Remove grid from chapter body

* CKS-334 Add comments and fix import path

* CKS-334 Add split CSS plugin

* CKS-334 Fix await in wrong place

* CKS-334 Split CSS

Reduces total build size and per-page size by including
only the required CSS

* CKS-334 Remove spacing modifiers

Save A LOT of space in the final build size as
ALL of the spacing modifiers CSS was being
included in EVERY single chapter page
of which there are 12000 or so

* CKS-334 Update package lock

* CKS-334 Update package lock

* CKS-334 Fix topic summary link

* CKS-334 Fix test

* CKS-334 Fix gatsby-react-router-scroll version

See gatsbyjs/gatsby#25778 (comment)

* CKS-334 Fix type def for test

* CKS-334 Fix flakey functional tests

* CKS-334 Add Asthma topic page tests

* CKS-334 Refactor breadcrumb step

* CKS-334 Re-instate topic page a11y tests

* CKS-334 Add chapter page functional tests

* CKS-334 Use new 2 level stacked nav for topic menu

* CKS-334 Fix tests

* CKS-334 Increase test timeout

* CKS-334 Add LangVersion to latest

Avoids "Feature 'using declarations' is not available in C# 7.3.
Please use language version 8.0 or greater." error

* CKS-334 Refactor topic graphql query

* CKS-334 Add topic chapters menu tests

* CKS-334 Add chapter level 2 tests

* CKS-334 Update design system version

* CKS-334 Bump global nav commit

* CKS-334 Update NDS version, exclude unused classes

* CKS-334 Fix styling issues after NDS update

Caused by the new max width on lists

* CKS-334 Fix fake API not debugging

* CKS-334 Fix HTML encoding in topic lead

* CKS-334 Fix jest tests not outputting debug logs

* CKS-334 Add topic page lead paragraph test

* CKS-334 Refactor chapter level 1 tests

* CKS-334 Increase test timeouts

* CKS-334 Update design system package

Gets new breadcrumbs update with wider max width
@robmarshall
Copy link
Contributor

I am also having this issue.

@ediblecode
Copy link
Contributor Author

I should point out as well that the hacky workaround isn't ideal because it doesn't move focus to the correct place in the document when you load a page that includes a hash. So keyboard users who try to tab from the id matching the hash can't and they start back at the top of the document. So not great for accessibility. So use that workaround with caution until it's fixed. (this is because it missed out on all the goodness from #24306)

@robmarshall
Copy link
Contributor

This issue is here as well it seems: #19480

pat270 added a commit to pat270/clay that referenced this issue Aug 31, 2020
…ink anchors scroll to the right spot and add @reach/[email protected] to remove peer dependency warning

See gatsbyjs/gatsby#25778 (comment)

fix(clayui.com): Linking CSS docs should take you to the right place

fix(clayui.com): Nav Toc should still be scrollable when the browser window is really short
pat270 added a commit to pat270/clay that referenced this issue Sep 1, 2020
…ink anchors scroll to the right spot and add @reach/[email protected] to remove peer dependency warning

See gatsbyjs/gatsby#25778 (comment)

fix(clayui.com): Linking CSS docs should take you to the right place

fix(clayui.com): Nav Toc should still be scrollable when the browser window is really short
@robmarshall
Copy link
Contributor

What is the verdict on this? I am concerned it will affect SEO on a large number of my clients pages if the links are not connecting properly.

@lukekarrys
Copy link

This seems like too simple of a fix for it to be right, but does this work for anyone else? I noticed that in the gatsby-react-router-scroll/scroll-handler it was checking for a scroll position of 0 in order to scroll to the hash node. I confirmed on my site that if I was scrolled to the top of the window it worked, but if I was scrolled down at all it didn't.

So I experimented by running patch-package with the patch below and it worked for me.

diff --git a/node_modules/gatsby-react-router-scroll/scroll-handler.js b/node_modules/gatsby-react-router-scroll/scroll-handler.js
index 91114f2..9284f09 100644
--- a/node_modules/gatsby-react-router-scroll/scroll-handler.js
+++ b/node_modules/gatsby-react-router-scroll/scroll-handler.js
@@ -104,7 +104,7 @@ var ScrollHandler = /*#__PURE__*/function (_React$Component) {
       scrollPosition = this._stateStorage.read(this.props.location, key);
     }
 
-    if (hash && scrollPosition === 0) {
+    if (hash) {
       this.scrollToHash(decodeURI(hash), prevProps);
     } else {
       this.windowScroll(scrollPosition, prevProps);

@inioluwa-io
Copy link

I had this issue too with but I worked around it by hard coding click events on all links and window.scroll({ top: ElementTop, behavior: "smooth" }). Don't forget to e.preventDefault() on click event. Something like this:


  useEffect(() => {
    const DOMNode = refs.current

    const links = Array.from(DOMNode.querySelectorAll("a"))

    const getElementScrollTopById = (id) => {
      if (DOMNode.querySelector(id)) {
        return DOMNode.querySelector(id).getBoundingClientRect().top
      }
      return null
    }

    links.forEach((link) => {
      const hash = link.hash
      if (hash) {
        const scrollTo = getElementScrollTopById(hash)
        link.addEventListener("click", (e) => {
          e.preventDefault()
          window.scroll({ top: scrollTo, behavior: "smooth" })
        })
      }
    })
  }, [])

@inioluwa-io
Copy link

I had this issue too with but I worked around it by hard coding click events on all links and window.scroll({ top: ElementTop, behavior: "smooth" }). Don't forget to e.preventDefault() on click event. Something like this:


  useEffect(() => {
    const DOMNode = refs.current

    const links = Array.from(DOMNode.querySelectorAll("a"))

    const getElementScrollTopById = (id) => {
      if (DOMNode.querySelector(id)) {
        return DOMNode.querySelector(id).getBoundingClientRect().top
      }
      return null
    }

    links.forEach((link) => {
      const hash = link.hash
      if (hash) {
        const scrollTo = getElementScrollTopById(hash)
        link.addEventListener("click", (e) => {
          e.preventDefault()
          window.scroll({ top: scrollTo, behavior: "smooth" })
        })
      }
    })
  }, [])

Keep in mind this removes setting the hash on the location

@LekoArts LekoArts added the topic: core Relates to Gatsby's core (e.g. page loading, reporter, state machine) label Sep 30, 2020
adamschwartz added a commit to cloudflare/cloudflare-docs-engine that referenced this issue Oct 10, 2020
Should fix:
#282
(Due to gatsbyjs/gatsby#25778)

We initially tried 2.24.1:
5fd5a56

But that seems to have caused:
gatsbyjs/gatsby#25899

But the comment from that issue seems to work:
gatsbyjs/gatsby#25899 (comment)
@github-actions
Copy link

Hiya!

This issue has gone quiet. Spooky quiet. 👻

We get a lot of issues, so we currently close issues after 60 days of inactivity. It’s been at least 20 days since the last update here.
If we missed this issue or if you want to keep it open, please reply here.
As a friendly reminder: the best way to see this issue, or any other, fixed is to open a Pull Request. Check out gatsby.dev/contribute for more information about opening PRs, triaging issues, and contributing!

Thanks for being a part of the Gatsby community! 💪💜

@github-actions github-actions bot added the stale? Issue that may be closed soon due to the original author not responding any more. label Oct 21, 2020
@spences10
Copy link
Contributor

Still not resolved AFAIK actions bot

@github-actions github-actions bot removed the stale? Issue that may be closed soon due to the original author not responding any more. label Oct 21, 2020
@hoffmanilya
Copy link

+1

@lukekarrys patch seems to be working well for me though.

@spences10
Copy link
Contributor

+1

@lukekarrys patch seems to be working well for me though.

Cool! I did the same, patch package is awesome!!

Still not resolved though 😇

@github-actions
Copy link

Hiya!

This issue has gone quiet. Spooky quiet. 👻

We get a lot of issues, so we currently close issues after 60 days of inactivity. It’s been at least 20 days since the last update here.
If we missed this issue or if you want to keep it open, please reply here.
As a friendly reminder: the best way to see this issue, or any other, fixed is to open a Pull Request. Check out gatsby.dev/contribute for more information about opening PRs, triaging issues, and contributing!

Thanks for being a part of the Gatsby community! 💪💜

@github-actions github-actions bot added the stale? Issue that may be closed soon due to the original author not responding any more. label Nov 11, 2020
@ediblecode
Copy link
Contributor Author

Hold your horses Gatsby bot - I don't think this is fixed and patch-package is a hack/workaround. Not stale!

@github-actions github-actions bot removed the stale? Issue that may be closed soon due to the original author not responding any more. label Nov 11, 2020
@DekiGk
Copy link

DekiGk commented Nov 16, 2020

@lukekarrys Thanks for the workaround. I did not know about patch-package. Awesome.

@ediblecode Did someone from the maintainers respond? If not maybe we can make a PR from @lukekarrys fix?

@ediblecode
Copy link
Contributor Author

I'm not sure they did. I'm also not convinced that fix will work for backwards navigation (with the browser back button) - it looks like it would always navigate to the hash, rather than the previous scroll position - at least without testing it that's what it looks like. Sorry, haven't had time to actually have a look at the fix properly.

@EricCoteReact
Copy link

EricCoteReact commented Nov 26, 2020

I found a very nice workaround. No need to keep older versions or use any effect hooks.

Write the following lines of code in the file gatsby-browser.js:

export const shouldUpdateScroll = ({ routerProps: { location } }) => {
  if (location.hash === '') {
    window.scrollTo(0, 0);
  }

  return false;
};

@mxstbr
Copy link
Contributor

mxstbr commented Dec 9, 2020

A PR with the fix for this based on @lukekarrys suggestion above is open now: #28555

@jawakarD
Copy link

Isn't it possible for the hash to not match with any element using hash as id? Shouldn't we scroll to the top in that case? For now we're not doing anything in that case.

If the current page (A) has scrollY of 4000 and next page's (B's) height is only 2000. After we click a link (/next-page#B) which takes us to the next page with a hash that doesn't have matching element. The viewport will be at the bottom of the B page.

Let me know If I should open a PR for this.

@dmaxchristiansen
Copy link

It looks like this is happening again on Gatsby v5. I noticed the behavior appearing after migrating this week from v4.

@edysmp
Copy link

edysmp commented Jan 31, 2023

https://www.gatsbyjs.com/docs/reference/release-notes/v5.5/ introduces a fix for anchor links.

@ZeldOcarina
Copy link

@edysmp it doesn't fix it for me unfortunately.. :(

@therealgilles
Copy link

It only works for me if I modify the code to add a setTimeout(..., 0) when calling node.scrollIntoView():

    _this.scrollToHash = function (hash, prevProps) {
      var node = document.getElementById(hash.substring(1));
      if (node && _this.shouldUpdateScroll(prevProps, _this.props)) {
        // node.scrollIntoView();
        setTimeout(() => node.scrollIntoView(), 0);
      }
    };

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: core Relates to Gatsby's core (e.g. page loading, reporter, state machine) type: bug An issue or pull request relating to a bug in Gatsby
Projects
None yet
Development

Successfully merging a pull request may close this issue.