Repository: executablebooks/MyST-Parser
Pull request #711 (2023-02-21 14:55:33)

👌 Handling of nested headers
For the longest time, nested headers in myst-parser have been a pain, particularly in things like admonitions

In Markdown (and HTML) headings are allowed "anywhere", for example:

```markdown
> # Heading 1
## Heading 2

Paragraph
```

Is rendered as:

```html
<blockquote>
    <h1>Heading 1</h1>
</blockquote>
<h2>Heading 2</h2>
<p>Paragraph</p>
```

However, because docutils/sphinx treats headers as nested sections, this becomes problematic

```xml
<blockquote>
   <section>
		<title>
            Heading 1
<section>
   <title>
      Heading 2
   <paragraph>
      Paragraph
```

Which sphinx cannot resolve the ToC tree from etc

This PR fixes this, by identifying if a heading is inside another component and instead outputting it as a "non-structural" rubric node

```xml
<blockquote>
   <rubric level=1>
		Heading 1
<section>
    <title>
      Heading 2
   <paragraph>
      Paragraph
```

Natively, docutils/sphinx does not deal with the "level" key in the rubric,
so here we also override the rubric HTML renderer to correctly output a `<h>` element, if "level" is present, to retrieve the desired:

```html
<blockquote>
    <h1>Heading 1</h1>
</blockquote>
<h2>Heading 2</h2>
<p>Paragraph</p>
```

~~Note, when such an identification is made, a `myst.nested_header` warning is emitted:~~

There is no longer any warning of nested headers, since this is the intended behaviour

~~Additionally, using the new `attrs_block` syntax extension, a user can specifically specify that a heading should not be included in the ToC~~

This was removed


codecov[bot]: # [Codecov](https://codecov.io/gh/executablebooks/MyST-Parser/pull/711?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=executablebooks) Report
Base: **89.46**% // Head: **89.30**% // Decreases project coverage by **`-0.16%`** :warning:
> Coverage data is based on head [(`0ad31f9`)](https://codecov.io/gh/executablebooks/MyST-Parser/pull/711?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=executablebooks) compared to base [(`1096e52`)](https://codecov.io/gh/executablebooks/MyST-Parser/commit/1096e520a8624425a84a2a226ea174212c83c836?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=executablebooks).
> Patch coverage: 62.50% of modified lines in pull request are covered.

<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##           master     #711      +/-   ##
==========================================
- Coverage   89.46%   89.30%   -0.16%     
==========================================
  Files          23       23              
  Lines        2828     2842      +14     
==========================================
+ Hits         2530     2538       +8     
- Misses        298      304       +6     
```

| Flag | Coverage Δ | |
|---|---|---|
| pytests | `89.30% <62.50%> (-0.16%)` | :arrow_down: |

Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=executablebooks#carryforward-flags-in-the-pull-request-comment) to find out more.

| [Impacted Files](https://codecov.io/gh/executablebooks/MyST-Parser/pull/711?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=executablebooks) | Coverage Δ | |
|---|---|---|
| [myst\_parser/parsers/docutils\_.py](https://codecov.io/gh/executablebooks/MyST-Parser/pull/711?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=executablebooks#diff-bXlzdF9wYXJzZXIvcGFyc2Vycy9kb2N1dGlsc18ucHk=) | `80.00% <45.45%> (-2.56%)` | :arrow_down: |
| [myst\_parser/mdit\_to\_docutils/base.py](https://codecov.io/gh/executablebooks/MyST-Parser/pull/711?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=executablebooks#diff-bXlzdF9wYXJzZXIvbWRpdF90b19kb2N1dGlscy9iYXNlLnB5) | `92.77% <100.00%> (ø)` | |
| [myst\_parser/sphinx\_ext/main.py](https://codecov.io/gh/executablebooks/MyST-Parser/pull/711?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=executablebooks#diff-bXlzdF9wYXJzZXIvc3BoaW54X2V4dC9tYWluLnB5) | `89.47% <100.00%> (+0.90%)` | :arrow_up: |

Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=executablebooks). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=executablebooks)

</details>

[:umbrella: View full report at Codecov](https://codecov.io/gh/executablebooks/MyST-Parser/pull/711?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=executablebooks).   
:loudspeaker: Do you have feedback about the report comment? [Let us know in this issue](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=executablebooks).

chrisjsewell: cc @choldgraf @rowanc1 I'm pretty pleased with this 😄 

Note, For the `{toc=false}` block attribute syntax, I'm discussing "best practice" here: https://github.com/jgm/djot/issues/213 
chrisjsewell: Actually, scrap the additional attribute logic, I have a better solution, and a few changes to make here...
chrisjsewell: Ok updated and ready to go!

To clarify, the logic is now:

- A `section` can only be a child of the root `document`, or another `section`
- If a header token is encountered, with a child that is not one of these, then it is added as a `rubric`
- Otherwise a new `section` is created, and the heading is added a `title`, which is a child of the `section`

(This is essentially exactly the same logic as djot 😄 )

There is no longer any warning about nested headings, because it is the intended behaviour of MyST (and should be documented as such)

One exception to this rule 😒  is that a few special directives (such as `only`) can specify that sections should be allowed to be created as a child of the current node, e.g.

```python
def MyDirective(Directive):
	def run(self) -> list[nodes.Node]:
         node = nodes.Element()
         self.state.nested_parse(self.content, self.content_offset, node, match_titles=True)
         return node.children
```

This is handled by the changes in this PR

chrisjsewell: Does this make sense @rowanc1 😄 
choldgraf: From a UX perspective this makes total sense to me 👍
rowanc1: As a note: in JupyterLab they use `tocSkip` (see [here](https://github.com/jupyterlab/jupyterlab/blob/bf2889c1571ba62426bfd0ed23976a6ce89b839f/packages/toc/src/utils/markdown.ts#L246)) which might be good to standardize on.

The section nesting is something we are also doing in the JATS export, which requires more structure (similar to RST). This has been somewhat at odds with the block structure (e.g. do blocks/cells define structure or do headings?!), but hasn't been too much of a problem so far, and we can at least transform between blocks --> sections very easily.

Overall, excited to see the nested heading errors go away!
chrisjsewell: > This has been somewhat at odds with the block structure (e.g. do blocks/cells define structure or do headings?!)

Oh yeh I know that one is a pain 😅
chrisjsewell: Ok so it looks like we are in agreement here, that I can merge this?
chrisjsewell: @rowanc1 another thing I want to do after this is merged, as a result of https://github.com/jgm/djot/issues/213, is change a bit how `colon_fences` work, to be more like djot divs
Since you are using them for admonitions, I imagine you would like to see them in myst core syntax 😄 

So at present, in myst-parser, this is treated as an admonition directive 👍 

```
:::{note}
Some text
:::
```

```xml
<note>
   <paragraph>
      Some text
```

But, this is treated like a code block 👎 

```
:::name
Some text
:::
```

```xml
<code language=name>
    Some text
```

I think, in keeping with the purpose of these block (and djot), it should be converted to

```xml
<div class=name>
   <paragraph>
      Some text
```

does that make sense?
rowanc1: Yes please to improved colon-fences. I see another MEP in our future. I like having more specificity on the colon blocks as to when to use them and why. I [sketched a bit here](https://myst-tools.org/docs/mystjs/syntax-overview#directives) about use colons when the content is markdown-like.

I would also like to be more like pandoc on the nesting front (i.e. a start of a div only requires three colons + attribute, and the end has any number of colons, but no attribute).
chrisjsewell: > i.e. a start of a div only requires three colons + attribute, and the end has any number of colons, but no attribute

yep as per djot, i.e. pandoc v2 😝 

> A div begins with a line of three or more consecutive colons, optionally followed by white space and a class name (but nothing else). It ends with a line of consecutive colons at least as long as the opening fence, or with the end of the document or containing block.

> The contents of a div are interpreted as block-level content.

Patch:
diff --git a/myst_parser/mdit_to_docutils/base.py b/myst_parser/mdit_to_docutils/base.py
index e920da3..22bb799 100644
--- a/myst_parser/mdit_to_docutils/base.py
+++ b/myst_parser/mdit_to_docutils/base.py
@@ -118,7 +118,7 @@ class DocutilsRenderer(RendererProtocol):
             "current_node",
             "reporter",
             "language_module_rst",
-            "_level_to_elem",
+            "_level_to_section",
         ):
             raise AttributeError(
                 f"'{name}' attribute is not available until setup_render() is called"
@@ -143,7 +143,7 @@ class DocutilsRenderer(RendererProtocol):
             self.document.settings.language_code
         )
         # a mapping of heading levels to its currently associated node
-        self._level_to_elem: dict[int, nodes.document | nodes.section] = {
+        self._level_to_section: dict[int, nodes.document | nodes.section] = {
             0: self.document
         }
         # mapping of section slug to section node
@@ -321,14 +321,18 @@ class DocutilsRenderer(RendererProtocol):
                 )
 
     def nested_render_text(
-        self, text: str, lineno: int, inline: bool = False, allow_headings: bool = True
+        self,
+        text: str,
+        lineno: int,
+        inline: bool = False,
+        temp_root_node: None | nodes.Element = None,
     ) -> None:
         """Render unparsed text (appending to the current node).
 
         :param text: the text to render
         :param lineno: the starting line number of the text, within the full source
         :param inline: whether the text is inline or block
-        :param allow_headings: whether to allow headings in the text
+        :param temp_root_node: If set, allow sections to be created as children of this node
         """
         tokens = (
             self.md.parseInline(text, self.md_env)
@@ -345,12 +349,21 @@ class DocutilsRenderer(RendererProtocol):
             if token.map:
                 token.map = [token.map[0] + lineno, token.map[1] + lineno]
 
-        current_match_titles = self.md_env.get("match_titles", None)
-        try:
-            self.md_env["match_titles"] = allow_headings
+        if temp_root_node is None:
             self._render_tokens(tokens)
-        finally:
-            self.md_env["match_titles"] = current_match_titles
+        else:
+            # we need to temporarily set the root node,
+            # and we also want to restore the level_to_section mapping at the end
+            current_level_to_section = {
+                i: node for i, node in self._level_to_section.items()
+            }
+            current_root_node = self.md_env.get("temp_root_node", None)
+            try:
+                self.md_env["temp_root_node"] = temp_root_node
+                self._render_tokens(tokens)
+            finally:
+                self.md_env["temp_root_node"] = current_root_node
+                self._level_to_section = current_level_to_section
 
     @contextmanager
     def current_node_context(
@@ -444,10 +457,10 @@ class DocutilsRenderer(RendererProtocol):
         # find the closest parent section
         parent_level = max(
             section_level
-            for section_level in self._level_to_elem
+            for section_level in self._level_to_section
             if level > section_level
         )
-        parent = self._level_to_elem[parent_level]
+        parent = self._level_to_section[parent_level]
 
         # if we are jumping up to a non-consecutive level,
         # then warn about this, since this will not be propagated in the docutils AST
@@ -465,12 +478,12 @@ class DocutilsRenderer(RendererProtocol):
         # append the new section to the parent
         parent.append(section)
         # update the state for this section level
-        self._level_to_elem[level] = section
+        self._level_to_section[level] = section
 
         # Remove all descendant sections from the section level state
-        self._level_to_elem = {
+        self._level_to_section = {
             section_level: section
-            for section_level, section in self._level_to_elem.items()
+            for section_level, section in self._level_to_section.items()
             if section_level <= level
         }
 
@@ -769,30 +782,28 @@ class DocutilsRenderer(RendererProtocol):
     def render_heading(self, token: SyntaxTreeNode) -> None:
         """Render a heading, e.g. `# Heading`."""
 
-        if (
-            token.attrs.get("toc", None) == "false"
-            or self.md_env.get("match_titles", None) is False
-        ):
-            if token.attrs.get("toc", None) != "false":
-                # this can occur if a nested parse is performed by a directive
-                # (such as an admonition) which contains a header.
-                # this would break the document structure
-                self.create_warning(
-                    "Disallowed nested header found, converting to rubric",
-                    MystWarnings.MD_HEADING_NESTED,
-                    line=token_line(token, default=0),
-                    append_to=self.current_node,
-                )
+        level = int(token.tag[1])
 
-            rubric = nodes.rubric(token.content, "")
+        # sections are only allowed as a parent of a document or another section
+        # the only exception to this, is if a directive has called a nested parse,
+        # and specifically specified that sections are allowed to be created as children
+        # of its root node (a.k.a match_titles=True)
+        parent_of_temp_root = (
+            self.md_env.get("temp_root_node", None) is not None
+            and self.current_node == self.md_env["temp_root_node"]
+        )
+        if not (
+            parent_of_temp_root
+            or isinstance(self.current_node, (nodes.document, nodes.section))
+        ):
+            # if this is not the case, we create a rubric node instead
+            rubric = nodes.rubric(token.content, "", level=level)
             self.add_line_and_source_path(rubric, token)
             self.copy_attributes(token, rubric, ("class", "id"))
             with self.current_node_context(rubric, append=True):
                 self.render_children(token)
             return
 
-        level = int(token.tag[1])
-
         # create the section node
         new_section = nodes.section()
         self.add_line_and_source_path(new_section, token)
@@ -1769,7 +1780,7 @@ class DocutilsRenderer(RendererProtocol):
             if inline and not REGEX_DIRECTIVE_START.match(rendered):
                 self.nested_render_text(rendered, position, inline=True)
             else:
-                self.nested_render_text(rendered, position, allow_headings=False)
+                self.nested_render_text(rendered, position)
         finally:
             self.document.sub_references.difference_update(references)
 
diff --git a/myst_parser/mocking.py b/myst_parser/mocking.py
index 563711f..9c8848b 100644
--- a/myst_parser/mocking.py
+++ b/myst_parser/mocking.py
@@ -118,7 +118,7 @@ class MockState:
             reporter = self.document.reporter
             language = renderer.language_module_rst
             title_styles: list[str] = []
-            section_level = max(renderer._level_to_elem)
+            section_level = max(renderer._level_to_section)
             section_bubble_up_kludge = False
             inliner = self.inliner
 
@@ -174,7 +174,7 @@ class MockState:
             self._renderer.nested_render_text(
                 "\n".join(block),
                 self._lineno + input_offset,
-                allow_headings=match_titles,
+                temp_root_node=node if match_titles else None,
             )
         self.state_machine.match_titles = sm_match_titles
 
@@ -469,9 +469,7 @@ class MockIncludeDirective:
                     source_dir,
                     path.parent,
                 )
-            self.renderer.nested_render_text(
-                file_content, startline + 1, allow_headings=True
-            )
+            self.renderer.nested_render_text(file_content, startline + 1)
         finally:
             self.renderer.document["source"] = source
             self.renderer.reporter.source = rsource
diff --git a/myst_parser/parsers/docutils_.py b/myst_parser/parsers/docutils_.py
index 02f62cf..61c7bb4 100644
--- a/myst_parser/parsers/docutils_.py
+++ b/myst_parser/parsers/docutils_.py
@@ -257,6 +257,10 @@ class Parser(RstParser):
         :param inputstring: The source string to parse
         :param document: The root docutils node to add AST elements to
         """
+        from docutils.writers._html_base import HTMLTranslator
+
+        HTMLTranslator.visit_rubric = visit_rubric_html
+        HTMLTranslator.depart_rubric = depart_rubric_html
 
         self.setup_parse(inputstring, document)
 
@@ -350,3 +354,75 @@ def cli_xml(argv: Optional[List[str]] = None):
 def cli_pseudoxml(argv: Optional[List[str]] = None):
     """Cmdline entrypoint for converting MyST to pseudo-XML."""
     _run_cli("pseudoxml", "pseudo-XML", argv)
+
+
+def visit_rubric_html(self, node):
+    """Override the default HTML visit method for rubric nodes.
+
+    docutils structures a document, based on the headings, into nested sections::
+
+        # h1
+        ## h2
+        ### h3
+
+        <section>
+            <title>
+                h1
+            <section>
+                <title>
+                    h2
+                <section>
+                    <title>
+                        h3
+
+    This means that it is not possible to have "standard" headings nested inside
+    other components, such as blockquotes, because it would break the structure::
+
+        # h1
+        > ## h2
+        ### h3
+
+        <section>
+            <title>
+                h1
+            <blockquote>
+                <section>
+                    <title>
+                        h2
+            <section>
+                <title>
+                    h3
+
+    we work around this shortcoming, in `DocutilsRenderer.render_heading`,
+    by identifying if a heading is inside another component
+    and instead outputting it as a "non-structural" rubric node, and capture the level::
+
+        <section>
+            <title>
+                h1
+            <blockquote>
+                <rubric level=2>
+                    h2
+            <section>
+                <title>
+                    h3
+
+    However, docutils natively just outputs rubrics as <p> tags,
+    and does not "honor" the heading level.
+    So here we override the visit/depart methods to output the correct <h> element
+    """
+    if "level" in node:
+        self.body.append(self.starttag(node, f'h{node["level"]}', "", CLASS="rubric"))
+    else:
+        self.body.append(self.starttag(node, "p", "", CLASS="rubric"))
+
+
+def depart_rubric_html(self, node):
+    """Override the default HTML visit method for rubric nodes.
+
+    See explanation in `visit_rubric_html`
+    """
+    if "level" in node:
+        self.body.append(f'</h{node["level"]}>\n')
+    else:
+        self.body.append("</p>\n")
diff --git a/myst_parser/sphinx_ext/main.py b/myst_parser/sphinx_ext/main.py
index 7bb9a16..2dd0c04 100644
--- a/myst_parser/sphinx_ext/main.py
+++ b/myst_parser/sphinx_ext/main.py
@@ -1,8 +1,10 @@
 """The setup for the sphinx extension."""
 from typing import Any
 
+from docutils import nodes
 from sphinx.application import Sphinx
 
+from myst_parser.parsers.docutils_ import depart_rubric_html, visit_rubric_html
 from myst_parser.warnings_ import MystWarnings
 
 
@@ -28,6 +30,11 @@ def setup_sphinx(app: Sphinx, load_parser=False):
 
     app.add_post_transform(MystReferenceResolver)
 
+    # override only the html writer visit methods for rubric, to use the "level" attribute
+    app.add_node(
+        nodes.rubric, override=True, html=(visit_rubric_html, depart_rubric_html)
+    )
+
     for name, default, field in MdParserConfig().as_triple():
         if "sphinx" not in field.metadata.get("omit", []):
             # TODO add types?
diff --git a/myst_parser/warnings_.py b/myst_parser/warnings_.py
index 66e02a0..3bac946 100644
--- a/myst_parser/warnings_.py
+++ b/myst_parser/warnings_.py
@@ -26,8 +26,6 @@ class MystWarnings(Enum):
     """Missing Markdown footnote definition."""
     MD_HEADING_NON_CONSECUTIVE = "header"
     """Non-consecutive heading levels."""
-    MD_HEADING_NESTED = "nested_header"
-    """Header found nested in another element."""
 
     DIRECTIVE_PARSING = "directive_parse"
     """Issue parsing directive."""


Test patch:
diff --git a/tests/test_renderers/fixtures/docutil_syntax_elements.md b/tests/test_renderers/fixtures/docutil_syntax_elements.md
index 6778826..5b99631 100644
--- a/tests/test_renderers/fixtures/docutil_syntax_elements.md
+++ b/tests/test_renderers/fixtures/docutil_syntax_elements.md
@@ -111,6 +111,16 @@ Heading Levels:
             d
 .
 
+Nested heading
+.
+> # heading
+.
+<document source="notset">
+    <block_quote>
+        <rubric level="1">
+            heading
+.
+
 Block Code:
 .
     foo
diff --git a/tests/test_renderers/fixtures/reporter_warnings.md b/tests/test_renderers/fixtures/reporter_warnings.md
index 8b112b9..c5b26ab 100644
--- a/tests/test_renderers/fixtures/reporter_warnings.md
+++ b/tests/test_renderers/fixtures/reporter_warnings.md
@@ -160,7 +160,7 @@ header nested in admonition
 # Header
 ```
 .
-<string>:2: (WARNING/2) Disallowed nested header found, converting to rubric [myst.nested_header]
+
 .
 
 nested parse warning
diff --git a/tests/test_renderers/fixtures/sphinx_syntax_elements.md b/tests/test_renderers/fixtures/sphinx_syntax_elements.md
index e21e41c..2cca93b 100644
--- a/tests/test_renderers/fixtures/sphinx_syntax_elements.md
+++ b/tests/test_renderers/fixtures/sphinx_syntax_elements.md
@@ -111,6 +111,16 @@ Heading Levels:
             d
 .
 
+Nested heading
+.
+> # heading
+.
+<document source="<src>/index.md">
+    <block_quote>
+        <rubric level="1">
+            heading
+.
+
 Block Code:
 .
     foo