fralau / mkdocs-macros-plugin

Create richer and more beautiful pages in MkDocs, by using variables and calls to macros in the markdown code.
https://mkdocs-macros-plugin.readthedocs.io
Other
318 stars 50 forks source link

Icons / Emoji fail to render in header with Macros enabled #215

Closed AnvithLobo closed 2 months ago

AnvithLobo commented 5 months ago

Icons / Emoji fail to render in sidebar and header section when macros is enabled with mkdocs material

index.md

# Test :material-robot-happy-outline:

## Test2  :smiley:

mkdocs.yml

plugins: 
  - search
  - blog 
  - macros
  - pymdownx.emoji:
    emoji_index: !!python/name:material.extensions.emoji.twemoji
    emoji_generator: !!python/name:material.extensions.emoji.to_svg

Plugin Versions

mkdocs==1.5.3
mkdocs-glightbox==0.3.7
mkdocs-macros-plugin==1.0.5
mkdocs-material==9.5.11
mkdocs-material-extensions==1.3.1
mkdocs-rss-plugin==1.12.1

When macros is enabled

image

Disabling macros plugin fixes the issue.

image

github-actions[bot] commented 5 months ago

Welcome to this project and thank you!' first issue

fralau commented 5 months ago

I have half an idea of why that could be. Would you have an Minimal Reproducible Example that I could test?

AnvithLobo commented 5 months ago

mkdocs.yml

site_name: My Docs

plugins: 
  - search
  - macros

markdown_extensions:
- pymdownx.emoji:
    emoji_index: !!python/name:material.extensions.emoji.twemoji
    emoji_generator: !!python/name:material.extensions.emoji.to_svg

theme:
  icon:
    annotation: material/arrow-right-circle
  name: material
  language: en

Index.md

# Test :material-robot-happy-outline:

## Test2  :smiley:
fralau commented 5 months ago

Thanks. 🙂

AnvithLobo commented 5 months ago

Hi thanks for taking a look. I've yet to dig deeper but I found the culprit causing it. This is the first time i've tried to debug a mkdocs plugin. So it'll probably take me some time to figure out exactly what's causing this.

https://github.com/fralau/mkdocs-macros-plugin/blob/bf6c8e4cd0cb0dee9999103d44f18550aaeee82b/mkdocs_macros/plugin.py#L789-L790

Disabling title rendering in the above line makes it so mkdocs renders the icon correctly but obviously any macros in the title won't be rendered.

fralau commented 5 months ago

That's very good: it narrows it down a lot. If I rewrite the title, that will of course overwrite that kind of processing.

Should not be too difficult to fix.

fralau commented 5 months ago

I had a look, and I confirm your observations: if we re-assign page.title the rendering of the icon in the title stops working.

It seems mystical, since the new string is identical.

Just writing this causes the bug (so MkDocs-Macros is not directly at cause):

            page.title = page.title + ""

At this point, I would invoke @squidfunk. What do you think could be the cause?

In the mean time, I put a test for the presence of a { as a proxy for the presence of macros. So that if there is none, page.title will not be reassigned and the icon in the title will be rendered. It's a little inelegant, but at least your page will be rendered correctly.

fralau commented 5 months ago

I pushed a workaround on Github. Could I ask you to test it?

That should leave us the time to investigate this strange behavior.

AnvithLobo commented 5 months ago

It seems mystical, since the new string is identical.

Yeah that's what I observed too. maybe memory address change or something makes the emoji renders to not be called / fail?

I pushed a workaround on Github. Could I ask you to test it?

That should leave us the time to investigate this strange behavior.

@fralau I'm sorry, am I missing something it seems the recent commit only contains a change to the doc file.

AnvithLobo commented 5 months ago

Yup as long as the Title doesn't contain any macros or { character it'll work.

It's a viable solution until we figure out the culprit causing it I guess.

What do you think about leaving this issue open?

Thanks for the fix :)

fralau commented 5 months ago

Absolutely, let's leave it open.

squidfunk commented 5 months ago

Just writing this causes the bug (so MkDocs-Macros is not directly at cause):

            page.title = page.title + ""

At this point, I would invoke @squidfunk. What do you think could be the cause?

I'm not sure, but be aware that the Page.title property is a weak property. Thus, I believe the described behavior is induced by MkDocs, not by Material for MkDocs, so it may be a good idea to ask the maintainers of MkDocs.

AnvithLobo commented 5 months ago

okay looking at weak_property led me to check how title from other plugins were being assigned.

But from what I could gather none of the plugins I checked explicitly did any changes to page.title

https://github.com/fralau/mkdocs-macros-plugin/blob/814205a64d7ee0d88bafefb8fa2df33ba9d7e886/mkdocs_macros/plugin.py#L791-L796

@fralau was there any reason for the above code? I would like to understand why this was added if there was any edgecase that I might be overlooking.

But I can remove that completely and everything works fine. Page title renders correctly everywhere. (hello is rendered using macros {{ test }} image

I'm still yet to trackdown any other places that title could be assigned from but this is the only instance I could find where the Title could be extracted from.

https://github.com/mkdocs/mkdocs/blob/e755aaed7ea47348a60495ab364d5483ab90a4a6/mkdocs/structure/pages.py#L281

AnvithLobo commented 5 months ago

Did more digging.

Looks like when title with meta vars is set removing page.title = breaks macros rendering.

I'm guessing there should be a way to replace the meta var title and not directly mess with page.title.

I'll have a look at all the preprocessors and report back If i find a solution.

ps: emoji rendering is not possible in metavar title anyway as : seems to be a bad character in mata_var. Leading me to believe they're probably doing something like variable.split(":" ) to assign key and value

AnvithLobo commented 5 months ago

Could you take a look at my fork. I did fix the issue.

image

mkdocs.yml

extra: 
  test: "macro 1"
  hi: ":material-robot-happy-outline: emoji_macro"

I've also taken a look at issue #144 but menu and title seem to be rendered without any issues as seen in the screenshot above. Maybe a upstream mkdocs fix?

currently only the below test case doesn't pass. But this is not a problem raised from macros as disabling the plugin still doesn't render emoji. It's most likely that the emoji plugin forgot to check meta variables.

---
title: Oh no ":material-robot-happy-outline:" {{ test}}
---

The way I've handled the rendering of title meta variable might not be desirable for you. Please review the changes and let me know if you'd like any adjustments. If the changes are satisfactory, please confirm so I can proceed to create a pull request.

Thanks :)

fralau commented 5 months ago

Thanks, because your investigation is helping a lot.

okay looking at weak_property led me to check how title from other plugins were being assigned.

Curious: what is weak_property?

But from what I could gather none of the plugins I checked explicitly did any changes to page.title

Interesting 🤔 .

https://github.com/fralau/mkdocs-macros-plugin/blob/814205a64d7ee0d88bafefb8fa2df33ba9d7e886/mkdocs_macros/plugin.py#L791-L796

@fralau was there any reason for the above code? I would like to understand why this was added if there was any edgecase that I might be overlooking.

Are you refering to the call to macro rendering? Yes: MkDocs-Macros must be able render correctly macros in the headers of pages (when used for navigation), in the same way that references to emojis or icons must be correctly rendered.

Correctly rendering macros for navigation (page names and contents table for each page) is an essential feature, just as rendering them in pages.

(Actually, it is even possible write a title for a page that contains a call to a macro, in the navsection of the config file and it will be rendered).

As you noted with #144, it's not the first mysterious issue that I am having with navigation in MkDocs. There must be something to know about it, that I haven't discovered yet.

Rather than jumping to fixes (which might or might not work for the 2'500+ Github projects that are using this plugin, not to mention the others I don't know about), I would like to first grasp better what the problem is, with its potential ramifications.

AnvithLobo commented 5 months ago

What's failing right now

nav:
    - Home {{ test }} : index.md
    - first {{ test }}:
      - Second {{ test }}:
          - test {{ test }}:
            - Environment for {{test}}: Docs/index.md
            - Also for {{ macro2}}: Docs/sub/index.md
    - Not interpreted: Docs/file2.md

extra: 
  test: "macro 1"
  hi: ":material-robot-happy-outline: emoji_macro"
  macro2: "macro 2"
  macro3: "macro 3"

Right now with code change macros in yaml keys for nav are not rendered. image

vs before code change except for where the keys with nested dict (ex: second {{test}}) macros were rendered correctly.

image

I'll dig through the on_nav function later to see if I can get around to fixing everything.

What's working right now

Curious: what is weak_property?

It's a custom class defined in mkdocs utils

https://github.com/mkdocs/mkdocs/blob/e755aaed7ea47348a60495ab364d5483ab90a4a6/mkdocs/utils/__init__.py#L388-L399

class weak_property:
    """Same as a read-only property, but allows overwriting the field for good."""

    def __init__(self, func):
        self.func = func
        self.__doc__ = func.__doc__

    def __get__(self, instance, owner=None):
        if instance is None:
            return self
        return self.func(instance)

Are you referring to the call to macro rendering? Yes: MkDocs-Macros must be able render correctly macros in the headers of pages (when used for navigation), in the same way that references to emojis or icons must be correctly rendered.

Correctly rendering macros for navigation (page names and contents table for each page) is an essential feature, just as rendering them in pages.

I believe this works perfectly right now even without page.title reassignment as I tested examples from these issue

And all of them render correctly without any issues.

image

Why this works

fralau commented 5 months ago

Removing that call, because it is redundant? Mmm... we might be doing some progress 🤔

Providing that MkDocs can handle navigation strings nicely on its own, while we handle explicitly the entry points of MkDocs-Macros (variables whose macros must be interpreted), we might have something that works.

Of course, this idea should be well formalized and tested in practice, and then it would be necessary to check that all test cases pass as before.

It might be a little tedious to do, since making sure that mkdocs serve starts correctly and that all pages display without breaking a server or causing massive warnings, might not be sufficient. It will also be necessary that a human checks carefully that everything displays as expected (the devil is in the detail).

AnvithLobo commented 5 months ago

Currently we are only doing a update of page title on on_page_markdown which only gets called on mkdocs.structure.pages.Page() and not on mkdocs.structure.nav.Section() hence the current behavior of macros not being rendered in section

I see two paths to make sure all macros in navs are rendered (not just the pages but sections too)

1) We make sure to recurse through all the navigation and update the title using on_nav method before on_page_markdown is called 2) use page.parent doc to recurse upwards to find any sections and update the title then. The downside of this being we will recurse multiple times to parent sections if a Section contains multiple Pages . We will also not be able to render macros on any empty sections.

Method 1 is the most ideal. But respecting render_macros flag becomes hard. Either we duplicate the logic of current on_page_markdown's render check or abstract it away to a different method so the code is not duplicated. There is still a looming question what needs to be done when a section has macro but the children pages say do not render macros on pages? Or do we always render macros on sections and handle each page according to the will of current page using the existing logic used in on_page_render

AnvithLobo commented 5 months ago

https://github.com/AnvithLobo/mkdocs-macros-plugin/commit/c6cc777aa507b19f61a502d814f329acc9cdb5e9

This commit should handle rendering of titles in navigation. One note though I wasn't able to cherry pick which pages / Sections have their Navigation title rendered as on_page_markdown doesn't receive the nav. Also best to handle any nav related rendering in Nav itself.

Error handling for

nav.title = self.env.from_string(nav.title).render(**self.variables)

Hasn't been implemented yet. You have a custom format_error which isn't compatible with on_nav So i'll probably have to implement a new format_error to handle errors from on_nav.

At least from what I can tell all test cases seem to be passing expect one no_module. But this fails in the 1.1.1 unmodified version too.

fralau commented 5 months ago

Awesome exploration work.

One note though I wasn't able to cherry pick which pages / Sections have their Navigation title rendered as on_page_markdown doesn't receive the nav. Also best to handle any nav related rendering in Nav itself.

Could explain a little more in detail what the problem is with sections?

(Error handling) Hasn't been implemented yet. You have a custom format_error which isn't compatible with on_nav So i'll probably have to implement a new format_error to handle errors from on_nav.

Why is it not compatible with on_nav? (or rather: what should be there to make it compatible?)

More generally, there is a sequence issue with doing that action on_nav (see the events diagram): it takes place before the page is actually populated. It means that any handling by mkdocs-macros would be missing the variables defined within the page.

Trying to manipulate the nav at that stage might thus not be the right approach. It must be possible to access that object in on_markdown (it's not passed as an argument, but that's mostly syntactic sugar, since self contains everything).

However, assuming that the first solution that you proposed works; which is summarized (if my understanding is correct) by:

Providing that MkDocs can handle navigation strings nicely on its own, while we handle explicitly the entry points of MkDocs-Macros (variables whose macros must be interpreted), we might have something that works.

And assuming that it leaves out only a scope of unsolved issues (which we should explain clearly), then I believe it would definitely be a progress.

fralau commented 5 months ago

I believe we need to go back to a number of key points about page title, when it appears as a section (navigation).

In standard MkDocs, that can be defined in (by priority order):

  1. The header of the page (title=...)
  2. The config file
  3. The first header 1 in the page.

MkDocs decides which one it takes, according to its own logic. I believe that MkDocs-Macros should avoid interfering with the building of the navigation.

When I remove the code:

            debug("Page title:",page.title)
            if "{" in page.title:
                page.title = self.render(markdown=page.title,
                                        force_rendering=force_rendering)
                debug("Page title after macro rendering:",page.title)

The module test case stops working. Here is the nav of that test case:

nav:
    - Home: index.md
    - Environment for {{ unit_price}}: environment.md
    - Second:
        - other.md
    - Not interpreted: literal.md

Right now, I do not see any better solution than enforcing the rendering of macros the title. If the side effect is the non-rendering of emojis when used at the same time as macros, I am willing to live with that problem, rather than jumping into a rabbit hole. 🙂

AnvithLobo commented 5 months ago

(Error handling) Hasn't been implemented yet. You have a custom format_error which isn't compatible with on_nav So i'll probably have to implement a new format_error to handle errors from on_nav.

Why is it not compatible with on_nav? (or rather: what should be there to make it compatible?)

Ah what I meant here is that the jinja template renderer md_template.render(**page_variables) is inside a try catch block in the self.render function to "pretty print" the error message using mkdocs_macros.errors.format_error . But since we also have to call the jinja render in on_nav I currently have not put them inside a try catch block as there is no equivalent error formatter for on_nav so if Jinja2 render throws a error in on_nav users will see a raw traceback instead.

In standard MkDocs, that can be defined in (by priority order):

  1. The header of the page (title=...)
  2. The config file
  3. The first header 1 in the page.

Yes this is correct. In situations where all three are defined Nav title is displayed in the menu while the title from meta var is displayed as the page title <head><title>.

MkDocs decides which one it takes, according to its own logic. I believe that MkDocs-Macros should avoid interfering with the building of the navigation.

Yup. The current Implementation i did wholey relies on Mkdocs to decide it. Macro plugin should just render the macros and pass it along as it is.

Right now, I do not see any better solution than enforcing the rendering of macros the title. If the side effect is the non-rendering of emojis when used at the same time as macros, I am willing to live with that problem, rather than jumping into a rabbit hole.

Yeah. Trying to obey render_macros defined in meta variables for navigation will just cause more problems than it solves.

But right now with the updated solution I'm not honoring render_by_default for nav. This is a easy fix and as it's just one more check I need to do before calling the recurse_nav method

fralau commented 5 months ago

Submit a PR and I will have a look at it.

But to be honest, the chances are that I will not pass it: adding potientally complex code (that even I might have trouble to fully understand and therefore maintain) might not worth it, considering that it is such a corner case.

In any case, that PR will remain there as a research item. Who knows that it might turn useful in the future?

fralau commented 5 months ago

@squidfunk wrote:

I'm not sure, but be aware that the Page.title property is a weak property. Thus, I believe the described behavior is induced by MkDocs, not by Material for MkDocs, so it may be a good idea to ask the maintainers of MkDocs.

Sorry, I missed your remark! Makes sense.

fralau commented 5 months ago

I documented this in the online documentation.