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

Make images inline elements #43

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bensyverson
Copy link

Summary

tl;dr: This one-line PR allows images to create new paragraphs, by treating them like links, which are also inline elements.

Overview

This PR addresses an issue with the way that Ink currently handles images. Essentially, when an image is encountered outside of an existing paragraph fragment, Ink treats it as a fragment at the same level as Heading, HTML, Blockquote, CodeBlock, HorizontalLine or Paragraph.

This prevents a bare image in its own paragraph from being surrounded by <p></p>, and probably causes some other undesirable behavior. Images should be treated the same as links, which are very similar and do have the expected behavior in Ink.

Example

Same as a link, an image passed to the parser on its own should result in a surrounding paragraph:

![](img.jpg) -> <p><img src="img.jpg"/></p>

And the following Markdown should generate two paragraphs:

![cat](cat.jpg)

![dog](dog.jpg)

⚡️

<p><img src="cat.jpg" alt="cat"/></p>
<p><img src="dog.jpg" alt="dog"/></p>

However, currently Ink outputs:

<img src="cat.jpg" alt="cat"/><img src="dog.jpg" alt="dog"/>

Regression / Implications

Drafting this PR uncovered a different bug which I did not address. The unit tests in ImageTests.swift put their references on the next line, like so:

func testImageWithReference() {
    let html = MarkdownParser().html(from: """
    ![][url]
    [url]: https://swiftbysundell.com
    """)

    XCTAssertEqual(html, #"<p><img src="https://swiftbysundell.com"/></p>"#)
}

Meanwhile, the Link tests all separate the reference by another line:

func testLinkWithReference() {
    let html = MarkdownParser().html(from: """
    [Title][url]

    [url]: swiftbysundell.com
    """)

    XCTAssertEqual(html, #"<p><a href="swiftbysundell.com">Title</a></p>"#)
}

Testing against other Markdown parsers, either style should work. However, currently (and also in this PR) in Ink, the reference must be separated by more than just a newline. And by making images behave more like links, those Image tests stopped working.

So I introduced newlines in the Image tests to separate the references, and they are at least at parity with links now.

Next steps

I made an effort to fix the reference behavior so that the following test would pass, but couldn't immediately see how to resolve it. @JohnSundell you may be able to work it out a lot faster!

func testLinkWithReferenceOnNewline() {
    let html = MarkdownParser().html(from: """
    [Title][url]
    [url]: swiftbysundell.com
    """)

    XCTAssertEqual(html, #"<p><a href="swiftbysundell.com">Title</a></p>"#)
}

Thanks again John!

Copy link
Contributor

@john-mueller john-mueller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 from me. This PR makes three additional CommonMark tests pass, without any regressions (574, 578, and 579 in the spec).

As for needing a blank line between a reference link/image and the link reference definition, the good news is that CommonMark actually does require that space. Try the following in the dingus:

[Title][url]

[url]: swiftbysundell.com

produces

<p><a href="swiftbysundell.com">Title</a></p>

but

[Title][url]
[url]: swiftbysundell.com

produces

<p>[Title][url]
[url]: swiftbysundell.com</p>

(and similarly for images).

Note that in the case with no blank line, Ink does produce different output than CommonMark, which should be dealt with eventually, but the point remains that we don't have to make it produce the same output as with the blank line.

@bensyverson
Copy link
Author

Oh, that's good news @john-mueller! I've noticed that some MD parsers do respect a reference link with no blank line, but it's inconsistent. Either way, it's definitely an edge case. Thanks for taking a look at this PR!

@john-mueller
Copy link
Contributor

Hey @bensyverson, I'm hoping that @JohnSundell will have some time to merge these soon. I'm thinking it would be good to go ahead and rebase this commit on top of #42 (in other words, rebase your feature/inline-images branch on top of your feature/link-titles branch. The unit tests from #42 will then need <p> tags added in the appropriate places so they keep passing.

I'm thinking it would look something like my inline-images-fixup branch.

After that, maybe just add a note that this PR depends on those changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants