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

fix(markdown) strong/emphasis requires whitespace after #3520

Merged

Conversation

joshgoebel
Copy link
Member

Resolves #3519. Prevents false positive of emphasis when bullets are nested inside block quotes. This doesn't detect the bullets, it only fixes the false positive.

Checklist

  • Added markup tests, or they don't apply here because...
  • Updated the changelog at CHANGES.md

@joshgoebel joshgoebel requested a review from allejo June 27, 2022 17:51
@allejo
Copy link
Member

allejo commented Jul 8, 2022

Gonna start a quick discussion on this one. I did some light reading on the commonmark spec and learned two things:

  • newlines count as terminating the given bold/italic sequence
  • we'd get false positives with __ too if there's a space after it

Given that any whitespace (including newlines) terminate the sequence, maybe use \s instead of [ ]. So I've got this snippet:

const BOLD = {
  className: 'strong',
  contains: [], // defined later
  variants: [
    {
      begin: /_{2}(?!\s)/, // <- changed
      end: /_{2}/
    },
    {
      begin: /\*{2}(?!\s)/, // <- changed
      end: /\*{2}/
    }
  ]
};
const ITALIC = {
  className: 'emphasis',
  contains: [], // defined later
  variants: [
    {
      begin: /\*(?![*\s])/, // <- changed
      end: /\*/
    },
    {
      begin: /_(?![_\s])/, // <- changed
      end: /_/,
      relevance: 0
    }
  ]
};

(I'd submit it as a review but GitHub doesn't let me make changes on non-touched lines)

image

This PR as-is would result in the following,

image

@allejo
Copy link
Member

allejo commented Jul 8, 2022

The markdown I'm using:

** i shouldn't be italic**

* and I shouldn't be italic either*

__ not really bold__

_ not italic_

Hello

**actually bold**

__again, bold__

*italic*
_oh i'm italic_

How GitHub renders this


** i shouldn't be italic**

  • and I shouldn't be italic either*

__ not really bold__

_ not italic_

Hello

actually bold

again, bold

italic
oh i'm italic

@double-beep
Copy link

  • newlines count as terminating the given bold/italic sequence

Not completely accurate:

**not bold
**

*not italic
*

**should be
bold**

*should be
italic*

**not bold
**

*not italic
*

should be
bold

should be
italic

@joshgoebel
Copy link
Member Author

joshgoebel commented Jul 8, 2022

So I've got this snippet:

Did you try and see if all the tests still passed with that snippet?

@allejo
Copy link
Member

allejo commented Jul 8, 2022

So I've got this snippet:

Did you try and see if all the tests still passed with that snippet?

Ran it against npm run build_and_test and just got an auto-detect failure (but I think that's due to my Node version; this happens on main for me too).

  1720 passing (14s)
  3 pending
  1 failing

  1) hljs.highlightAuto()
       should be detected as sfz:

      AssertionError: sample.txt should be detected as sfz, but was gradle
      + expected - actual

      -gradle
      +sfz

Not completely accurate:

**not bold
**

*not italic
*

**should be
bold**

*should be
italic*

Ahhh good catch on this, you're right. Anything that's not "punctuation" before the closing sequence keeps it bold/italic. But if it's just a newline and closing, then it's not valid since the newline is considered "punctuation"

@allejo
Copy link
Member

allejo commented Oct 16, 2022

I don't mean to overstep and push to your branch, @joshgoebel! I'd like to vote that we fix the immediate false positives by disallowing newlines inside of bolds and italics. And in the future, come up with a more robust expression that matches the markdown spec and can handle newlines (or whatever new APIs that may be added).

@joshgoebel joshgoebel merged commit 71f5cb2 into highlightjs:main Oct 16, 2022
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.

(markdown) List items inside blockquotes/HTML comments aren't highlighted correctly
3 participants