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.

Sign up for GitHub

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

Fixes #27605: inline math blocks can't be preceeded/followed by alphanumerical characters #30175

Merged
merged 7 commits into from
Apr 2, 2024
Merged

Fixes #27605: inline math blocks can't be preceeded/followed by alphanumerical characters #30175

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
1edf43a
Fix #27605: inline math blocks can't be preceeded/followed by alphanu…
jmlt2002 Mar 28, 2024
e375bf3
Add 2 test cases
jmlt2002 Mar 29, 2024
88f41f3
Allow math expressions to be preceeded but not suceeded by alphanumer…
jmlt2002 Mar 31, 2024
eb7546e
Allow math expressions to be suceeded only by punctuation.
jmlt2002 Apr 1, 2024
774e664
Merge branch 'main' into fix-inline-math-blocks-preceeded-followed-al…
GiteaBot Apr 2, 2024
6c95855
Merge branch 'main' into fix-inline-math-blocks-preceeded-followed-al…
GiteaBot Apr 2, 2024
15abbcc
Merge branch 'main' into fix-inline-math-blocks-preceeded-followed-al…
GiteaBot Apr 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions modules/markup/markdown/markdown_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -509,15 +509,23 @@ func TestMathBlock(t *testing.T) {
},
{
`$a a$b b$`,
`<p><code class="language-math is-loading">a a$b b</code></p>` + nl,
`<p><code class="language-math is-loading">a a</code>b b$</p>` + nl,
},
{
`a a$b b`,
`<p>a a$b b</p>` + nl,
},
{
`a$b $a a$b b$`,
`<p>a$b <code class="language-math is-loading">a a$b b</code></p>` + nl,
`<p>a<code class="language-math is-loading">b </code>a a<code class="language-math is-loading">b b</code></p>` + nl,
},
{
"a$x$",
`<p>a<code class="language-math is-loading">x</code></p>` + nl,
},
{
"$x$a",
`<p><code class="language-math is-loading">x</code>a</p>` + nl,
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, the new tests do not seem right.

$x$a shouldn't be considered as math expr IMO

Copy link
Contributor Author

@jmlt2002 jmlt2002 Mar 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It seems that GitHub doesn't allow preceding/succeeding alphanumeric characters, additionally it also doesn't allow any preceding characters at all, whereas succeeding non-alphanumerical characters are allowed. GitLab's behaviour seems a bit irregular since it allows succeeding alphabetical characters but not numerical characters. Imo, both of these seem a bit irregular overall and it would be better to render everything.

May i ask why is it that $x$a shouldn't be considered a math expression, and what would be the correct behaviour for the rendering?

Copy link
Contributor

@wxiaoguang wxiaoguang Mar 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

That's just my personal feeling, if it is possible I think it's better to use some stricter syntax. If $x$a is considered as a math expr, then something like The price is between US$1 and US$10 / The price is between $1 and $10 will be rendered incorrectly? (haven't really tested, just a guess)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see now, the behaviour you described will indeed render incorrectly.
image
I will change my code to accept only preceding characters but not succeeding characters.

Copy link
Contributor Author

@jmlt2002 jmlt2002 Mar 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

88f41f3

It's implemented here.

},
{
"$$a$$",
Expand Down
16 changes: 3 additions & 13 deletions modules/markup/markdown/math/inline_parser.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,11 +41,6 @@ func (parser *inlineParser) Trigger() []byte {
return parser.start[0:1]
}

func isAlphanumeric(b byte) bool {
// Github only cares about 0-9A-Za-z
return (b >= '0' && b <= '9') || (b >= 'A' && b <= 'Z') || (b >= 'a' && b <= 'z')
}

// Parse parses the current line and returns a result of parsing.
func (parser *inlineParser) Parse(parent ast.Node, block text.Reader, pc parser.Context) ast.Node {
line, _ := block.PeekLine()
Expand All @@ -55,12 +50,6 @@ func (parser *inlineParser) Parse(parent ast.Node, block text.Reader, pc parser.
return nil
}

precedingCharacter := block.PrecendingCharacter()
if precedingCharacter < 256 && isAlphanumeric(byte(precedingCharacter)) {
// need to exclude things like `a$` from being considered a start
return nil
}

// move the opener marker point at the start of the text
opener := len(parser.start)

Expand All @@ -75,14 +64,15 @@ func (parser *inlineParser) Parse(parent ast.Node, block text.Reader, pc parser.
ender += pos

// Now we want to check the character at the end of our parser section
// that is ender + len(parser.end)
// that is ender + len(parser.end) and check if char before ender is '\'
pos = ender + len(parser.end)
if len(line) <= pos {
break
}
if !isAlphanumeric(line[pos]) {
if line[ender-1] != '\\' {
break
}

// move the pointer onwards
ender += len(parser.end)
}
Expand Down