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

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

Open
AnvithLobo opened this issue Feb 27, 2024 · 26 comments
Open

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

AnvithLobo opened this issue Feb 27, 2024 · 26 comments
Labels
bug Something isn't working documentation Documentation is required fixed A fix has been submitted keep in mind Interesting idea, keep in mind for the future

Comments

@AnvithLobo
Copy link

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

Copy link

Welcome to this project and thank you!' first issue

@fralau
Copy link
Owner

fralau commented Feb 27, 2024

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

@AnvithLobo
Copy link
Author

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
Copy link
Owner

fralau commented Feb 27, 2024

Thanks. 🙂

@AnvithLobo
Copy link
Author

AnvithLobo commented Feb 29, 2024

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.

page.title = self.render(markdown=page.title,
force_rendering=force_rendering)

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
Copy link
Owner

fralau commented Feb 29, 2024

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
Copy link
Owner

fralau commented Mar 2, 2024

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 pushed a commit that referenced this issue Mar 2, 2024
  - to prevent a problem in case pymdownx.emoji is being used
@fralau fralau added bug Something isn't working solved The issue has been satisfactorily solved please test An updated was pushed. This needs to be tested and removed solved The issue has been satisfactorily solved labels Mar 2, 2024
@fralau
Copy link
Owner

fralau commented Mar 2, 2024

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
Copy link
Author

AnvithLobo commented Mar 2, 2024

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.

fralau pushed a commit that referenced this issue Mar 2, 2024
  Files were missing
@AnvithLobo
Copy link
Author

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
Copy link
Owner

fralau commented Mar 2, 2024

Absolutely, let's leave it open.

@fralau fralau added help wanted Extra attention is needed and removed please test An updated was pushed. This needs to be tested labels Mar 2, 2024
@squidfunk
Copy link

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
Copy link
Author

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

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)

@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
Copy link
Author

AnvithLobo commented Mar 3, 2024

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
Copy link
Author

AnvithLobo commented Mar 3, 2024

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 :)

AnvithLobo added a commit to AnvithLobo/mkdocs-macros-plugin that referenced this issue Mar 4, 2024
- allow macros in meta variables
- allow automatic parsing of title by mkdocs as explicitly  assigning page.title will override that.
@fralau
Copy link
Owner

fralau commented Mar 4, 2024

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 🤔 .

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)

@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
Copy link
Author

AnvithLobo commented Mar 4, 2024

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

  • Mkdocs automatically parses title from H1 so rendering the title correctly in H1 solves the issue of macros not being rendering in title
  • macro in Title meta var by default is not rendered by the on_page_markdown function. So by directly updating the self.variables["page"].meta["title"] by calling the 2nd jinja2 render function on it solves the issue of parsing any macros. And then let mkdocs handle the processing from there.

@fralau
Copy link
Owner

fralau commented Mar 4, 2024

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
Copy link
Author

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
Copy link
Author

AnvithLobo@c6cc777

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
Copy link
Owner

fralau commented Mar 5, 2024

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
Copy link
Owner

fralau commented Mar 5, 2024

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. 🙂

@fralau fralau added keep in mind Interesting idea, keep in mind for the future fixed A fix has been submitted and removed help wanted Extra attention is needed labels Mar 5, 2024
@AnvithLobo
Copy link
Author

(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
Copy link
Owner

fralau commented Mar 6, 2024

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
Copy link
Owner

fralau commented Mar 6, 2024

@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 pushed a commit that referenced this issue Mar 6, 2024
@fralau
Copy link
Owner

fralau commented Mar 6, 2024

I documented this in the online documentation.

@fralau fralau linked a pull request Mar 6, 2024 that will close this issue
@fralau fralau added the documentation Documentation is required label Mar 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working documentation Documentation is required fixed A fix has been submitted keep in mind Interesting idea, keep in mind for the future
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants