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

Resolve roles in navigation #56

Open
mhostetter opened this issue Apr 1, 2022 · 41 comments
Open

Resolve roles in navigation #56

mhostetter opened this issue Apr 1, 2022 · 41 comments
Labels
feat: ToC Related to Table of Contents feat: upstream a feature that exists upstream but is not well supported in sphinx-immaterial

Comments

@mhostetter
Copy link
Contributor

I noticed that Sphinx roles are not applied in the navigation tab or content tab headings. Is there anything we can do about that? Is there a way to run a Sphinx role processor on the the titles or captions?

Content tabs

Here I tried to add an inline literal to a content tab heading. I also tried to add an octicon from the sphinx-design extension.
Potentially the octicon did get converted to a SVG but isn't displaying as such?

    Examples
    --------
    .. md-tab-set::

        .. md-tab-item:: n = 14

            Content one...

        .. md-tab-item:: ``n = 15``

            Content two...

        .. md-tab-item:: :octicon:`graph` Graphs

            Content three...

renders as

image

HTML
<div class="tabbed-labels docutils">
    <label class="tabbed-label" for="__tabbed_1_1">
        n = 14
    </label>
    <label class="tabbed-label" for="__tabbed_1_2">
        &lt;literal&gt;n = 15&lt;/literal&gt;
    </label>
    <label class="tabbed-label" for="__tabbed_1_3">
        &lt;raw format=”html” xml:space=”preserve”&gt;&lt;svg version=”1.1” width=”1.0em” height=”1.0em”
        class=”sd-octicon sd-octicon-graph” viewBox=”0 0 16 16” aria-hidden=”true”&gt;&lt;path fill-rule=”evenodd”
        d=”M1.5 1.75a.75.75 0 00-1.5 0v12.5c0 .414.336.75.75.75h14.5a.75.75 0 000-1.5H1.5V1.75zm14.28 2.53a.75.75 0
        00-1.06-1.06L10 7.94 7.53 5.47a.75.75 0 00-1.06 0L3.22 8.72a.75.75 0 001.06 1.06L7 7.06l2.47 2.47a.75.75 0
        001.06 0l5.25-5.25z”&gt;&lt;/path&gt;&lt;/svg&gt;&lt;/raw&gt; Graphs
    </label>
</div>

Navigation tabs

Here I tried to add an octicon to a navigation tab, but it didn't render as expected.

.. toctree::
   :caption: Getting Started
   :hidden:

   getting-started.rst

.. toctree::
   :caption: :octicon:`info` Basic Usage
   :hidden:

   basic-usage/galois-field-classes.rst
   basic-usage/compilation-modes.rst
   basic-usage/field-element-representation.rst
   basic-usage/array-creation.rst
   basic-usage/array-arithmetic.rst
   basic-usage/linear-algebra.rst
   basic-usage/poly-creation.rst
   basic-usage/poly-arithmetic.rst

.. toctree::
   :caption: Tutorials
   :hidden:

renders as

image

HTML
<ul class="md-tabs__list">
    <li class="md-tabs__item">
        <a href="getting-started.html" class="md-tabs__link">
            <span class="md-ellipsis">Getting Started</span>
        </a>
    </li>
    <li class="md-tabs__item">
        <a href="basic-usage/galois-field-classes.html" class="md-tabs__link">
            <span class="md-ellipsis">:<wbr>octicon:<wbr>`info` Basic Usage</span>
        </a>
    </li>
    <li class="md-tabs__item">
        <a href="tutorials/intro-to-prime-fields.html" class="md-tabs__link">
            <span class="md-ellipsis">Tutorials</span>
        </a>
    </li>
    ...
</ul>
@jbms
Copy link
Owner

jbms commented Apr 1, 2022

@2bndy5 can probably address the content tabs better than I can.

As far as the toc, I'm not sure if the :caption: is normally supposed to contain rST or if it is intended to be plain text. If the latter we might not want to change the meaning, at least by default, to avoid an incompatibility with other sphinx themes. We might instead want a way to explicitly opt in.

Separately, though, page/section titles can contain rST and normally in sphinx that does result in styling of the sidebar toc. I intentionally converted all titles to plain text when generating the navigation because I think in many cases it is preferable not to have special styling in the toc. For example if you have a literal block in a title, I think it can look strange to have the literal styling in the toc. It is also tricky to define css rules so that things like literal styles display in a reasonable way inside the yoc. But I can see how icons could be desired and wouldn't have as many difficulties. I'm certainly open to suggestions for how to improve things.

@mhostetter
Copy link
Contributor Author

I agree that literals in a TOC (eg, classes/functions in an API reference) could be ugly. I'm not so worried about the inline literal case. When discovering I couldn't inline n = 14 in the content tab, I was ok with just using normal text styling.

However, the octicons would be nice to add IMHO. I would be happy with adding an exception (or special processing) to only enable :octicon: from sphinx-design or another extension. I think those icons could add a little flair. 🎉

@2bndy5
Copy link
Collaborator

2bndy5 commented Apr 1, 2022

I can try to fix the content tabs so that inline RST roles should work. There may be some roles that you shouldn't need to use in the labels for a content tab (like a ref). I'm surprised the octicon didn't work already because it does interpret the label as inline text (I know literals work in content tab labels).

If they ever get around to publishing the next release, sphinx-design will have 10000+ inline icons from the Google material icon fonts repo. I submitted this feature hoping we could use it abusively, like using an inline icon in a page's header title (that would hopefully show in the main nav menu also).

@2bndy5
Copy link
Collaborator

2bndy5 commented Apr 1, 2022

I was able to get the roles parsed properly when treating the raw text as a separate paragraph. This results in a automatic <p> element that triggers builtin CSS:
image

current working patch

from

# add tab label
textnodes, _ = self.state.inline_text(self.arguments[0], self.lineno)
tab_label = nodes.rubric(
self.arguments[0], *textnodes, classes=["tabbed-label"]
)
self.add_name(tab_label)
tab_item += tab_label

into

from docutils.statemachine import StringList

        # add tab label
        tab_label = nodes.rubric(self.arguments[0], classes=["tabbed-label"])
        label_text = StringList(initlist=[self.arguments[0]])
        self.state.nested_parse(label_text, self.content_offset, tab_label)
        self.add_name(tab_label)
        tab_item += tab_label

This new CSS rule fixed the <p> tag problem:

label.tabbed-label > p {
  margin: 0;
}

@jbms
Copy link
Owner

jbms commented Apr 2, 2022

Instead of using nested_parse you should use inline_text as in md_admonition.py:

textnodes, messages = self.state.inline_text(title_text, self.lineno)

@2bndy5
Copy link
Collaborator

2bndy5 commented Apr 2, 2022

That's what it is using now; it doesn't seem to handle parsing roles. unless I appended the children improperly somehow.

@2bndy5
Copy link
Collaborator

2bndy5 commented Apr 2, 2022

I'm also looking into a way to strip the <p> element from tab label output (which better suites upstream CSS).

@jbms
Copy link
Owner

jbms commented Apr 2, 2022

I do believe inline_text is intended to parse rST --- I'll try to figure out what is happening.

@jbms
Copy link
Owner

jbms commented Apr 2, 2022

Actually I'm rather confused --- roles already seem to work fine with content tabs:

        .. md-tab-set::

            .. md-tab-item:: Customized content ``n = 4``
                :class: custom-tab-item-style

                This content could be styled differently from other page content.

            .. md-tab-item:: Custom CSS :math:`n = 3`

                .. literalinclude:: _static/extra_css.css
                    :language: css
                    :start-at: /* *********************** custom-tab-item-style
                    :end-before: /* ************************* inline icon stuff

content-tab-roles

@2bndy5
Copy link
Collaborator

2bndy5 commented Apr 2, 2022

That's what I thought as well, but I was able to reproduce the issue (not using main branch right now though).

@jbms
Copy link
Owner

jbms commented Apr 2, 2022

Maybe it depends on the sphinx or docutils version?

@jbms
Copy link
Owner

jbms commented Apr 2, 2022

I was using Sphinx 4.4.0 and docutils 0.17.1

@2bndy5
Copy link
Collaborator

2bndy5 commented Apr 2, 2022

I didn't know sphinx allowed anything newer than docutils v16

@2bndy5
Copy link
Collaborator

2bndy5 commented Apr 2, 2022

I'm using docutils 0.17.1 with sphinx v4.4.0. I'm switching to main to see if it reproduces there.

@2bndy5
Copy link
Collaborator

2bndy5 commented Apr 2, 2022

using the main branch, I was able to partially fix the issue by making sure the label's text started with literal text (not a inline interpreted text role). But notice how it still doesn't parse the octicon role:
image
Rather the octicon:graph acts like a cross-reference to the tab-set (I think)

Examples
--------

.. md-tab-set::

    .. md-tab-item:: n = 14

        Content one...

    .. md-tab-item:: when ``n = 15``

        Content two...

    .. md-tab-item:: A :octicon:`graph` Graphs

        Content three...

@2bndy5
Copy link
Collaborator

2bndy5 commented Apr 2, 2022

The math role works also, but I think that's getting transformed at page load.
image

@jbms
Copy link
Owner

jbms commented Apr 2, 2022

If you add "sphinx_design" to the list of extensions in conf.py then octicon works.

There does indeed appear to be a bug though in the case that the label starts out with a special role.

@2bndy5
Copy link
Collaborator

2bndy5 commented Apr 2, 2022

If you add "sphinx_design" to the list of extensions in conf.py then octicon works.

yep. just realized that as well.

I wonder if we pad the text with whitespace, then we might get the proper structure.

@jbms
Copy link
Owner

jbms commented Apr 2, 2022

We shouldn't have to pad with whitespace --- I think the first docutils node is getting converted to a string, which is why we are seeing the docutils markup

@2bndy5
Copy link
Collaborator

2bndy5 commented Apr 2, 2022

        textnodes, _ = self.state.inline_text(" " + self.arguments[0], self.lineno)

this works:
image

@jbms
Copy link
Owner

jbms commented Apr 2, 2022

The bug is actually in these two lines:

self.arguments[0], *textnodes, classes=["tabbed-label"]

For docutils nodes that inherit from TextElement, the first argument to __init__ is the rawsource, and the second argument is the element text --- after that come the children.

Therefore, the first child is getting interpreted as text in both places.

Adding an extra "" argument fixes that.

@2bndy5
Copy link
Collaborator

2bndy5 commented Apr 2, 2022

oh, I didn't notice which line you were talking about with the "",
with

#L95
        tab_label = nodes.rubric(
            "", "", *textnodes, classes=["tabbed-label"]
        )

#L154-157
        label_node = content_tab_label(
            "",
            "",
            *tab_label.children,

renders as expected:
image

2bndy5 added a commit that referenced this issue Apr 2, 2022
@2bndy5
Copy link
Collaborator

2bndy5 commented Apr 7, 2022

I took a peek at nav_adapt.py (the code that assembles the ToCs), and it looks like most of the code was designed around using plain text for section titles. I don't think it will be an easy 1-2 line fix.

[EDIT]
I was able to hack it with raw HTML as the title's text (_TocVisitor._render() instead of _TocVisitor._render_title()), but I had to forego the <wbr> insertion tactic we implemented for wrapping long API names into multiple lines (to avoid the automatic ellipsis).

BTW, inline literals don't look that weird. I'm rather used to seeing a mix of monospace fonts with regular fonts in the side nav of other themes, namely the rtd-theme.
image "Additional Samples" is 2 separate inline literals

@jbms
Copy link
Owner

jbms commented Apr 7, 2022

Yeah, render_partial is the way that Sphinx uses to render snippets like this. However, when profiling documentation generation I discovered a problem with render_partial: it is actually quite expensive because it sets up a fresh docutils Publisher for every call, and renders a full document, only to extract a small portion.

In normal use Sphinx ends up calling render_partial ~5 times for every page just to render titles in a few places (e.g. for the "Prev" and "Next" page links, the ancestor pages, etc.). For my API documentation where every symbol is a separate page, I have several hundred pages, and every call to render_partial adds about 1.5 milliseconds. That doesn't seem too slow, but when you multiply that by 5 calls * 500 pages you are spending 3.5 seconds just generating a few tiny titles per page. The total build was around 60 seconds (without using parallel build), so 3.5 seconds is a pretty significant amount of time to spend on such a tiny thing. If we call render_partial for every single TOC entry per page it would be much, much slower.

I worked around that issue by monkey patching render_partial to check if the input node is just a pure text node, in which case it just renders it directly without spinning up docutils. For the TOC, we could potentially do similar checking, and handle text-only nodes specially --- we could do the <wbr> injection only in that case. Then in the general non-text-only case, we could ideally combine all of the titles together into a single document, render that document, then extract back the titles, so that we only require a single call to render_partial.

In fact I didn't even notice the render_partial issue until after fixing a larger performance issue: currently in nav_adapt.py we generate the TOC separately for each page, by first asking Sphinx to generate a complete global TOC (as docutils nodes) for all pages, then pruning it to just include what we want for the single page, and converting it for use by the HTML templates. This ends up having quadratic cost in the number of pages, since for every page we first generate a complete TOC covering all pages --- therefore with a large number of pages a large amount of time is spent just generating the TOCs.

The solution I implemented was to ask Sphinx for the TOC just once, convert it to an internal format, and then for each page quickly extract just the portion we need. One tricky thing is that normally Sphinx converts the page references to relative URLs as part of the TOC generation, but that is problematic since we will be re-using the same TOC docutils representation for all pages. That requires manually fixing up the relative URLs in nav_adapt.py.

We would have to see how we can make rendering the title as HTML fit with this optimization --- in most cases the HTML would be identical, but it is possible that the title itself could contain a link to another page, which would get rendered as a relative URL. Though maybe we can ignore that issue since it is kind of weird to have a separate link within a TOC title.

I will try to bring over these fixes as PRs today so that they aren't just piling up on my end.

@2bndy5
Copy link
Collaborator

2bndy5 commented Apr 7, 2022

If a ref link is present in a toc title, then I would be surprised if clicking the link within the displayed toc will actually have an effect.

@jbms
Copy link
Owner

jbms commented Apr 7, 2022

I don't think there is anything that prevents those referenced being turned into <a> elements, but yeah agreed that we don't really want them to do that. Probably we could solve that with some preprocessing of the docutils nodes before rendering them.

There is a better example than a reference node, though --- if you have a node that references an image file (e.g. an icon), then there is the same issue with a relative URL. This issue only applies if the image is referenced by URL in generated HTML, though --- if it is embedded then it wouldn't be an issue.

@2bndy5 2bndy5 mentioned this issue Apr 13, 2022
jbms pushed a commit that referenced this issue Apr 20, 2022
* patch literal blocks captions; add result directive

* update docs for results directive

* add task-list directive and keys role

* add experimental button idea

* add in new demo CSS

* pleasing pylint

* ran black on button.py

* partial solution to content tabs in #56

* improve api doc signatures and code block captions

* pleasing stylelint

* pleasing pylint

* polish & remove experimental button.py

- CSS additions consolidated and revised
- result directive (and corresponding CSS)
- various improvements to the docs

* pleasing stylelint

* still pleasing stylelint

* hopefully the last stylelint fix

* [stylelint] WTH is with property ordering in scss

* adjust custon keys' CSS for chrome

* remove button testing CSS

* use descrptive directive name

* use snake casing for docutils node

* move a margin reset to typeset.scss

* review changes to kbd_keys.py

* use plain text for keys role in latex

* simplfy example CSS for custom task-list

* move rst-result CSS to _highlight.scss

* wrap each param in span & style said span

* [no ci] move comment to proper location

* condensed selectors = less deviation from upstream

* set autocrlf to input

* try overriding git global config for this repo

* override config for all files (not by core config)
@2bndy5 2bndy5 changed the title Resolve roles in navigation & content tabs Resolve roles in navigation ~& content tabs~ Oct 29, 2022
@2bndy5 2bndy5 changed the title Resolve roles in navigation ~& content tabs~ Resolve roles in navigation Oct 29, 2022
@2bndy5
Copy link
Collaborator

2bndy5 commented Oct 30, 2022

I took a swing at letting docutils.nodes.raw get partially rendered and then only injecting <wbr> tags in the title's text data... So, we should be able to use sphinx-design's inline icons (which instantiate as a docutils.nodes.raw node) in the page titles.

@jbms Can you think of an objection to using render_partial() for raw nodes in the nav menu?

This feels hacky because it is focused solely on letting SVG glyphs be part of the title in the nav menu.

example

..  in admonitions.rst

:material-regular:`warning` Admonitions
=======================================

renders like so:
image

I had to use python's html.parser.HTMLParser to extract only the text data from a title's text and inject the <wbr> appropriately. Clearly, <wbr> isn't used in such a short title, but it didn't break the word wrapping in all the other section titles/headers.

@jbms
Copy link
Owner

jbms commented Oct 30, 2022

One question is whether it is always desirable to allow rST styling to apply to the navigation as well --- for example, if some portions use a literal or code role, do we want those styles (e..g font and possibly color) to also apply to the navigation, and if so, how do we ensure that the CSS rules properly interoperate with the CSS rules for navigation items.

The other question is for the specific case of icons, is it actually desired to have the icon present in the header (e.g. <hN> element), or is it just desired to have it in the navigation, as with the mkdocs-material feature: https://squidfunk.github.io/mkdocs-material/reference/#setting-the-page-icon
?

Regarding the implementation of using render_partial:

We should just test to see how expensive this is --- when generating API documentation, the total number of navigation items can be quite large, and we don't want to spend an excessive time rendering the navigation. Sphinx 5 significantly optimized render_partial by allowing the docutils publisher to be reused, but render_partial still does a lot of work, because it uses the docutils publisher interface that was really designed to process an entire document (but that is the only interface available in docutils). If it is too expensive, we could special case nodes that contain only text.

I think rather than parsing the HTML to add back in wbr, it would be better to modify the HTMLTranslator to just add wbr when translating text nodes in the first place.

Another alternative would be to use a custom HTML translator that only supports a limited set of docutils nodes, and just converts everything else to text.

@2bndy5
Copy link
Collaborator

2bndy5 commented Oct 30, 2022

I'll push my changes to a test branch for experimentation/review.

My primary goal is to allow icons in the nav menu (not concerned about supporting all other roles). Yes, putting the inline icon role in the header/section title also makes it show on the page, and I'm ok with that since it is expected behavior. But if the goal is to only use icons in the nav menu (like the upstream insider feature), then we'd first have to support the metadata tags feature that was merged in from v8.5.6.

I'm on the fence about how to approach this. As an experienced Sphinx user, my first instinct is to use the header/section title. But I imagine those migrating from mkdocs might first try the metadata tags approach. From a dev standpoint, it would be more cohesive to do it like upstream does (even though that's still a premium feature).

FWIW, the inline icon in the main section of the page seems a bit strange to me because I'm just not used to seeing it. (switched to octicons/alert since last preview image)
image

@2bndy5
Copy link
Collaborator

2bndy5 commented Oct 30, 2022

I just realized the metadata tag approach won't work with toctree captions. Doesn't feel like a deal breaker, but it would be easy to support if using the inline icon role approach.

@jbms
Copy link
Owner

jbms commented Oct 30, 2022

Another option would be to introduce a custom role for specifying the nav icons that generates a docutils node type that we will ignore when rendering the section headings but is handled when generating the nav labels.

Could also be a custom directive to specify an icon for the current section.

@2bndy5
Copy link
Collaborator

2bndy5 commented Oct 30, 2022

I kinda like that idea because then users could put it in other places like paragraphs. Compared to sphinx-design, maybe we could support a CSS class (optional and separated by a semi-colon), so they could further adjust the color or animated it.

I'm picturing something like:

:md-icon:`material/document`
:md-icon:`material/document;custom-class`

Where the role arguments would be a path to the icon (required) [and custom CSS class].

@2bndy5
Copy link
Collaborator

2bndy5 commented Oct 30, 2022

We may have to regex the icon in toctree captions because the directive's option accepts only text.

@2bndy5
Copy link
Collaborator

2bndy5 commented Oct 31, 2022

I think rather than parsing the HTML to add back in wbr, it would be better to modify the HTMLTranslator to just add wbr when translating text nodes in the first place.

Seems to me that this is the only way to support icons in the toctree caption because Sphinx' visitor for Text nodes (inherited by title nodes) does its own filtering (based on astext() output). This idea also feels more like a refactor than a patch to nav_adapt.py. I'm not against it, but it will take me some time to write a working draft (lots going on in nav_adapt already).

@mhostetter
Copy link
Contributor Author

mhostetter commented Jan 23, 2023

Hey guys. Just wanted to check in on this issue. Any chance we can make this possible? I think it would add a lot of flair to the websites! ✨

As always, very appreciative of your work here!

@2bndy5
Copy link
Collaborator

2bndy5 commented Jan 23, 2023

I've become focused on only allowing an icon (like our new si-icon role) into the toc. I stumped a while back on how to actually implement this though (because of the toctree directive's :caption: in which sphinx' src treats as text only).

@jbms
Copy link
Owner

jbms commented Jan 24, 2023

I've become focused on only allowing an icon (like our new si-icon role) into the toc. I stumped a while back on how to actually implement this though (because of the toctree directive's :caption: in which sphinx' src treats as text only).

That is a good point --- I did not think about that.

For page titles, Sphinx by default preserves markup and this theme specifically strips it and renders them as text only, unlike other themes. However, toctree captions are indeed text only in Sphinx. I think there are a few possible solutions:

  • Support this functionality only for page titles, not toctree captions.
  • Monkey patch Sphinx's handling of toctree caption to treat it as rST rather than plain text. This could be controlled by a global option. Possibly upstream Sphinx would be interested in this change as well.
  • Modify Sphinx to add a new option to toctree, e.g. rich-caption, that can be used in lieu of caption and which will be interpreted as rST rather than plain text.

Also of note, mkdocs-material-insiders has some new features related to this, including the "typeset plugin" (https://squidfunk.github.io/mkdocs-material/reference/#built-in-typeset-plugin) which is exactly the feature requested here. While this plugin would not be relevant to Sphinx and we can't make use of the mkdocs-material-insiders code, I think it could make sense to take it into account in coming up with a more comprehensive plan for what options we want to provide in sphinx immaterial.

@2bndy5
Copy link
Collaborator

2bndy5 commented Jan 24, 2023

I was aiming for option 2 a while ago, but the millennial in me would settle for option 1 (for a more instant gratification).

@jbms
Copy link
Owner

jbms commented Jan 24, 2023

I was aiming for option 2 a while ago, but the millennial in me would settle for option 1 (for a more instant gratification).

Seems fine since it is a subset of the work for option 2. Option 2 should also be pretty easy to implement.

@2bndy5
Copy link
Collaborator

2bndy5 commented May 7, 2023

  • Support this functionality only for page titles, not toctree captions.

I was able to do this by supporting roles in the section headings while maintaining the needed <wbr> injection for wrapping long words (like API names). My new attempt is on the nav-roles branch.

  • Monkey patch Sphinx's handling of toctree caption to treat it as rST rather than plain text

I tried doing this without monkey patching the toctree directive. Instead, I tried wrapping the original directive in a derivative used to overload the toctree... This seemed to fail, but I'm not sure the failure was exclusive to the wrapping approach. It'd probably better to address this in a separate issue later.

mkdocs-material-insiders has some new features related to this

I also tried implementing the icon metadata approach used in upstream to only prefix the ToC entries with an icon (using our si-icon role's mechanisms), but the html-page-context event that I used was triggered too late for the icons' SVG data to get added to the generated CSS (in env-check-consistency event). Or maybe, the env from separate threads that parse the doc srcs didn't contain the added ToC icons... It doesn't seem too difficult to solve this, I just need a better understanding of all the involved event triggers concerning inline icons' SVGs.

In a side note, I think the support for roles in section headers is a more stable approach due to the way the ToCs are extracted (seems deep-copied) from a cache when nav_adapt.py handles the html-page-context event. Not to mention, roles in section headers is how this is supported for every other Sphinx theme I've ever used.

@2bndy5 2bndy5 added the feat: ToC Related to Table of Contents label May 7, 2023
@2bndy5
Copy link
Collaborator

2bndy5 commented Sep 4, 2023

Upstream seems to have added page icons in v9.2.0 (no longer an insiders feature).

@2bndy5 2bndy5 added the feat: upstream a feature that exists upstream but is not well supported in sphinx-immaterial label Apr 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: ToC Related to Table of Contents feat: upstream a feature that exists upstream but is not well supported in sphinx-immaterial
Projects
None yet
Development

No branches or pull requests

3 participants