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

Bug: Markdown renderer messes with embedded HTML #2312

Closed
halfdan opened this issue Mar 2, 2014 · 5 comments · Fixed by #2451
Closed

Bug: Markdown renderer messes with embedded HTML #2312

halfdan opened this issue Mar 2, 2014 · 5 comments · Fixed by #2451
Assignees
Labels
affects:editor Work relating to the Koenig Editor bug [triage] something behaving unexpectedly
Milestone

Comments

@halfdan
Copy link
Contributor

halfdan commented Mar 2, 2014

Issue Summary

Markdown supports HTML and should just print it out as is. The showdown renderer seems to add additional <br />s after certain tags, causing them to break.

Steps to Reproduce

Enter the following text into the markdown editor:

Test

<table class="test">
  <tr>
    <td>Foo</td>
  </tr>
  <tr>
    <td>Bar</td>
  </tr>
</table>

<audio class="podcastplayer" controls>
    <source src="foobar.mp3" type="audio/mp3" preload="none"></source>
    <source src="foobar.off" type="audio/ogg" preload="none"></source>
</audio>

Test

Result:

<p>Test</p>

<table>  
  <tr>    
      <td>Foo</td>
  </tr>
  <tr>    
      <td>Bar</td>
  </tr>
</table>

<p><audio class=“podcastplayer” controls> <br />
    <source src="foobar.mp3" type="audio/mp3" preload="none"></source>
    <source src="foobar.off" type="audio/ogg" preload="none"></source>
</audio></p>

<p>Test</p>

Note that the table renders just fine without additional elements added. The audio tag however is surrounded by a <p> and has an additional <br /> added inside causing the element to break in Chrome (at least for me).

Note that the rendered markdown also contains fancy quotation marks (which is probably related to #1795).

screen shot 2014-03-02 at 16 30 02

Technical details

  • Ghost Version: master - latest commit: b84c8a4
  • Browser: Chrome 33
@halfdan
Copy link
Contributor Author

halfdan commented Mar 2, 2014

Most likely caused by the incomplete list of block tags in https://github.com/coreyti/showdown/blob/master/src/showdown.js#L328

@ErisDS ErisDS added this to the 0.7 milestone Mar 2, 2014
@ErisDS ErisDS added bug labels Mar 2, 2014
@brianchirls
Copy link

@halfdan Thanks for filing this. But I don't think the problem is in the block tag list.

The problem appears to be in showdown_typeography, which is doing the actual work of replacing the quotes. Disabling the extension fixes inline HTML rendering.

showdown_typeography attempts to parse out exceptions to quotable text, such as inline code blocks, but it does not consider inline HTML. Foolproof parsing of HTML cannot be done with a regular expression and requires a true parser, but one can use a regex to find opening tags that may contain attributes. The following code in typeography.js seems to address the issue (a little clean-up and testing wouldn't hurt):

    // Extract html blocks
    text = text.replace(/<([a-z][a-z0-9\-]*)\s+([a-z][a-z0-9\-]*(="[^"]*")\s*)*\/?>/ig, function (match) {
            i += 1;
            iCodeblocks[i] = match;
            return "{typog-icb-" + i + "}";
        });

There are some additional scenarios where inappropriate fancy quotes remain, such as inside of a <textarea>, but I think a more thorough investigation into how that is typically handled in code like smartypants warrants its own ticket.

I suggest patching this as soon as possible and filing another issue for a more thorough refactor of showdown_typeography for improved performance (regex fewer passes), to replace the potentially colliding {typog-icb-" + i + "} and for improved fancy quote exceptions.

I hope this bug does not have to wait until Q3 2014, since it is already causing problems with the sample post that ships with Ghost and breaks any imports from Wordpress, which are encoded as HTML.

@halfdan
Copy link
Contributor Author

halfdan commented Mar 6, 2014

@brianchirls I am talking about two different issues. The first is most certainly caused by the block tag list. With the latter you're probably right in that the typography extension causes this.

@brianchirls
Copy link

Ah yes, you're right @halfdan. But I think they both fall under "messes with embedded HTML", so hopefully they'll be patched soon.

@ErisDS
Copy link
Member

ErisDS commented Mar 13, 2014

The typography issue is reasonably easy to fix. I believe I have a fix for the showdown block HTML issue as well but it's pretty involved.

ErisDS added a commit to ErisDS/Ghost that referenced this issue Mar 20, 2014
fixes TryGhost#2312

- showdown fork understands more html tags
ErisDS added a commit to ErisDS/Ghost that referenced this issue Mar 20, 2014
issue TryGhost#2312

- The typography extension is still interfering in HTML blocks, reference style links and other bits and pieces it probably shouldn't be :(
- We'll add it back when it's ready.
tigefa4u pushed a commit to tigefa4u/Ghost that referenced this issue Aug 3, 2022
refs https://github.com/TryGhost/Team/issues/1435
refs TryGhost/Admin@a47b61c

A recent change for showing all subscriptions of a member on detail screen introduced a circular json structure with subscription -> tiers -> subscriptions, which throws an error on saving any member with paid subscription on member detail screen.
tigefa4u pushed a commit to tigefa4u/Ghost that referenced this issue Aug 3, 2022
refs https://github.com/TryGhost/Team/issues/1435
refs TryGhost/Admin@a47b61c

A recent change for showing all subscriptions of a member on detail screen introduced a circular json structure with subscription -> tiers -> subscriptions, which throws an error on saving any member with paid subscription on member detail screen.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects:editor Work relating to the Koenig Editor bug [triage] something behaving unexpectedly
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants