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.

Sign up for GitHub

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

Jump to bottom

Make TOC sections expandable and collapsible by keyboard #1582

Merged
merged 27 commits into from
Feb 12, 2024
Merged

Make TOC sections expandable and collapsible by keyboard #1582

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
85badcb
Consistent focus ring (first pass) (#1549)
gabalafou Nov 13, 2023
920c91d
Resolve current sidebar link notch and focus ring (#1561)
gabalafou Nov 16, 2023
38ef477
Restyle Sphinx Design tabs (#1555)
gabalafou Nov 16, 2023
9803c5e
Fix tabbed panel colors (#1567)
gabalafou Nov 17, 2023
ba86357
aria attributes do not update if user uses mouse instead of keyboard
gabalafou Nov 20, 2023
2c35dac
details/summary, no checkbox
gabalafou Dec 3, 2023
598ebed
clean up
gabalafou Dec 3, 2023
7b43a8d
details["open"]
gabalafou Dec 4, 2023
eda8049
make it work for nav level 0
gabalafou Dec 5, 2023
a009423
Merge branch 'feature-focus' into keyboardable-toc-chevrons
gabalafou Dec 13, 2023
741af85
pull link outside of details tag
gabalafou Dec 19, 2023
b57b376
make it work with show nav level 0
gabalafou Dec 19, 2023
967f598
make level 0 heading clickable
gabalafou Dec 19, 2023
cb6207d
comments
gabalafou Dec 19, 2023
01e5f23
restore .current notches to toc parents
gabalafou Dec 19, 2023
791ff49
fix tests
gabalafou Dec 19, 2023
c76033d
clean up
gabalafou Dec 19, 2023
1ba63b3
more comments
gabalafou Dec 19, 2023
dc567ca
make toggle icon bigger at level 0
gabalafou Dec 19, 2023
4ff5404
comments
gabalafou Dec 19, 2023
5f5faa5
clarify comment
gabalafou Jan 24, 2024
85f2ad1
remove list-captions class from in-page toc styles
gabalafou Jan 24, 2024
312f3b8
Update tests/test_build.py
gabalafou Feb 10, 2024
309c6b0
Update src/pydata_sphinx_theme/toctree.py
gabalafou Feb 10, 2024
123140c
update test for verbose HTML boolean attribute
gabalafou Feb 10, 2024
89eba83
more HTML boolean attribute notation updates
gabalafou Feb 10, 2024
0d8693e
Update comment about link vs non-link HTML structure
gabalafou Feb 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
26 changes: 0 additions & 26 deletions src/pydata_sphinx_theme/assets/styles/components/_navbar-links.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,32 +26,6 @@
@include link-style-text;
}
}

/**
* Togglable expand/collapse
* This is only applicable to the primary sidebar which has these checkboxes
*/
.toctree-checkbox {
position: absolute;
display: none;
}

.toctree-checkbox {
~ ul {
display: none;
}
~ label .fa-chevron-down {
transform: rotate(0deg);
}
}
.toctree-checkbox:checked {
~ ul {
display: block;
}
~ label .fa-chevron-down {
transform: rotate(180deg);
}
}
Comment on lines -29 to -54
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No more of the label/checkbox implementation (replaced in this PR with details/summary)

}

// Don't display the `site navigation` in the header menu
Expand Down
3 changes: 1 addition & 2 deletions src/pydata_sphinx_theme/assets/styles/components/_toc-inpage.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,7 @@ nav.page-toc {
margin-bottom: 1rem;
}

.bd-toc .nav,
.list-caption {
.bd-toc .nav {
.nav {
display: none;

Expand Down
140 changes: 93 additions & 47 deletions src/pydata_sphinx_theme/assets/styles/sections/_sidebar-primary.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
* e.g., between-pages navigation.
*/

$sidebar-padding-right: 1rem;
.bd-sidebar-primary {
display: flex;
flex-direction: column;
Expand All @@ -13,7 +14,7 @@
@include make-col(3);

// Borders padding and whitespace
padding: 2rem 1rem 1rem 1rem;
padding: 2rem $sidebar-padding-right 1rem 1rem;
border-right: 1px solid var(--pst-color-border);
background-color: var(--pst-color-background);
overflow-y: auto;
Expand Down Expand Up @@ -140,61 +141,107 @@
.list-caption {
list-style: none;
padding-left: 0px;
}
li {
position: relative;
// If it has children, add a bit more padding to wrap the content to avoid
// overlapping with the <label>
&.has-children {
> .reference {
padding-right: 30px;

// Level 0 TOC heading is put inside the <summary> tag
// so let the <summary> tag take up more space
li.toctree-l0.has-children {
> details {
> summary {
position: relative;
height: auto;
width: auto;
display: flex;
justify-content: space-between;
align-items: baseline;

.toctree-toggle {
// Prevent toggle icon from getting squished by summary being a
// flexbox
Comment on lines +158 to +159
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just want to say I really appreciate comments like this in the CSS. Makes reviewing so much easier when the intent is clear.

flex: 0 0 auto;

// Make the level 0 chevron icon slightly bigger than descendant
// levels
.fa-chevron-down {
font-size: 1rem;
}
}
}
}
}
}
// Navigation item chevrons
label.toctree-toggle {
position: absolute;
top: 0;
right: 0;
height: 30px;
width: 30px;

cursor: pointer;
li.has-children {
$toctree-toggle-width: 30px;

display: flex;
justify-content: center;
align-items: center;
position: relative;

&:hover {
background: var(--pst-color-surface);
> .reference,
.caption {
margin-right: calc(
$toctree-toggle-width + $focus-ring-width
); // keep clear of the toggle icon
padding-top: 0.25rem; // align caption text with toggle chevron
}

i {
display: inline-block;
font-size: 0.75rem;
text-align: center;
&:hover {
color: var(--pst-color-primary);
> details {
> summary {
// Remove browser default toggle icon
list-style: none;
&::-webkit-details-marker {
display: none;
}

// The summary element is natively focusable, but delegate the focus state to the toggle icon
&:focus-visible {
outline: none;

> .toctree-toggle {
outline: $focus-ring-outline;
outline-offset: -$focus-ring-width; // Prevent right side of focus ring from disappearing underneath the sidebar's right edge
}
}

// Container for expand/collapse chevron icon
.toctree-toggle {
cursor: pointer;

// Position it so that it's aligned with the top right corner of the
// last positioned element, in this case the li.has-children
position: absolute;
top: 0;
right: 0;

// Give it dimensions
width: $toctree-toggle-width;
height: $toctree-toggle-width; // make it square

// Vertically and horizontally center the icon within the container
display: inline-flex;
justify-content: center;
align-items: center;

.fa-chevron-down {
font-size: 0.75rem;
}
}
}

// The section is open/expanded, rotate the toggle icon (chevron) so it
// points up instead of down
&[open] {
> summary {
.fa-chevron-down {
transform: rotate(180deg);
}
}
}
}
}
.label-parts {
width: 100%;
height: 100%;
&:hover {
background: none;
}
i {
width: 30px;
position: absolute;
top: 0.3em; // aligning chevron with text
right: 0em; // aligning chevron to the right
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The other parts of this PR suggest that this class was applied to the label element when styling the toggle icon for a level 0 heading. It's purpose, I think was to make the entire heading clickable. I refactored all of this so that the heading and the icon is put inside a summary tag, which makes the whole heading and the icon clickable.

}
}

/* Between-page links and captions */
nav.bd-links {
margin-right: -$sidebar-padding-right; // align toctree toggle chevrons with right edge of sidebar and allow text to flow closer to the right edge

@include media-breakpoint-up($breakpoint-sidebar-primary) {
display: block;
}
Expand All @@ -213,6 +260,7 @@ nav.bd-links {
padding: 0.25rem 0.65rem;
@include link-sidebar;
box-shadow: none;
margin-right: $focus-ring-width; // prevent the right side focus ring from disappearing under the sidebar right edge

&.reference.external {
&:after {
Expand All @@ -224,11 +272,9 @@ nav.bd-links {
}
}

.current {
> a {
@include link-sidebar-current;
background-color: transparent;
}
.current > a {
@include link-sidebar-current;
background-color: transparent;
}

// Title
Expand Down
108 changes: 75 additions & 33 deletions src/pydata_sphinx_theme/toctree.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -272,10 +272,8 @@ def generate_toctree_html(

# Open the sidebar navigation to the proper depth
for ii in range(int(show_nav_level)):
for checkbox in soup.select(
f"li.toctree-l{ii} > input.toctree-checkbox"
):
checkbox.attrs["checked"] = None
for details in soup.select(f"li.toctree-l{ii} > details"):
details["open"] = "open"

return soup

Expand Down Expand Up @@ -354,8 +352,6 @@ def add_collapse_checkboxes(soup: BeautifulSoup) -> None:
"""Add checkboxes to collapse children in a toctree."""
# based on https://github.com/pradyunsg/furo

toctree_checkbox_count = 0

for element in soup.find_all("li", recursive=True):
# We check all "li" elements, to add a "current-page" to the correct li.
classes = element.get("class", [])
Expand All @@ -364,7 +360,7 @@ def add_collapse_checkboxes(soup: BeautifulSoup) -> None:
if "current" in classes:
parentli = element.find_parent("li", class_="toctree-l0")
if parentli:
parentli.select("p.caption ~ input")[0].attrs["checked"] = ""
parentli.find("details")["open"] = None
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice side effect of using details this is way more readable than the original one


# Nothing more to do, unless this has "children"
if not element.find("ul"):
Expand All @@ -373,40 +369,86 @@ def add_collapse_checkboxes(soup: BeautifulSoup) -> None:
# Add a class to indicate that this has children.
element["class"] = classes + ["has-children"]

# We're gonna add a checkbox.
toctree_checkbox_count += 1
checkbox_name = f"toctree-checkbox-{toctree_checkbox_count}"

# Add the "label" for the checkbox which will get filled.
if soup.new_tag is None:
continue

label = soup.new_tag(
"label", attrs={"for": checkbox_name, "class": "toctree-toggle"}
)
label.append(soup.new_tag("i", attrs={"class": "fa-solid fa-chevron-down"}))
if "toctree-l0" in classes:
# making label cover the whole caption text with css
label["class"] = "label-parts"
element.insert(1, label)

# Add the checkbox that's used to store expanded/collapsed state.
checkbox = soup.new_tag(
"input",
# For table of contents nodes that have subtrees, we modify the HTML so
# that the subtree can be expanded or collapsed in the browser.
#
# The HTML markup tree at the parent node starts with this structure:
#
# - li.has-children
# - a.reference or p.caption
# - ul
#
# Note the first child of li.has-children is p.caption only if this node
# is a section heading. (This only happens when show_nav_level is set to
# 0.)
#
# Now we modify the tree structure in one of two ways.
#
# (1) If the node holds a section heading, the HTML tree will be
# modified like so:
#
# - li.has-children
# - details
# - summary
# - p.caption
# - .toctree-toggle
# - ul
#
# (2) Otherwise, if the node holds a link to a page in the docs:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am struggling to grapple some of the specifics so apologies if this does not make sense at all. Why does it only matter for case 1) that show_nav_level == 0 and not here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. This is fairly complicated to explain.

Let's begin by reviewing the levels of navigation on the site:

  • sections within site (user guide, contributor guide, examples)
  • parts within a section (within contributor guide: contributor guide, team practices, about the project)
  • pages within a part (e.g., contributor guide / contributor guide / get started with development)
  • (optional) sub-pages, and sub-pages of sub-pages, and so on
  • heading within page or sub-page (e.g. contributor guide / contributor guide / get started with development / workflow for contributing changes)

The first level is put in the top nav, all the other levels are put in the left sidebar, except the last level, the within (sub)page level, which is put in the right sidebar.

Normally, when the table of contents is generated for the left sidebar, several nested tree structures using HTML lists of nested lists are created to capture each group of pages and sub-pages within a part, but the parts themselves are rendered outside of these nested list structures.

However, when show_nav_level == 0 the generate_toctree_html() function modifies the HTML so that the section parts become part of the nested list-of-lists structure.

Note that this part of the code you've asked about is in a function called add_collapse_checkboxes. This function goes through every list item in the nested list structure it receives, and if that list item contains a list, then it decorates the HTML so that that list item can be toggled open or closed.

But here's the catch. Unlike the pages and sub-pages (nav level > 0), the section parts (nav level = 0) are not links; there is no URL and no page in the site that maps to any section part.

This is the key issue. Every node in the sidebar table of contents is a link except for the section headings, which are all 0th-level nodes in the sidebar (or 1st-level nodes if you consider the "Section Navigation" heading at the top to be the root node for the entire sidebar TOC).

To reiterate, the key difference is that section nodes are not links, whereas page and sub-page nodes are. When a node is a link, we have to think about whether we want to put the link inside the summary tag or not. Why? If we put a link in a summary tag, we introduce an ambiguity: is the link click to expand/collapse or to navigate to a new page? I made the decision that the link click should be a link click and not an expand/collapse. However, I saw no reason why the user shouldn't be able to click the text of the section part to expand/collapse the details/summary, since a section part is not a link.

When show_nav_level > 0, the section parts are rendered outside of the list-of-lists structure, so they are never even visited by this add_collapse_checkboxes function.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just updated the code comment here. Hopefully, it's slightly clearer, or at least more precise, now.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the in-depth explanation; this makes sense. I went through the rest of the PR again and could not spot any obvious gotchas. So, I am happy to approve it as is.

That being said I know @drammock is a lot more familiar with the whole parsing/HTML generation side of things so I want to give him the chance to go over this or I can merge in a couple of days.

#
# - li.has-children
# - a.reference
# - details
# - summary
# - .toctree-toggle
# - ul
#
# Why the difference? In the first case, the TOC section heading is not
# a link, but in the second case it is. So in the first case it makes
# sense to put the (non-link) text inside the summary tag so that the
# user can click either the text or the .toctree-toggle chevron icon to
# expand/collapse the TOC subtree. But in the second case, putting the
# link in the summary tag would make it unclear whether clicking on the
# link should expand the subtree or take you to the link.

# Create <details> and put the entire subtree into it
details = soup.new_tag("details")
details.extend(element.contents)
element.append(details)

# Hoist the link to the top if there is one
toc_link = element.select_one("details > a.reference")
if toc_link:
element.insert(0, toc_link)

# Create <summary> with chevron icon
summary = soup.new_tag("summary")
span = soup.new_tag(
"span",
attrs={
"type": "checkbox",
"class": ["toctree-checkbox"],
"id": checkbox_name,
"name": checkbox_name,
"class": "toctree-toggle",
"role": "presentation", # This element and the chevron it contains are purely decorative; the actual expand/collapse functionality is delegated to the <summary> tag
},
)
span.append(soup.new_tag("i", attrs={"class": "fa-solid fa-chevron-down"}))
summary.append(span)

# if this has a "current" class, be expanded by default
# (by checking the checkbox)
if "current" in classes:
checkbox.attrs["checked"] = ""
# Prepend section heading (if there is one) to <summary>
collapsible_section_heading = element.select_one("details > p.caption")
if collapsible_section_heading:
# Put heading inside summary so that the heading text (and chevron) are both clickable
summary.insert(0, collapsible_section_heading)

# Prepend <summary> to <details>
details.insert(0, summary)

element.insert(1, checkbox)
# If this TOC node has a "current" class, be expanded by default
# (by opening the details/summary disclosure widget)
if "current" in classes:
details["open"] = "open"


def get_local_toctree_for(
Expand Down