Fix heading nav with folded chapters

This fixes an issue when folding is enabled. The folding was not
properly hiding the sub-chapters because it was assuming it could hide
the next list element. However, the heading nav was the next list
element, so the remaining chapters remained visible.

The solution required some deeper changes to how the chapters were
organized in the sidebar. Instead of nested chapters being a list
element *sibling*, the nested chapter's `ol` is now a *child* of its
parent chapter. This makes it much easier to just hide everything
without regard of the exact sibling order.

This required wrapping the chapter title and the toggle chevron inside a
span so that the flex layout could be localized to just those elements,
and allow the following `ol` elements to lay out regularly.

Closes #2880

Author: ehuss

crates/mdbook-html/front-end/css/chrome.css

@@ -571,17 +571,18 @@ html:not(.sidebar-resizing) .sidebar {
     line-height: 2.2em;
 }
 
-.chapter ol {
-    width: 100%;
+.chapter li {
+    color: var(--sidebar-non-existant);
 }
 
-.chapter li {
+/* This is a span wrapping the chapter link and the fold chevron. */
+.chapter-link-wrapper {
+    /* Used to position the chevron to the right, allowing the text to wrap before it. */
     display: flex;
-    color: var(--sidebar-non-existant);
 }
+
 .chapter li a {
-    display: block;
-    padding: 0;
+    /* Remove underlines. */
     text-decoration: none;
     color: var(--sidebar-fg);
 }
@@ -594,21 +595,22 @@ html:not(.sidebar-resizing) .sidebar {
     color: var(--sidebar-active);
 }
 
-.chapter li > a.toggle {
+/* This is the toggle chevron. */
+.chapter-fold-toggle {
     cursor: pointer;
-    display: block;
+    /* Positions the chevron to the side. */
     margin-inline-start: auto;
     padding: 0 10px;
     user-select: none;
     opacity: 0.68;
 }
 
-.chapter li > a.toggle div {
+.chapter-fold-toggle div {
     transition: transform 0.5s;
 }
 
 /* collapse the section */
-.chapter li:not(.expanded) + li > ol {
+.chapter li:not(.expanded) > ol {
     display: none;
 }
 
@@ -617,10 +619,12 @@ html:not(.sidebar-resizing) .sidebar {
     margin-block-start: 0.6em;
 }
 
-.chapter li.expanded > a.toggle div {
+/* When expanded, rotate the chevron to point down. */
+.chapter li.expanded > span > .chapter-fold-toggle div {
     transform: rotate(90deg);
 }
 
+/* Horizontal line in chapter list. */
 .spacer {
     width: 100%;
     height: 3px;
@@ -630,6 +634,7 @@ html:not(.sidebar-resizing) .sidebar {
     background-color: var(--sidebar-spacer);
 }
 
+/* On touch devices, add more vertical spacing to make it easier to tap links. */
 @media (-moz-touch-enabled: 1), (pointer: coarse) {
     .chapter li a { padding: 5px 0; }
     .spacer { margin: 10px 0; }
@@ -741,7 +746,6 @@ html:not(.sidebar-resizing) .sidebar {
     content: '';
     position: absolute;
     left: -16px;
-    top: 0;
     margin-top: 10px;
     width: 8px;
     height: 8px;

crates/mdbook-html/front-end/templates/toc.js.hbs

@@ -29,14 +29,9 @@ class MDBookSidebarScrollbox extends HTMLElement {
                 && current_page.endsWith('/index.html')) {
                 link.classList.add('active');
                 let parent = link.parentElement;
-                if (parent && parent.classList.contains('chapter-item')) {
-                    parent.classList.add('expanded');
-                }
                 while (parent) {
-                    if (parent.tagName === 'LI' && parent.previousElementSibling) {
-                        if (parent.previousElementSibling.classList.contains('chapter-item')) {
-                            parent.previousElementSibling.classList.add('expanded');
-                        }
+                    if (parent.tagName === 'LI' && parent.classList.contains('chapter-item')) {
+                        parent.classList.add('expanded');
                     }
                     parent = parent.parentElement;
                 }
@@ -62,9 +57,9 @@ class MDBookSidebarScrollbox extends HTMLElement {
             }
         }
         // Toggle buttons
-        const sidebarAnchorToggles = document.querySelectorAll('#mdbook-sidebar a.toggle');
+        const sidebarAnchorToggles = document.querySelectorAll('.chapter-fold-toggle');
         function toggleSection(ev) {
-            ev.currentTarget.parentElement.classList.toggle('expanded');
+            ev.currentTarget.parentElement.parentElement.classList.toggle('expanded');
         }
         Array.from(sidebarAnchorToggles).forEach(el => {
             el.addEventListener('click', toggleSection);
@@ -237,17 +232,12 @@ window.customElements.define('mdbook-sidebar-scrollbox', MDBookSidebarScrollbox)
     // be expanded.
     function updateHeaderExpanded(currentA) {
         // Add expanded to all header-item li ancestors.
-        let current = currentA.parentElement.parentElement.parentElement;
-        while (current.tagName === 'LI') {
-            const prevSibling = current.previousElementSibling;
-            if (prevSibling !== null
-                && prevSibling.tagName === 'LI'
-                && prevSibling.classList.contains('header-item')) {
-                prevSibling.classList.add('expanded');
-                current = prevSibling.parentElement.parentElement;
-            } else {
-                break;
+        let current = currentA.parentElement;
+        while (current) {
+            if (current.tagName === 'LI' && current.classList.contains('header-item')) {
+                current.classList.add('expanded');
             }
+            current = current.parentElement;
         }
     }
 
@@ -343,19 +333,6 @@ window.customElements.define('mdbook-sidebar-scrollbox', MDBookSidebarScrollbox)
         if (activeSection === null) {
             return;
         }
-        const activeItem = activeSection.parentElement;
-        const activeList = activeItem.parentElement;
-
-        // Build a tree of headers in the sidebar.
-        const rootLi = document.createElement('li');
-        rootLi.classList.add('header-item');
-        rootLi.classList.add('expanded');
-        const rootOl = document.createElement('ol');
-        rootOl.classList.add('section');
-        rootLi.appendChild(rootOl);
-        const stack = [{ level: 0, ol: rootOl }];
-        // The level where it will start folding deeply nested headers.
-        const foldLevel = 3;
 
         const main = document.getElementsByTagName('main')[0];
         headers = Array.from(main.querySelectorAll('h2, h3, h4, h5, h6'))
@@ -365,57 +342,90 @@ window.customElements.define('mdbook-sidebar-scrollbox', MDBookSidebarScrollbox)
             return;
         }
 
+        // Build a tree of headers in the sidebar.
+
+        const stack = [];
+
+        const firstLevel = parseInt(headers[0].tagName.charAt(1));
+        for (let i = 1; i < firstLevel; i++) {
+            const ol = document.createElement('ol');
+            ol.classList.add('section');
+            if (stack.length > 0) {
+                stack[stack.length - 1].ol.appendChild(ol);
+            }
+            stack.push({level: i + 1, ol: ol});
+        }
+
+        // The level where it will start folding deeply nested headers.
+        const foldLevel = 3;
+
         for (let i = 0; i < headers.length; i++) {
             const header = headers[i];
             const level = parseInt(header.tagName.charAt(1));
+
+            const currentLevel = stack[stack.length - 1].level;
+            if (level > currentLevel) {
+                // Begin nesting to this level.
+                for (let nextLevel = currentLevel + 1; nextLevel <= level; nextLevel++) {
+                    const ol = document.createElement('ol');
+                    ol.classList.add('section');
+                    const last = stack[stack.length - 1];
+                    const lastChild = last.ol.lastChild;
+                    // Handle the case where jumping more than one nesting
+                    // level, which doesn't have a list item to place this new
+                    // list inside of.
+                    if (lastChild) {
+                        lastChild.appendChild(ol);
+                    } else {
+                        last.ol.appendChild(ol);
+                    }
+                    stack.push({level: nextLevel, ol: ol});
+                }
+            } else if (level < currentLevel) {
+                while (stack.length > 1 && stack[stack.length - 1].level >= level) {
+                    stack.pop();
+                }
+            }
+
             const li = document.createElement('li');
             li.classList.add('header-item');
             li.classList.add('expanded');
             if (level < foldLevel) {
                 li.classList.add('expanded');
             }
+            const span = document.createElement('span');
+            span.classList.add('chapter-link-wrapper');
             const a = document.createElement('a');
+            span.appendChild(a);
             a.href = '#' + header.id;
             a.classList.add('header-in-summary');
             a.innerHTML = header.children[0].innerHTML;
             a.addEventListener('click', headerThresholdClick);
-            li.appendChild(a);
             const nextHeader = headers[i + 1];
             if (nextHeader !== undefined) {
                 const nextLevel = parseInt(nextHeader.tagName.charAt(1));
                 if (nextLevel > level && level >= foldLevel) {
-                    const div = document.createElement('div');
-                    div.textContent = '❱';
                     const toggle = document.createElement('a');
-                    toggle.classList.add('toggle');
+                    toggle.classList.add('chapter-fold-toggle');
                     toggle.classList.add('header-toggle');
-                    toggle.appendChild(div);
                     toggle.addEventListener('click', () => {
                         li.classList.toggle('expanded');
                     });
-                    li.appendChild(toggle);
+                    const toggleDiv = document.createElement('div');
+                    toggleDiv.textContent = '❱';
+                    toggle.appendChild(toggleDiv);
+                    span.appendChild(toggle);
                     headerToggles.push(li);
                 }
             }
-
-            // Find the appropriate parent level.
-            while (stack.length > 1 && stack[stack.length - 1].level >= level) {
-                stack.pop();
-            }
+            li.appendChild(span);
 
             const currentParent = stack[stack.length - 1];
             currentParent.ol.appendChild(li);
-
-            // Create new nested ol for potential children.
-            const nestedOl = document.createElement('ol');
-            nestedOl.classList.add('section');
-            const nestedLi = document.createElement('li');
-            nestedLi.appendChild(nestedOl);
-            currentParent.ol.appendChild(nestedLi);
-            stack.push({ level: level, ol: nestedOl });
         }
 
-        activeList.insertBefore(rootLi, activeItem.nextSibling);
+        const activeItemSpan = activeSection.parentElement;
+        activeItemSpan.after(stack[0].ol);
     });
 
     document.addEventListener('DOMContentLoaded', reloadCurrentHeader);

crates/mdbook-html/src/html_handlebars/helpers/toc.rs

@@ -57,39 +57,45 @@ impl HelperDef for RenderToc {
         out.write("<ol class=\"chapter\">")?;
 
         let mut current_level = 1;
+        let mut first = true;
 
         for item in chapters {
-            let (_section, level) = if let Some(s) = item.get("section") {
-                (s.as_str(), s.matches('.').count())
-            } else {
-                ("", 1)
-            };
+            let level = item
+                .get("section")
+                .map(|s| s.matches('.').count())
+                .unwrap_or(1);
 
             // Expand if folding is disabled, or if levels that are larger than this would not
             // be folded.
             let is_expanded = !fold_enable || level - 1 < (fold_level as usize);
 
             match level.cmp(&current_level) {
                 Ordering::Greater => {
-                    while level > current_level {
-                        out.write("<li>")?;
-                        out.write("<ol class=\"section\">")?;
-                        current_level += 1;
-                    }
-                    write_li_open_tag(out, is_expanded, false)?;
+                    // There is an assumption that when descending, it can
+                    // only go one level down at a time. This should be
+                    // enforced by the nature of markdown lists and the
+                    // summary parser.
+                    assert_eq!(level, current_level + 1);
+                    current_level += 1;
+                    out.write("<ol class=\"section\">")?;
+                    write_li_open_tag(out, is_expanded)?;
                 }
                 Ordering::Less => {
                     while level < current_level {
-                        out.write("</ol>")?;
                         out.write("</li>")?;
+                        out.write("</ol>")?;
                         current_level -= 1;
                     }
-                    write_li_open_tag(out, is_expanded, false)?;
+                    write_li_open_tag(out, is_expanded)?;
                 }
                 Ordering::Equal => {
-                    write_li_open_tag(out, is_expanded, !item.contains_key("section"))?;
+                    if !first {
+                        out.write("</li>")?;
+                    }
+                    write_li_open_tag(out, is_expanded)?;
                 }
             }
+            first = false;
 
             // Spacer
             if item.contains_key("spacer") {
@@ -105,6 +111,8 @@ impl HelperDef for RenderToc {
                 continue;
             }
 
+            out.write("<span class=\"chapter-link-wrapper\">")?;
+
             // Link
             let path_exists = match item.get("path") {
                 Some(path) if !path.is_empty() => {
@@ -121,7 +129,7 @@ impl HelperDef for RenderToc {
                     true
                 }
                 _ => {
-                    out.write("<div>")?;
+                    out.write("<span>")?;
                     false
                 }
             };
@@ -142,41 +150,35 @@ impl HelperDef for RenderToc {
             if path_exists {
                 out.write("</a>")?;
             } else {
-                out.write("</div>")?;
+                out.write("</span>")?;
             }
 
             // Render expand/collapse toggle
             if let Some(flag) = item.get("has_sub_items") {
                 let has_sub_items = flag.parse::<bool>().unwrap_or_default();
                 if fold_enable && has_sub_items {
-                    out.write("<a class=\"toggle\"><div>❱</div></a>")?;
+                    // The <div> here is to manage rotating the element when
+                    // the chapter title is long and word-wraps.
+                    out.write("<a class=\"chapter-fold-toggle\"><div>❱</div></a>")?;
                 }
             }
-            out.write("</li>")?;
+            out.write("</span>")?;
         }
-        while current_level > 1 {
-            out.write("</ol>")?;
+        while current_level > 0 {
             out.write("</li>")?;
+            out.write("</ol>")?;
             current_level -= 1;
         }
 
-        out.write("</ol>")?;
         Ok(())
     }
 }
 
-fn write_li_open_tag(
-    out: &mut dyn Output,
-    is_expanded: bool,
-    is_affix: bool,
-) -> Result<(), std::io::Error> {
+fn write_li_open_tag(out: &mut dyn Output, is_expanded: bool) -> Result<(), std::io::Error> {
     let mut li = String::from("<li class=\"chapter-item ");
     if is_expanded {
         li.push_str("expanded ");
     }
-    if is_affix {
-        li.push_str("affix ");
-    }
     li.push_str("\">");
     out.write(&li)
 }

tests/gui/books/heading-nav-folded/book.toml

@@ -0,0 +1,6 @@
+[book]
+title = "heading-nav-folded"
+
+[output.html.fold]
+enable = true
+level = 0