Docusaurus: h3 topics with Markdown formatting causes a glitch on mobile

[Luanf]

🐛 Bug Report

Found this while investigating a Jest issue: https://github.com/facebook/jest/issues/7856
When navigating a "topics" (?) menu on a mobile screen size, clicking an h3 header that has Markdown formatting does not closes the menu.

Have you read the Contributing Guidelines on issues?

Yes.

To Reproduce

Note: I'm using a local Jest repository using Docusaurus 1.7.2 to reproduce this

1. Add some Markdown formatting (such as bold, italics or code) to a h3 header on a document.

2. Use a responsive website size small enough to have this UI:
   

3. Click the vertical three dots button to see all references:
   

4. Attempting to navigate to a h3 section formatted with Markdown will not hide the page, although it correctly redirects the URL:
   

Expected behavior

The open menu should have closed.

Actual Behavior

URL changes but the menu isn't closed.
Removing the backticks from the h3 makes it function properly.
Adding other formatting such as bold and italics generate the same issue.

[shakcho]

I'd like to help with this issue, as I'm first-time contributor and the issue is labeled with good-first-time, is there anything specific thing I need to take care of, please let me know.
Thanks

[shakcho]

I've found the issue, basically in /Docusaurus/packages/docusaurus-1.x/lib/core/nav/SideNav.js in line number 190

const headings = document.querySelector('.toc-headings');
headings && headings.addEventListener('click', function(event) {
                if (event.target.tagName === 'A') {
                  document.body.classList.remove('tocActive');
                }
  }, false);

we are adding click listener on ".toc-headings" and removing toActive class if the target is "A" tag, where in case of Jest documention it is

<li><a href="#afterallfn-timeout"><code>afterAll(fn, timeout)</code></a></li>

so the target will come as "CODE" tag instead of "A" tag, hence it is breaking.

Proposed Solution

const headingsLink = document.querySelectorAll('.toc-headings a');
for (let i = 0; i < headingsLink.length;  i++) {
      headingsLink[i].addEventListener( "click", function(event) {
                document.body.classList.remove('tocActive');
  }, false);

[yangshun]

Thanks for looking into this! I'm not sure if the proposed solution works, but I think there's a better way than adding event listeners to all the <a> as there could be a performance overhead of adding too many event listeners. The original approach tries to overcome this overhead by using event delegation but its implementation failed to handle the case of other elements being targets. This problem also exists on docusaurus.io: https://docusaurus.io/docs/en/commands#docusaurus-publish

I think the proper fix is to recursively traverse the parents of the event.target with event.target.parentNode and terminate when:

• an instances where tagName === 'A' is found
• It hits .toc-headings (means it's not clicking on any <a> or its children)

Feel free to create a PR to fix this! You can use the local Docusaurus site to test the fix.

[shakcho]

Hi @yangshun I actually thought about the performance issue with the above solution and thought to do it in the same approach you mentioned, but I thought to traverse to the parents will be more costly.

Anyway, I would like to propose another solution which is pure CSS fix and no code changes will be required in the js.

The CSS only Solution

.toc-headings a { 
    position: relative;
}
.toc-headings a:after { 
  position : absolute ;
  content : "";
  left : 0;
  right : 0;
  top : 0;
  bottom: 0;
}

By using this css style, we basically force all the click event in Anchor tag, it doesn't matter whatever the children present.

Your proposed solution with JavaScript

const headings = document.querySelector('.toc-headings');
headings && headings.addEventListener('click', function(event) {
  let el = event.target;
  while(el !== event.currentTarget){
    if (el.tagName === 'A') {
      document.body.classList.remove('tocActive');
      break;
    } else{
      el = el.parentNode;
    }
  }
}, false);

I've tested both the solution and both are working fine.
We can choose any one of them, please let me know which one will better way to solve the issue also let me know if I'm missing something, Based on that I will create the Pull Request.

Thanks