- Issue created by @ultimike
- 🇮🇪Ireland lostcarpark
It's also worth noting that the current implementation does not correctly handle line breaks in accordance with the Markdown spec.
Currently line breaks within a paragraph always get converted to a
<br>
tag. Markdown is intended to allow paragraphs to be formatted with line breaks in the source document, to allow easy readability while composing. To insert a line break, a line should either be ended with 2 or more spaces, or a HTML<br>
tag.I think removing the reliance on the "Convert line breaks" filter would allow CommonMark to handle line breaks correctly according to the Markdown spec, though I haven't yet tested this.
- @lostcarpark opened merge request.
- 🇮🇪Ireland lostcarpark
This change didn't require much, other than disabling the requirement for the "Convert line breaks" filter.
Once disabled, all that's needed is to add
<p>
and<br>
to the list of allowed tags.CommonMark then handles line breaks correctly.
I would like to add some test cases around correct formatting of line breaks, then should be ready for review.
- 🇺🇸United States ultimike Florida, USA
Great - thank you, @lostcarpark!
MR looks great (so far 😀)
I don't recall why I felt the need to encourage the use of the "Convert line breaks into HTML" text filter in the first place. I partially blame @lostcarpark because I'm pretty sure we were sitting together at Dev Days Vienna when I was working on this 😜
-mike
- 🇺🇸United States ultimike Florida, USA
Changing to work against the 2.0.x branch. Hopefully, no refactoring necessary. 🤞🏼
- 🇧🇪Belgium Dries
Thank you for working on this so quickly.
The merge request looks good to me. It feels like a triple win: less code, easier to setup, and better Markdown compatibility.
Just a small heads-up that the README.md still needs to be updated to reflect this change. I didn't see that in the MR.
- 🇮🇪Ireland lostcarpark
Thanks for the feedback.
I'll make sure I update the README.
I also want to improve test coverage (there's one test that's passing that I think should fail with this change, so I'm wondering if it was actually working as expected).
- 🇮🇪Ireland lostcarpark
Rebased against 2.0.x branch, and fixed conflicts.
- I added a test,
testParagraphs()
that line breaks are handled correctly. - I updated the checks of Markdown Easy filters in Status report, and updated the test of it using
xpath
, as the CSS based checks seemed to always be passing, even when they shouldn't. - I updated the "tips" list on the config form, which was including the list HTML in the translatable string. It's now using an
item_list
render array, with separate translatable strings. - Finally, I've updated the README, and hope I've adequately covered the changes.
Some concerns:
- We now have 6 bullets in the "tips" on the filter page. Is that too much, and is there scope for rationalizing?
- The README could do with some careful review to make sure my additions are correct, and that I haven't missed anything.
- I added a test,
- 🇧🇪Belgium Dries
I reviewed the patch and it looks good.
That said, it might change quite a bit depending on what happens with the "Limit allowed HTML tags" filter. It could be best to wait for that decision?
- 🇺🇸United States ultimike Florida, USA
Before finalizing this issue, let's decide on #3530906
(Very) temporarily postponing this issue.
-mike