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

Make TOC sections expandable and collapsible by keyboard #1582

Merged
merged 27 commits into from
Feb 12, 2024
Merged
Show file tree
Hide file tree
Changes from 17 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
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
139 changes: 92 additions & 47 deletions src/pydata_sphinx_theme/assets/styles/sections/_sidebar-primary.scss
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,106 @@
.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;

.nav {
display: block;
}
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'm not totally sure what I'm doing here. The display: block rule here overrides a display: none rule on line 12 in _toc-inpage.scss, but I don't fully understand what the rules in _toc_inpage.scss are about.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Going by the comments in toc-inpage.scss that display: none seems to be related to https://pydata-sphinx-theme.readthedocs.io/en/latest/user_guide/navigation.html#collapse-entire-toctree-captions-parts

so that the user can make those collapsible or specify the level through the config.py

Copy link
Collaborator Author

@gabalafou gabalafou Jan 24, 2024

Choose a reason for hiding this comment

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

I dig some digging on this. I decided that it probably makes most sense to remove the .list-caption selector from the in-page-TOC stylesheet, which is what I did in my latest commit, 85f2ad1.

Semantically, I don't think that the .list-caption class (introduced in #594) should have ever been added to that stylesheet. As far as I can tell, the class is only applied in the left sidebar, whereas the in-page-TOC stylesheet is for the right sidebar. So why was it added to that stylesheet? I'm not sure. Maybe it was to pick up some of the other rules in that block of styles? In any case, it doesn't seem necessary now, and removing .list-caption from that stylesheet allows me to remove the display: block override, as well as make that stylesheet more semantically coherent.

As for your hunch, @trallard, I think it's close. It looks to me that the CSS block in the in-page-TOC stylesheet is related to show_toc_level, which is similar to but different from show_nav_level. I think the first one applies to the right sidebar whereas the second applies to the left. See #256.

Copy link
Collaborator Author

@gabalafou gabalafou Jan 24, 2024

Choose a reason for hiding this comment

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

Here's a screenshot of the website with:

  • .list-caption removed from _toc-inpage.scss
  • show_nav_level = 0
  • show_toc_level = 4

It looks right to me. I also clicked around, and things looked right. As far as I can tell, the .list-caption is only used in the left sidebar and only when show_nav_level is set to 0.


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

> .toctree-toggle {
flex: 0 0 auto;
}
}
}
}
// 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);
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 {
// Get rid of the default toggle icon
> summary {
position: absolute;
top: 0;
right: 0;

// Remove browser default details/summary icon
list-style: none;
&::-webkit-details-marker {
display: none;
}

&:hover {
> .toctree-toggle {
background: var(--pst-color-surface);
}
}

&:focus-visible {
outline: none;

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

// Navigation item chevrons
.toctree-toggle {
cursor: pointer;

display: inline-flex;
justify-content: center;
align-items: center;
width: 30px;
height: 30px;

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

.toctree-l10 & {
.fa-chevron-down {
font-size: inherit;
}
}
}
}

// 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 +259,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 +271,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
71 changes: 38 additions & 33 deletions src/pydata_sphinx_theme/toctree.py
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"] = None

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,49 @@ 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",
# Modify the tree so that it looks like:
#
# li.has-children
# a.reference ~ span.toctree-toggle
# > details
# > summary
# >
# > ul

# Create <details> and subtree into it
details = soup.new_tag("details")
details.extend(element.contents)
element.append(details)
toc_link = element.select_one("details > a.reference")
if toc_link:
element.insert(
0, toc_link
) # Move link outside to make it a clearer when focus is on link versus on the summary expand/collapse

# Create <summary> with chevron icon
summary = soup.new_tag("summary")
collapsible_section_heading = element.select_one("details > p.caption")
if collapsible_section_heading:
# Put level 0 heading inside summary so that the heading text (and chevron) are both clickable
summary.append(collapsible_section_heading)
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)
details.insert(0, summary)

# if this has a "current" class, be expanded by default
# (by checking the checkbox)
# If this has a "current" class, be expanded by default
# (by opening the details/summary disclosure widget)
if "current" in classes:
checkbox.attrs["checked"] = ""

element.insert(1, checkbox)
details["open"] = None
gabalafou marked this conversation as resolved.
Show resolved Hide resolved


def get_local_toctree_for(
Expand Down
18 changes: 9 additions & 9 deletions tests/test_build.py
Original file line number Diff line number Diff line change
Expand Up @@ -418,7 +418,7 @@ def test_sidebars_nested_page(sphinx_build_factory, file_regression) -> None:

subindex_html = sphinx_build.html_tree("section1/subsection1/page1.html")

# For nested (uncollapsed) page, the label included `checked=""`
# For nested (uncollapsed) page, the disclosure widget should be open
sidebar = subindex_html.select("nav.bd-docs-nav")[0]
file_regression.check(sidebar.prettify(), extension=".html")

Expand Down Expand Up @@ -463,7 +463,6 @@ def test_sidebars_show_nav_level0(sphinx_build_factory) -> None:
# part li
assert "toctree-l0 has-children" in " ".join(li[0].attrs["class"])
assert "caption-text" in li[0].select("p span")[0].attrs["class"]
assert "label-parts" in li[0].find("label").attrs["class"]

# basic checks on other levels
assert "toctree-l1 has-children" in " ".join(li[1].attrs["class"])
Expand All @@ -473,12 +472,13 @@ def test_sidebars_show_nav_level0(sphinx_build_factory) -> None:
subsection_html = sphinx_build.html_tree("section1/subsection1/index.html")
sidebar = subsection_html.select("nav.bd-docs-nav")[0]

# get all input elements
input_elem = sidebar.select("input")
# get all <details> elements
details_elem = sidebar.select("details")
assert len(details_elem)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Assuming we want to ensure the list is not empty I'd suggest

Suggested change
assert len(details_elem)
assert len(details_elem)>0, "The list must have at least one element"

gabalafou marked this conversation as resolved.
Show resolved Hide resolved

# all input elements should be collapsed in this page
for ii in input_elem:
assert "checked" in ii.attrs
# all <details> elements should be open in this page
for ii in details_elem:
assert "open" in ii.attrs


def test_included_toc(sphinx_build_factory) -> None:
Expand Down Expand Up @@ -736,8 +736,8 @@ def test_show_nav_level(sphinx_build_factory) -> None:
# Both the column alignment and the margin should be changed
index_html = sphinx_build.html_tree("section1/index.html")

for checkbox in index_html.select("li.toctree-l1.has-children > input"):
assert "checked" in checkbox.attrs
for details_elem in index_html.select("li.toctree-l1.has-children > details"):
assert "open" in details_elem.attrs


# the switcher files tested in test_version_switcher_error_states, not all of them exist
Expand Down
2 changes: 1 addition & 1 deletion tests/test_build/math_header_item.html
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
<li class="nav-item current active">
<li class="nav-item pst-header-nav-item current active">
<a class="nav-link nav-internal" href="#">
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 didn't add this class in this PR, but this PR branches off of the feature-focus branch, which added this class but didn't update the tests yet.

Page
<span class="math notranslate nohighlight">
Expand Down