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

The section may require two clicks for getting collapsed when the navigation.expand feature is on. #2386

Closed
4 of 5 tasks
Labels
bug Issue reports a bug resolved Issue is resolved, yet unreleased if open

Comments

@cainmagi
Copy link

cainmagi commented Mar 3, 2021

Dear author,

I've found a bug and checked that ...

  • ... the problem doesn't occur with the default MkDocs template
    • Not applied, because this feature does not exist in default MkDocs template.
  • ... the problem is not in any of my customizations (CSS, JS, template)
  • ... the documentation does not mention anything about my problem
  • ... there are no open or closed issues that are related to my problem

Description

There is a bug in the following feature:

features:
  - navigation.expand

When this feature is on, the section (expanded in default) may require two clicks for getting collapsed.

Expected behavior

Every time I refresh the page, the expanded section should only require one click for collapsing.

Actual behavior

The bug is:

  • Although all items in the navbar are expanded. A section would not get collapse. It requires me to click it for the second time, then it would behave normally.
  • Such a bug would not exist if the section is the parent of the current page.

image

I guess that the bug exists in the JS codes, it may not take the expand case into consideration.

Steps to reproduce the bug

Please use your current repository, and change the mkdocs.yml like this:

At line 55

  features:
    # - navigation.instant
-   - navigation.sections
+   # - navigation.sections
    - navigation.tabs
+   - navigation.expand

At line 159

-   - Changing the fonts: setup/changing-the-fonts.md
-   - Changing the language: setup/changing-the-language.md
-   - Changing the logo and icons: setup/changing-the-logo-and-icons.md
-   - Setting up navigation: setup/setting-up-navigation.md
-   - Setting up site search: setup/setting-up-site-search.md
-   - Setting up site analytics: setup/setting-up-site-analytics.md
+   - Changing test:
+     - Changing the fonts: setup/changing-the-fonts.md
+     - Changing the language: setup/changing-the-language.md
+     - Changing the logo and icons: setup/changing-the-logo-and-icons.md
+   - Setting up test:
+     - Setting up navigation: setup/setting-up-navigation.md
+     - Setting up site search: setup/setting-up-site-search.md
+     - Setting up site analytics: setup/setting-up-site-analytics.md

The modified file is attached here:

mkdocs.zip

I am not very familiar with your theme. Please fix it if you have time. Thank you!

Package versions

  • Python: 3.7.1
  • MkDocs: 1.1.2
  • Material: 7.0.3

Project configuration

Please download the mkdocs.yml in the following link:

mkdocs.zip

System information

  • OS:
    • Edition: Windows 10 Home
    • Version: 20H2
    • Installed on: 12/‎15/‎2020
    • OS build: 19042.804
    • Experience: Windows Feature Experience Pack 120.2212.551.0
  • Browser: Chrome 88.0.4324.190
@cainmagi
Copy link
Author

cainmagi commented Mar 3, 2021

I find the following modification would fix the bug. But I am not sure whether it is a correct solution and would not exert other issues:

Change the template: partials/nav-item.html

 {#-
  This file was automatically generated - do not edit
 -#}
{% macro render(nav_item, path, level) %}
  {% set class = "md-nav__item" %}
  {% if nav_item.active %}
    {% set class = class ~ " md-nav__item--active" %}
  {% endif %}
  {% if nav_item.children %}
    {% if "navigation.sections" in features and level == 1 + (
      "navigation.tabs" in features
    ) %}
      {% set class = class ~ " md-nav__item--section" %}
    {% endif %}
    <li class="{{ class }} md-nav__item--nested">
      {% set checked = "checked" if nav_item.active %}
      {% if "navigation.expand" in features and not checked %}
-       <input class="md-nav__toggle md-toggle" data-md-toggle="{{ path }}" data-md-state="indeterminate" type="checkbox" id="{{ path }}" checked>
+       <input class="md-nav__toggle md-toggle" data-md-toggle="{{ path }}" type="checkbox" id="{{ path }}" checked>
      {% else %}
        <input class="md-nav__toggle md-toggle" data-md-toggle="{{ path }}" type="checkbox" id="{{ path }}" {{ checked }}>
      {% endif %}
      <label class="md-nav__link" for="{{ path }}">

In my explanation, the bug is triggered by the following reason.

  • When expand feature is on, those expanded sections are set data-md-state="indeterminate".
  • The JS in assets/javascripts/patches/indeterminate/index.ts removes the checked state of those expanded sections with data-md-state="indeterminate" and sets them indeterminate.
  • When users click on these sections, the state indeterminate is transformed to checked. But from the user's view, there is nothing happens. This is why the first click would not make the section collapsed.

I do not know why you need to set it "indeterminate". I think this configuration would only cause unwanted behaviors. If there is any reasons for those such an "indeterminate" state is necessary, please let me know. Thank you!

@squidfunk
Copy link
Owner

squidfunk commented Mar 4, 2021

Thanks for reporting! That's indeed a bug. The problem originates from the fact that when clicking on a checkbox with state indeterminate, the checkbox is in a checked and not unchecked state, where it should be unchecked first. The indeterminate state must be used (as opposed to checked) because when all checkboxes would be checked, all layers would be expanded in the mobile view.

@squidfunk squidfunk added the bug Issue reports a bug label Mar 4, 2021
@squidfunk
Copy link
Owner

Fixed in f05c34e.

@squidfunk squidfunk added the resolved Issue is resolved, yet unreleased if open label Mar 4, 2021
@cainmagi
Copy link
Author

cainmagi commented Mar 4, 2021

Dear author,

Wow, you are quite efficient. Thank you!

cainmagi

@cainmagi cainmagi closed this as completed Mar 4, 2021
@squidfunk
Copy link
Owner

Released as 7.0.4.

@cainmagi
Copy link
Author

cainmagi commented Mar 5, 2021

I am sorry to find that after the new version updated, although the bug of double clicks gets fixed in desktop mode. The mobile view requires double clicks for expanding each section. I guess that this problem is caused by the following reason:

  • When clicking the section in desktop view, the indeterminate mode should be switched to unchecked to make it collapsed, this is exactly what we have now.
  • However, when clicking the section in mobile view, the indeterminate should be switched to checked to make it expanded.

Certainly, the two behaviors conflict with each other. I suggest that we should render different navigation bars for desktop view and mobile view respectively. If we do that, we could benefit from:

  • We do not need to maintain the indeterminate flag.
  • When we enter the mobile view by resizing the browser, what have done on the navbar would not influence the navbar in mobile view.

Please consider this suggestion. I think implementing two navbars, and let them controlled by css would be easier to maintain the indeterminate state and check the screen size by javascript.

@cainmagi cainmagi reopened this Mar 5, 2021
@squidfunk
Copy link
Owner

You're right! Thanks for reporting and re-opening. It's a tricky topic 😅

@squidfunk
Copy link
Owner

I think I've got a fix in 5d0cae7 – could you check the lastest master (see how to install from git)?. Now, on desktop (nav in sidebar), the section will collapse on first click and on mobile/tablet (nav in drawer) it will open on first click. Note that it's correct that when you open/close a section on mobile, it's also open/closed on desktop – that's the normal behavior.

@cainmagi
Copy link
Author

cainmagi commented Mar 5, 2021

I have checked it with my cell phone. It works well now.
Thank you!

This was referenced Mar 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment