search

getkirby / kirby

Kirby's core application folder
https://getkirby.com
Other
1.32k stars 168 forks source link

Closing `</source>` tag in `<picture>` element with `markdown.extra` #4406

Closed helllicht closed 2 years ago

helllicht commented 2 years ago

Description

Kirby adds a closing </source> tag inside a picture element when markdown.extra is true.

Screenshot kirby-picture-bug

To reproduce

Example panel field

fields:
  text:
    type: textarea

Example field content

Some Text

<picture><source srcset="https://via.placeholder.com/800x600.webp 1400w" sizes="100vw" type="image/webp"><img src="https://via.placeholder.com/800x600.jpg" alt=""></picture>

Setup

Additional context Validator says: "Error: Stray end tag source."

afbora commented 2 years ago

@helllicht Can you share how you use it in textarea field (directly markdown codes) to test it please?

helllicht commented 2 years ago

@afbora You're right! Added some example code in the description for easier reproduction. Hope that helps.

afbora commented 2 years ago

I can reproduce the issue with picture > source, audio > source and video > source. So generally related with nested tags with void elements.

But seems this issue related with parsedown library. I'm not sure that we can fix it 🤔

lukasbestle commented 2 years ago

Parsedown Extra uses PHP's DOMDocument class to parse and output the markup. Seems like DOMDocument doesn't know about the source element and its void nature. Or maybe Parsedown doesn't tell it that we target a HTML5 output, so it uses XHTML mode.

It may help to manually self-close the source tag as if it was XHTML:

Some Text

<picture><source srcset="https://via.placeholder.com/800x600.webp 1400w" sizes="100vw" type="image/webp" /><img src="https://via.placeholder.com/800x600.jpg" alt=""></picture>
afbora commented 2 years ago

As far as I have looked the Parsedown codes, it seems a little difficult to solve this on the Parsedown side. IMO @lukasbestle solution is ideal 💯

Also here was the related idea/issue about quitting Parsedown: https://github.com/getkirby/kirby/issues/1961

helllicht commented 2 years ago

Parsedown Extra uses PHP's DOMDocument class to parse and output the markup. Seems like DOMDocument doesn't know about the source element and its void nature. Or maybe Parsedown doesn't tell it that we target a HTML5 output, so it uses XHTML mode.

It may help to manually self-close the source tag as if it was XHTML:

Some Text

<picture><source srcset="https://via.placeholder.com/800x600.webp 1400w" sizes="100vw" type="image/webp" /><img src="https://via.placeholder.com/800x600.jpg" alt=""></picture>

Good suggestion, but unfortunately this doesn't work – it's only moving the </source> tag directly after the opening tag:

<picture><source srcset="[https://via.placeholder.com/800x600.webp 1400w](https://via.placeholder.com/800x600.webp)" sizes="100vw" type="image/webp"></source><img src="https://via.placeholder.com/800x600.jpg" alt=""></picture>
lukasbestle commented 2 years ago

Could you please try if it works if you put the following in line 580 of kirby/dependencies/parsedown-extra/ParseDownExtra.php:

$DOMDocument->loadHTML('<!DOCTYPE html>' . $elementMarkup);

It should also work with your original example.

helllicht commented 2 years ago

@lukasbestle Just tried it, unfortunately does't change anything in the outcome…

lukasbestle commented 2 years ago

Then I'm afraid this is an issue with the PHP DOMDocument class. It just seems to lack proper support for HTML5. :(

There is an extended HTML5DOMDocument library, but I'm not sure about its performance impact and stability.

So to be honest I feel like we cannot solve this directly in Kirby. What you could do is the following:

Alternatively you can make your markdown component use a totally different Markdown implementation. In https://github.com/getkirby/kirby/issues/1961 we have collected some candidates.