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

bug: navigating to anchor links within an admonition not working with navigation.instant #5239

Closed
4 tasks done
Valastiri opened this issue Mar 21, 2023 · 4 comments
Closed
4 tasks done
Labels
bug Issue reports a bug resolved Issue is resolved, yet unreleased if open

Comments

@Valastiri
Copy link

Valastiri commented Mar 21, 2023

Context

We have a "reported issues" tracking page on our documentation that utilizes H3 elements within collapsed admonitions to help provide navigation on page with the TOC and create a more condensed page (reading wise).

Recently we reintroduced navigation.instant to our docs as part of mkdocs-material 9.1.1 to solve the following issues outlined in this PR:

Ideally what usually happens is when a user clicks on a link in the TOC it would take them to the relevant section on the page and open the admontion.

removed live site example as we removed instant navigation on production
example.zip with minimal repro provided below

Bug description

Issue

With navigation.instant enabled on our build we recently discovered that clicking on links in the table of contents in certain browsers outlined below would not jump to the relevant anchor/section when the header is positioned within a collapsible admonition.

It is currently not working on:

  • Safari (all devices)
  • Firefox (Windows + iPadOS tested)

Example:

??? bug "Title of Admonition"
		### header

		text

Additional Context

This setup has worked for us on all browsers and devices until we discovered this issue recently with no changes to our build. Currently, it only works on the Chrome Browser (Windows + MacOS + Mobile Devices).


It is important to note that anchor links / section links work flawlessly on any browser when the header is not placed within an admonition.

Related links

Relevant Discussions I looked at:

Reproduction

Example provided below

Please see the page titled Example Issue: it is a clone of our reported issues page on our live site.

example.zip

Steps to reproduce

  1. Ensure navigation.instant is active in the config.
  2. Ensure that admonitions is configured in the config.
  3. Create a collapsible admonition with a header element nested within (example in bug section or below here) on a page of your choice
  4. Spin up mkdocs serve or build
  5. Navigate to localhost:8000 or open index.html on Safari or Firefox
  6. Using Safari or Firefox // on the page try and navigate to a header nested within an admonition (samples available in repro zip file)

Expected outcome: You would be taken to the link you clicked on in the TOC + admonition would open to reveal content.

Current outcome: Navigation does not happen and TOC links are inoperable.


Example admonition setup:

??? bug "Title of Admonition"
		### header

		text

Browser

Safari, Firefox

Before submitting

@squidfunk squidfunk added needs investigation Issue must be investigated by the maintainers bug Issue reports a bug and removed needs investigation Issue must be investigated by the maintainers labels Mar 22, 2023
@squidfunk
Copy link
Owner

Thanks for reporting, and especially for the excellent reproduction. That made troubleshooting straightforward. This is a regression of the latest refactoring in instant loading and I took the time to investigate why this is the case. My findings:

Previously, instant loading filtered hash changes (= clicking on anchors) that happen on the same page, and leave them to the browser. This triggered the logic we have in place to detect whether an anchor is targeting something that is located in a hidden element, which is currently only implemented for details elements:

/* Open and focus details on location target */
target$
.pipe(
map(target => target.closest("details:not([open])")!),
filter(details => el === details),
map(() => ({
action: "open", reveal: true
}) as Details)
),

Now, if you look at the commit history, these lines were added two years ago before the advent of code annotations, content tabs with anchor links and other features that may include anchors in hidden elements. The latest refactoring of instant loading that was released in 9.1.1 has a different behavior as it will now also handle anchor links. The reason is primarily to correctly implement scroll restoration, i.e., when you click on a link, then scroll down, then navigate away and go back to that section. Additionally, instant loading had sometimes problems on slow connections, which were also fixed with this release. However, it leads to the problem that in some browsers (notably Firefox and Safari), the hashchange event is not triggered anymore, as observed. While I would attribute this to a browser implementation inconsistency, it is likely related to us now also calling Event.preventDefault for pure anchor navigation events:

// We now know that we have a link to an internal page, so we prevent
// the browser from navigation and emit the URL for instant navigation.
// Note that this also includes anchor links, which means we need to
// implement anchor positioning ourselves.
ev.preventDefault()
return of(new URL(el.href))

The issue should be fixed with 183b0be. I consider this a temporary bandaid, as we must defer control of anchor links to instant loading for the reasons stated. We will soon tackle proper expansion of nested hidden elements, which is not solely relevant when following anchor links, but also for search highlighting (see #4125). Also, see #4125 (comment) why this is particularly challenging, as cases become pathological quite quickly.

@squidfunk squidfunk added the resolved Issue is resolved, yet unreleased if open label Mar 24, 2023
@squidfunk
Copy link
Owner

Released as part of 9.1.4.

@endel
Copy link

endel commented May 2, 2023

Sorry for commenting on a closed thread but navigation.instant is causing the page to scroll when clicking on code tab anchors for me. Is this something happening for you as well? (I'm using the insiders version)

(If that's not a known issue, I can prepare a repro scenario soon!)

navigation-instant-bug.mp4

@squidfunk
Copy link
Owner

Not known. Please create a bug report, provide a minimal reproduction, and we're happy to look into it!

mattkaar added a commit to cmu-ini/aia-web that referenced this issue Aug 27, 2023
- Adds cheating callout in team exercises section
- Reinforce individual work of homework assignments
- Disable `navigation.instant` due to anchor link issues with Safari: squidfunk/mkdocs-material#5239 (comment)
mattkaar added a commit to cmu-ini/aia-web that referenced this issue Aug 28, 2023
* Update time, location and schedule for F23

* Adds academic misconduct section

- Adds cheating callout in team exercises section
- Reinforce individual work of homework assignments
- Disable `navigation.instant` due to anchor link issues with Safari: squidfunk/mkdocs-material#5239 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issue reports a bug resolved Issue is resolved, yet unreleased if open
Projects
None yet
Development

No branches or pull requests

3 participants