themotte / rDrama

This code runs https://www.themotte.org. Forked from https://github.com/Aevann1/rDrama
GNU Affero General Public License v3.0
26 stars 31 forks source link

Comment preview does not match rendering #266

Open zorbathut opened 2 years ago

zorbathut commented 2 years ago

See https://www.themotte.org/post/30/bugs-suggestions-and-small-comments/1529?context=8#context

Add some unit tests for this one once you've fixed it!

zorbathut commented 2 years ago

https://www.themotte.org/post/30/bugs-suggestions-small-comments-and-site/7730?context=8#context

ToaKraka commented 2 years ago

New example:

> HOI4: $185  
> Stellaris: $235  
> EU4: $230 while on sale

Note that there are two spaces after each of the first two lines.

In the preview, this becomes a single p with two brs, all in a single blockquote, which is what's intended:

HOI4: $185
Stellaris: $235
EU4: $230 while on sale

However, in the actual comment, it instead becomes three ps in three separate blockquotes:

HOI4: $185

Stellaris: $235

EU4: $230 while on sale

zorbathut commented 2 years ago

Yeah the preview absolutely seems like the right outcome there.

The nice thing about test-suiting this all up is that it'll also make it easier to do careful tweaks.

ToaKraka commented 2 years ago

Supposedly, the Commonmark specification already "contains over 500 embedded examples which serve as conformance tests".

zorbathut commented 2 years ago

Oh nice! Yeah, that's a pretty good place to start; if we then want our own changes we can just riff of that.

ToaKraka commented 2 years ago

It looks like the implementation of Markdown that you're using achieves a compliance rate of approximately 60 percent versus the CommonMark specification. Maybe it's too late in the game for this suggestion, but you could consider switching from Marked to Commonmark's reference implementation. The impact on performance supposedly would be negligible. Disregard. The most recent test shows much higher compliance.

zorbathut commented 1 year ago

https://www.themotte.org/post/216/meta-something-shiny-and-two-things/45224?context=8#context

zorbathut commented 1 year ago

Formatting bug report: I just made this post: https://www.themotte.org/post/269/culture-war-roundup-for-the-week/49063?context=8#context

which has a numbered list in a > blockquote. In the preview it showed as one block with "1.", "2." as the numbers, but in the actual post it shows up as two separate blockquotes with "1.", "1." as the numbers.

zorbathut commented 1 year ago

https://www.themotte.org/post/216/meta-something-shiny-and-two-things/57338?context=8#context

justcool393 commented 1 year ago

https://github.com/themotte/rDrama/issues/266#issuecomment-1288108270

re linebreaks: @zorbathut this is WAI from rDrama's perspective but probably not from themotte's. there was some specific code added to sanitize.py to make single line breaks into paragraph breaks

https://www.themotte.org/post/216/meta-something-shiny-and-two-things/57849?context=8#context

the subject of the great holy \n war (i.e. me and @official-techsupport and @TLSM discussing it a bit and getting confused on over what each other was saying)

official-techsupport commented 1 year ago

For the record, IIRC the holy war ended with us all ending up against a certain giant rodent, reconciling our differences into:

  1. Get rid of markdown single-newline-elimination behavior.
  2. space-space-newline should produce a <br/> rather than </p><p>.
  3. don't do any of that inside of the code/preformatted blocks
justcool393 commented 1 year ago

yeah idk if that is what themotte wants though or if they just want to return to the standard markdown ebhavior

zorbathut commented 1 year ago

https://www.themotte.org/post/349/culture-war-roundup-for-the-week/63544?context=8#context

zorbathut commented 1 year ago

What's the markdown behavior? Can someone give me an example of the difference?

justcool393 commented 1 year ago

What's the markdown behavior? Can someone give me an example of the difference?

so the markdown behavior is to have two line breaks make a paragraph break (i.e.

text line 1

text line 2

is equivalent to

<p>text line 1</p>
<p>text line 2</p>

)

the reason 2 is required is apparently a thing from email systems where it's hard wrapped (source @TLSM in a discussion with me and also @Shog9 here https://meta.stackexchange.com/a/40978/546132). (you're supposed to do 2 spaces before a newline to get a single line break.)

anyway if you're not coming from that, it's kinda unintuitive, and rdrama hated it so changed it so that a single line break created a paragraph break (see L163-L164 of files/helpers/sanitize.py)

https://github.com/themotte/rDrama/blob/1e9ca628922132e3a97125940ee689a6db028277/files/helpers/sanitize.py#L163-L164

it's hard to know because the code has changed at random intervals for seemingly no good reason so i don't think even rdrama.net has correct behavior currently. anyway yeah, requires thinking about what the community prefers. there are, as i can tell, 3 options that are somewhat sane.

  1. the default markdown behavior (2 spaces before newline =
    or 2 lines = paragraph break)
  2. one line = line break, 2 lines = paragraph break
  3. single line = paragraph break, 2 spaces before line = line break

anyway... yeah.

zorbathut commented 1 year ago

https://github.com/themotte/rDrama/issues/253 also touches on an issue; monospace behaves differently in preview and not-preview

rudyon commented 1 year ago

some (most?) of the issues seem to be caused by mistletoe following commonmark. while marked follows GFM.

at somepoint (i couldn't track down the commit for this. sorry) someone realized that singletildes didn't work properly (which is normal actually since mistletoe follows commonmark and commonmark doesn't support singletildes) and worked around it with a regex.

https://github.com/themotte/rDrama/blob/143eb36aaa7c39a95d4406809b996303b279c05e/files/helpers/sanitize.py#L203-L204C66 (i can't get this to work for some reason sorry)

which causes issues when there is a backslash involved like https://www.themotte.org/post/30/bugs-suggestions-and-small-comments/1529?context=8#context

essentially there needs to be a decision upon whether to use commonmark or gfm going forwards. if it is gfm mistletoe needs to be expended (which mistletoe has support for and i already played with it) or replaced. if it is commonmark then marked needs to be expanded/restricted or replaced

my vote is on gfm because it is likely what people think of when they think of markdown

rudyon commented 1 year ago

btw i can indeed confirm that removing these lines https://github.com/themotte/rDrama/blob/1e9ca628922132e3a97125940ee689a6db028277/files/helpers/sanitize.py#L163-L164

fixes #440

(i have no clue why the code link works here but didn't here )

zorbathut commented 1 year ago

Nobody seems to object to GFM, so, GFM it is!

zorbathut commented 1 year ago

https://www.themotte.org/post/565/culture-war-roundup-for-the-week/117581?context=8#context

Something here is glitchy, another item to add to the queue I guess.

TLSM commented 1 year ago

https://www.themotte.org/post/565/culture-war-roundup-for-the-week/117581?context=8#context

The code block issue looks like something that got caught in the crossfire of e6f52711815ed0afbdaaf4a04205d7355674e6fe

pre {
   line-height: .5rem;
}

Perhaps the intended fix was line-height: 1 ? I think the issue was monospace blocks were displaying at 1.5 line height.

zorbathut commented 4 months ago

https://www.themotte.org/post/1063/culture-war-roundup-for-the-week/225669?context=8#context

table rendering isn't working right