Redundant `search-button-field` in navbar causes toggle primary/secondary buttons to fail

[hwhsu1231]

Problem Description

Hello Sphinx Book Theme Community,

I believe some people have recently noticed the following two severe bugs in the new version of the sphinx-book-theme:

1. An extra search-button-field appears by default in the navbar between the announcement banner and the doc page.
2. The expand/collapse functionality of the toggle primary/secondary buttons in desktop or mobile layout stops working.

Actually, these two bugs have already been reported by others:

1. The 1st bug was reported in #986.
2. The 2nd bug was reported in #917, #935, and #988.

And some people have even tried submitting PRs to fix them:

1. PR #987 to fix issue #986 (the 1st bug).
2. PR #991 to fix issue #935 (the 2nd bug).

After thoroughly examining the source code, I have confirmed the root causes of these two bugs. Simply put:

1. The 1st bug is caused by changes in the new version of the upstream pydata-sphinx-theme.
2. The 2nd bug is due to an oversight in the original design of the downstream sphinx-book-theme.

Furthermore, there is a clear causal relationship between these two bugs:

Changes in the new version of the PyData theme caused the default value of navbar_persistent in the Book theme to become ["search-button-field"]. This exposed the fact that when designing the expand/collapse functionality for the primary/secondary toggle buttons, the Book theme apparently failed to consider whether it would function properly "when there are elements present in the navbar".

When do these two bugs show up?

After testing with the following commands, we can confirm that the two bugs mentioned above only start appearing from version v1.3.0:

Click to expand the testing commands

# Build the v1.4.0 documentation
VERSION=v1.4.0
git clone --branch=$VERSION [https://github.com/executablebooks/sphinx-book-theme.git](https://github.com/executablebooks/sphinx-book-theme.git) sbt-$VERSION
cd sbt-$VERSION
uv run --python 3.12 --link-mode=copy --extra doc sphinx-build -b html docs docs/_build/html
firefox docs/_build/html/index.html
cd ..

# Build the v1.3.0 documentation
VERSION=v1.3.0
git clone --branch=$VERSION [https://github.com/executablebooks/sphinx-book-theme.git](https://github.com/executablebooks/sphinx-book-theme.git) sbt-$VERSION
cd sbt-$VERSION
uv run --python 3.12 --link-mode=copy --extra doc sphinx-build -b html docs docs/_build/html
firefox docs/_build/html/index.html
cd ..

# Build the v1.2.0 documentation
VERSION=v1.2.0
git clone --branch=$VERSION [https://github.com/executablebooks/sphinx-book-theme.git](https://github.com/executablebooks/sphinx-book-theme.git) sbt-$VERSION
cd sbt-$VERSION
uv run --python 3.12 --link-mode=copy --extra doc sphinx-build -b html docs docs/_build/html
firefox docs/_build/html/index.html
cd ..

# Build the v1.1.4 documentation
VERSION=v1.1.4
git clone --branch=$VERSION [https://github.com/executablebooks/sphinx-book-theme.git](https://github.com/executablebooks/sphinx-book-theme.git) sbt-$VERSION
cd sbt-$VERSION
uv run --python 3.12 --link-mode=copy --extra doc --reinstall sphinx-build -b html docs docs/_build/html
firefox docs/_build/html/index.html

This is primarily caused by commit 8e676945 in the PyData theme (which first appeared in the 0.17.1 release). Furthermore, Book theme 1.3.0 officially began depending on PyData 0.17.1:

Book Theme	PyData Theme	Status
1.4.0	0.20.0	FAILED
1.3.0	0.17.1	FAILED
1.2.0	0.16.1	PASSED
1.1.4	0.15.4	PASSED

[hwhsu1231]

Why does the 1st bug happen?

For the Book theme, it does not need the navbar from the PyData theme by default. Therefore, we can see that starting from the v1.0.0 tag, the default values of the navbar_<start|center|end|persistent> theme options in theme.conf are all set to empty:

sphinx-book-theme/src/sphinx_book_theme/theme/sphinx_book_theme/theme.conf

Lines 32 to 37 in 6601a2f

	# Header / navbar.
	# Over-ride the PST navbar components (users could provide their own if they wish)
	navbar_start =
	navbar_center =
	navbar_end =
	navbar_persistent =

This overriding approach remained effective up until Book v1.2.0. The reason this override worked is because the upstream PyData theme previously (e.g., cd00a86) directly set the default values for these four theme options in theme.conf:

https://github.com/pydata/pydata-sphinx-theme/blob/cd00a86a87aba3903543c00c5ce5f009c436e0b1/src/pydata_sphinx_theme/theme/pydata_sphinx_theme/theme.conf#L43-L46

However, starting from commit 8e67694, the default value of the navbar_persistent theme option is now determined in __init__.py based on the newly added disable_search theme option:

# If the user hasn't explicitly set navbar_persistent, default it based on
# disable_search: show the search button field unless search is disabled.
if "navbar_persistent" not in theme_options:
    if theme_options.get("disable_search", False):
        navbar_persistent = []
    else:
        navbar_persistent = ["search-button-field"]
    theme_options["navbar_persistent"] = navbar_persistent

As a result, unless disable_search is explicitly set to True in conf.py, the default value of the navbar_persistent theme option will always be ["search-button-field"], even though the Book theme sets navbar_persistent = in its theme.conf.

Since commit 8e67694 was first released in PyData theme version 0.17.1, and the Book theme only updated its dependency to PyData 0.17.1 starting from version 1.3.0, this explains why these two bugs only begin to appear from Book theme version 1.3.0 onwards.

How to fix the 1st bug?

Once we understand why the first bug happens, its fix is not difficult to conceive: find a way to properly initialize navbar_persistent as an empty list within the Book theme. In fact, PR #987 submitted by @flying-sheep already perfectly fixes the first bug. Therefore, we just need to wait for that PR to be merged.

[hwhsu1231]

Why does the 2nd bug happen?

In the upstream PyData theme, sections/header.html includes the following two toggle buttons:

• <button class="pst-navbar-icon sidebar-toggle primary-toggle"></button>
• <button class="pst-navbar-icon sidebar-toggle secondary-toggle"></button>

Their purpose is to display primary/secondary toggle buttons on both sides of the header when the layout switches to a mobile layout (narrow screens), allowing users to expand or collapse the primary/secondary sidebars.

Originally, both the PyData theme and the Book theme located these elements by querying elements with the primary-toggle and secondary-toggle classes using the document.querySelector() method:

const primaryToggle = document.querySelector(".primary-toggle");
const secondaryToggle = document.querySelector(".secondary-toggle");

However, ever since the 1st bug appeared, we can observe that once the navbar is rendered, two buttons with the primary-toggle/secondary-toggle classes simultaneously appear on the entire page. Taking the primary-toggle class as an example:

• One comes from sections/header.html in the PyData theme.
• One comes from components/toggle-primary-sidebar.html in the Book theme.

Consequently, whether it is setupMobileSidebarKeyboardHandlers() from the PyData theme or fixSidebarToggle() from the Book theme, their calls to document.querySelector(".primary-toggle") will only find the first button with the primary-toggle class originating from the PyData theme. The same applies to the secondary-toggle class.

This causes the toggle buttons provided by the Book theme to fail to properly bind their expected toggle functions.

How to fix the 2nd bug?

After understanding the ins and outs of how these two bugs arise, we know that once the 1st bug is fixed—ensuring that the default value of navbar_* remains always empty—the Book theme's current toggle primary/secondary buttons will work normally. However, even if the 1st bug is fixed, if a user attempts to modify navbar_* during customization, the problem of the second bug will still resurface.

Frankly speaking, there are many ways to resolve the second bug. Currently, PR #991 submitted by @BetterAndBetterII can only fix:

• Toggle primary button functionality under the desktop layout.
• Toggle primary button functionality under the mobile layout.
• Toggle secondary button functionality under the mobile layout.

Although continuing to modify along the lines of that PR would indeed fix the remaining two toggling features, it is predictable that this would introduce increasingly complex changes.

Therefore, I believe the most reliable solution is to override sections/header.html in the Book theme without PyData's toggle primary/secondary buttons. The specific implementation is as follows: copy a copy of sections/header.html from the corresponding version of the PyData theme to the corresponding location in the Book theme, and then remove those two toggle buttons:

  {% if theme_navbar_start or theme_navbar_center or theme_navbar_end or theme_navbar_persistent %}
  <div class="bd-header__inner bd-page-width">
-   <button class="pst-navbar-icon sidebar-toggle primary-toggle" aria-label="{{ _('Site navigation') }}">
-     <span class="fa-solid fa-bars"></span>
-   </button>
    {% set navbar_start, navbar_class, navbar_align = navbar_align_class() %}
    {% if theme_navbar_start %}
    <div class="{{ navbar_start }} navbar-header-items__start">
      {% for navbar_item in theme_navbar_start %}
        <div class="navbar-item">{% include navbar_item %}</div>
      {% endfor %}
    </div>
    {% endif %}
    <div class="{{ navbar_class }} navbar-header-items">
      {% if theme_navbar_center %}
      <div class="{{ navbar_align }} navbar-header-items__center">
        {% for navbar_item in theme_navbar_center %}
          <div class="navbar-item">{% include navbar_item %}</div>
        {% endfor %}
      </div>
      {% endif %}
      {% if theme_navbar_end or theme_navbar_persistent %}
      <div class="navbar-header-items__end">
        {% for navbar_item in theme_navbar_persistent %}
          <div class="navbar-item navbar-persistent--container">
            {% include navbar_item %}
          </div>
        {% endfor %}
        {% for navbar_item in theme_navbar_end %}
          <div class="navbar-item">{% include navbar_item %}</div>
        {% endfor %}
      </div>
      {% endif %}
    </div>
    {# A search button to show up only on mobile #}
    {% for navbar_item in theme_navbar_persistent %}
      <div class="navbar-persistent--mobile">
        {%- include navbar_item %}
      </div>
    {% endfor %}
-
-   {% if not remove_sidebar_secondary and secondary_sidebar_items %}
-     <button class="pst-navbar-icon sidebar-toggle secondary-toggle" aria-label="{{ _('On this page') }}">
-       <span class="fa-solid fa-outdent"></span>
-     </button>
-   {% endif %}
  </div>
  {% endif %}

In this way, we can ensure that when navbar_* is rendered, only a single set of .primary-toggle/.secondary-toggle buttons will appear on the entire page.

[flying-sheep]

In fact, PR #987 submitted by @flying-sheep already perfectly fixes the first bug. Therefore, we just need to wait for that PR to be merged.

I hope that happens soon …