Issue with urls inside markdown tables

[martinagvilas]

Hi! I'm trying to upgrade the turing way book to the new jupyter-book beta version and found a bug when I want to link a url inside a table. I'm kind of new in the world of sphinx, commonmark and parsers, so this might be a bug I need to report to sphinx instead of here. I'm sorry if that is the case!

To give a reproducible example of the error, if I want to build jupyter-book using a single markdown file containing the following:

| Prerequisite | Importance | Notes |
|---|---|---|
| Version Control | Very Important | |
| Reproducible Environments | Very Important | [Binder](https://mybinder.org/) |

(rendered markdown version below)

Prerequisite	Importance	Notes
Version Control	Very Important
Reproducible Environments	Very Important	Binder

I get the following error:

Exception occurred:
  File "/Users/martina.gonzales/anaconda3/envs/executable-book/lib/python3.7/site-packages/sphinx/writers/html5.py", line 211, in visit_reference
    assert len(node) == 1 and isinstance(node[0], nodes.image)
AssertionError

But if I link it outside the table:

[Binder](https://mybinder.org/)

there is no issue.

One possible reason for the issue, but I might be totally wrong in this: Reading the sphinx error message in context, it seems that the writer in sphinx is expecting the node parent of the reference to be a TextElement. But when I used the MyST API to manually convert these texts to docutils I get that the parent of the reference is <entry>, which I don't think is of text kind. In the isolated reference case (when is not inside the table), the parent node is <paragraph>.

I'm using:

• sphinx [2.4.4]
• myst-parser [0.8.0]
• python [3.7.7]

cc @choldgraf

[choldgraf]

Hmm, I can confirm this breaks on my machine too, and with this minimal example:

| [mybinder](https://mybinder.org) |
|----------------------------------|

I think that the fix would be somewhere here: https://github.com/ExecutableBookProject/MyST-Parser/blob/f28529919f261178ec7add8dcd61fbdb594123f1/myst_parser/docutils_renderer.py#L531 but maybe @chrisjsewell has an idea?

[chrisjsewell]

Hey @martinagvilas thanks for the report.

Well a little lower down: https://github.com/ExecutableBookProject/MyST-Parser/blob/f28529919f261178ec7add8dcd61fbdb594123f1/myst_parser/docutils_renderer.py#L534

This is where children of the row are added as children of the entry node.

Firstly I would look at: https://github.com/ExecutableBookProject/MyST-Parser/blob/master/tests/test_renderers/fixtures/tables.md (in raw format), these are the tests defining exactly how the markdown is converted to docutils AST. We would want to add additional test(s) here to clarify the current/desired conversion.

Then I would look here: https://github.com/docutils-mirror/docutils/blob/master/test/test_parsers/test_rst/test_tables.py, at the equivalent behaviour for the rST parser.

As you mention, it may be that for your case the link is not being wrapped in a paragraph. Of the top of my head, it would also be good to check if this is always the case, or is it because you only have the single link in the cell, e.g. what would be the outcome for these:

[a](https://b.com)
-------------------
c

[a](https://b.com) d
----------------------
c

[choldgraf]

Quick update:

| [mybinder](https://mybinder.org) d|
|----------------------------------|

and

| [mybinder](https://mybinder.org) d|
----------------------------------|
| c |

do not work

[martinagvilas]

Indeed, inspecting the output of to_docutils() of

| [mybinder](https://mybinder.org) d|
|----------------------------------|

shows no paragraph wrapper in entry :

<document source="notset">
    <table classes="colwidths-auto">
        <tgroup cols="1">
            <colspec colwidth="100.0">
            <thead>
                <row>
                    <entry>
                        <reference refuri="https://mybinder.org">
                            mybinder
                         d
            <tbody>

as outputs in these tests expect

[martinagvilas]

What about changing this to something like this:

def render_table_row(self, token):
    row = nodes.row()
    with self.current_node_context(row, append=True):
        for child in token.children:
            entry = nodes.entry()
            para = nodes.paragraph("")
            style = child.attrGet("style")  # i.e. the alignment when using e.g. :--
            if style:
                entry["classes"].append(style)
            with self.current_node_context(entry, append=True):
                with self.current_node_context(para, append=True):
                        self.render_children(child)

... to wrap the paragraph as well.

I tried it and it seems to render ok.

[choldgraf]

@martinagvilas wow thanks for prototyping that fix! Glad that you were able to orient yourself to the codebase quickly. That looks reasonable to me, but @chrisjsewell would know better than I do. Are there other places in the code where we just "wrap" an element in another element?

[chrisjsewell]

Well its easy, when its such a well written code base 😆

Yeh that's pretty much it 👍 , I should be able to add it in the next day.
Or feel free to make a PR, but make sure the requisite tests are added, and we make want to also check that things like https://myst-nb.readthedocs.io/en/latest/use/glue.html#pasting-into-tables are still work before merging

[choldgraf]

Indeed - I must admit I also made a mental note that this must be a well-written codebase :-)

[martinagvilas]

It is a great codebase, I agree 🚀

I can try to submit a PR with some tests if you haven't started working on that already.

I checked the glue features (amazingly awesome 😄) and it seems to be rendering fine with the changes:

The plots do look smaller than here but I tried rendering the same tables without the <paragraph> wrapper changes and the size is the same, so I think it is a separate issue.

[ncalexan]

We also are having this exact issue: https://bugzilla.mozilla.org/show_bug.cgi?id=1828390.

[choldgraf]

@ncalexan I thought this was resolved in

#128

Can you confirm you're on the latest Myst parser?

[ncalexan]

@ncalexan I thought this was resolved in

#128

Can you confirm you're on the latest Myst parser?

We're on

pypi:myst-parser==1.0

(from https://searchfox.org/mozilla-central/rev/d405168c4d3c0fb900a7354ae17bb34e939af996/python/sites/docs.txt#7) and I see from https://pypi.org/project/myst-parser/ that the latest is 2.0.0, so we're well behind.

Thank you so much for suggesting the update, I'll see if we can get this fixed!