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

Markdown widget: Code blocks in lists are incorrectly ordered, sometimes don't appear #2676

Closed
jamescooke opened this issue May 28, 2023 · 10 comments · Fixed by #2803
Closed
Assignees
Labels
bug Something isn't working Task

Comments

@jamescooke
Copy link

Current behaviour

Very similar to this old issue in Rich: Textualize/rich#1036

When we use the same content (a list with a code block):

cat > order.md
This is a test.

1. first item
2. second item

       code block

Normal text

And then render it with Frogmouth:

frogmouth order.md

Then the code block does not appear:

Screenshot from 2023-05-28 16-34-15

If we tweak the code block to use triple backticks, then the block appears, but out of order:

cat > order2.md
This is a test.

1. first item
2. second item

   ``
   code block
   ``

Normal text

Please forgive my use of a double backtick above - I wasn't able to get a triple backtick to render in a code block in this markdown issue body 😬 . I've attached the file for confirmation: order2.md

frogmouth order2.md

Screenshot from 2023-05-28 16-36-40

Expected behaviour

Render code block as part of the second item consistently.

Rich appears to render both markdown excerpts OK (and they appear the same). Tested with rich==13.3.5
Screenshot from 2023-05-28 18-50-20

@davep
Copy link
Collaborator

davep commented May 28, 2023

This looks very much to be in the realm of the Textual, with this being handled by the Markdown widget. I'll move this over there.

@davep davep transferred this issue from Textualize/frogmouth May 28, 2023
@davep davep changed the title Code blocks in lists are incorrectly ordered, sometimes don't appear Markdown widget: Code blocks in lists are incorrectly ordered, sometimes don't appear May 28, 2023
@Textualize Textualize deleted a comment from github-actions bot May 28, 2023
@davep
Copy link
Collaborator

davep commented May 28, 2023

Given Markdown is based on markdown-it, the first thing to do here is be sure how it handles things. Looks like it renders this as expected:

Screenshot 2023-05-28 at 20 55 32

As such, at first glance, this would appear to be a bug in Markdown.

@davep davep added bug Something isn't working Task labels May 28, 2023
@jamescooke
Copy link
Author

Thanks for moving the issue from Frogmouth @davep

I've confirmed the behaviour is in the Textual Markdown widget as follows:

  • Used the markdown.py example file from the docs
  • Replaced EXAMPLE_MARKDOWN variable with the failing contents from order.md and order2.md in turn.
  • Ran each example with python markdown.py with textual==0.26.0 installed.
  • Received the same render as Frogmouth (with code block not appearing with order.md and out of order in order2.md) as screenshotted in the original issue description.

@TomJGooding
Copy link
Contributor

I'm sure that @davep is already way ahead of me on this, but posting anyway just in case it helps!

I experimented with a blockquote inside a list item and it looks like Textual renders this as expected:

image

I'm still trying to get to grips with the Markdown update method, but it looks like a blockquote token type is appended to the stack, whereas a code block is one of few token types (fence) that is instead appended to the ouput.

I hoped it might be as simple as changing this to the stack, but this breaks the docs markdown_viewer.py example.

@davep
Copy link
Collaborator

davep commented Jun 19, 2023

Half of the above will be fixed by #2803 (the part above indented code blocks).

I'm about to take a look at the problem of the code block appearing out of order when part of a list. The general problem can be seen here:

Screenshot 2023-06-19 at 14 16 56

I've not dived into the code yet, but I wanted to get an overview of what the widget tree looks like. Right off it seems clear that a list is an "atomic" or "leaf" widget, rendering its content and not containing other widgets (in other words the items in the list aren't widgets). Meanwhile the code is a MarkdownFence so can't be inline in the list.

This fits perfectly with what @TomJGooding finds above.

@davep
Copy link
Collaborator

davep commented Jun 19, 2023

Actually, scratch that, I'm talking nonsense (for a change). Turns out that I was managing to render the tree just a little too soon after doing the update on the Markdown. (╯°□°)╯︵ ┻━┻

So, after I wait a moment and then refresh things the tree makes a lot more sense:

Screenshot 2023-06-19 at 14 54 58

@davep davep self-assigned this Jun 19, 2023
davep added a commit to davep/textual that referenced this issue Jun 19, 2023
See Textualize#2676

Co-authored-by: TomJGooding <101601846+TomJGooding@users.noreply.github.com>
@davep
Copy link
Collaborator

davep commented Jun 19, 2023

@TomJGooding You hoped right, like 99% of the way there right anyway:

I hoped it might be as simple as changing this to the stack, but this breaks the docs markdown_viewer.py example.

Came down to e2dd711

@davep davep linked a pull request Jun 19, 2023 that will close this issue
@TomJGooding
Copy link
Contributor

Ah, that makes sense. I don't think that co-author was really deserved, but thank you!

@davep
Copy link
Collaborator

davep commented Jun 19, 2023

I don't think that co-author was really deserved, but thank you!

You found the right spot and mostly found the fix, I just copied your homework and tweaked it. ;-)

willmcgugan pushed a commit that referenced this issue Jun 20, 2023
* Initial set of Markdown widget unit tests

Noting too crazy or clever to start with, initially something to just test
the basics and to ensure that the resulting Textual node list is what we'd
expect.

Really just the start of a testing framework for Markdown.

* Allow handling of an unknown token

This allow for a couple of things:

1. First and foremost this will let me test for unhandled tokens in testing.
2. This will also let applications support other token types.

* Update the Markdown testing to get upset about unknown token types

* Treat a code_block markdown token the same as a fence

I believe this should be a fine way to solve this. I don't see anything that
means that a `code_block` is in any way different than a fenced block that
has no syntax specified.

See #2781.

* Add a test for a code_block within Markdown

* Allow for inline fenced code and code blocks

See #2676

Co-authored-by: TomJGooding <101601846+TomJGooding@users.noreply.github.com>

* Update the ChangeLog

* Improve the external Markdown elements are added to the document

* Improve the testing of Markdown

Also add a test for the list inline code block

* Remove the unnecessary pause

* Stop list items in Markdown being added to the focus chain

See #2380

* Remove hint to pyright/pylance/pylint that it's okay to ignore the arg

---------

Co-authored-by: TomJGooding <101601846+TomJGooding@users.noreply.github.com>
@github-actions
Copy link

Don't forget to star the repository!

Follow @textualizeio for Textual updates.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Task
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants