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

com_content Ensure introtext and fulltext are valid html #23242

Closed
wants to merge 1 commit into from

Conversation

Chrissi2812
Copy link

We had issues with the readmore tag causing the webpage layout to collaps due to unclosed <div>

Summary of Changes

The content for introtext and fulltext are parsed by DOMDocument.
So no longer invalid html gets saved to database.

Testing Instructions

Try to save following article with a Readmore tag.

<div>This is a Simple Text</div>
<div><hr id="system-readmore" /></div>
<div>The Fulltext</div>

Expected result

The Intro should be:

<div>This is a Simple Text</div>

Or

<div>This is a Simple Text</div>
<div></div>

Actual result

<div>This is a Simple Text</div>
<div>

This breaks the article listing.

Documentation Changes Required

None.

The splitting may caused html to get invalid (missing end tags).
@mbabker
Copy link
Contributor

mbabker commented Dec 7, 2018

This type of behavior honestly belongs in a model (or potentially a plugin, but definitely not the table class), and presumably behind some kind of option. This is one of those cases where you're changing user input without any type of communication and even though it has the best of intentions we still shouldn't be doing that. Also, be aware that DOMDocument lacks HTML5 support and can potentially wreak havoc on your content.

As far as the DomHelper class goes, new files shouldn't be added directly to the libraries/vendor directory in the CMS as these all come from external packages/repositories. This class should be submitted as a pull request to https://github.com/joomla-framework/utilities instead to be added to the same spot.

@Chrissi2812
Copy link
Author

Thanks for your advice with the plugin 👍🏽

I have put together a simple plugin

@ggppdk
Copy link
Contributor

ggppdk commented Dec 10, 2018

Some 3rd party editors are already trying to do this at browser (client) side, to close and reopen at least 1 parent HTML tag (or all the nested of tags)

you were testing with tinymce ?
If this does not work with tinymce (i have not tested yet) , then this is a real bug

I do not say to make a fix or not at client side
but at least a fix in the Table or in the Model should be made

@Chrissi2812
Copy link
Author

@ggppdk Yes I have used tinymce.
It only happens when the content contains a <div>.

We didn't encountered it our self (a client found this) so we are not 100% sure were the content got pasted from.

To replicate you have to use this in Source Edit mode:

<div></div>

After this all line breaks are new <div> and cause the <hr> to be wrapped by these.

So is this a bug of joomla or tinymce?

@ggppdk
Copy link
Contributor

ggppdk commented Dec 10, 2018

It is definitely not a tinyMce bug, as introtext and fulltext are not related to tinyMce code ...
i just mentioned the fact that some 3rd party editors choose to try to make a fix at client side

and also a possibility is to make fix at client side, but even in this case it is not tinyMce bug

About being a bug or not
since we cannot guarantee where the user will place the readmore,

i would say it is an issue with any core or 3rd party extension
that does not make a check if readmore has broken the HTML in bad place

@mbabker
Copy link
Contributor

mbabker commented Dec 10, 2018

i would say it is an issue with any core or 3rd party extension
that does not make a check if readmore has broken the HTML in bad place

Even if it is a bug the approach in this PR has too many potential issues to be included in core.

  • DOMDocument is known to not be HTML5 friendly
  • The hook modifying the data is too close to the database (this is the type of validation that belongs in a model with a parameter and potential alert to the user, not in a table (ORM entity) with no parameter and no alert to the user their markup may have changed (and if the readmore split is actually done in the table I'd say that's wrong too, that should be a model doing it)
  • If anything, the check should at most ensure that when the content is split that whatever element is open before the readmore tag is closed (and only that element) on the introtext side, similar for the fulltext side; we shouldn't be parsing/processing the entire text field

@ggppdk
Copy link
Contributor

ggppdk commented Dec 10, 2018

TinyMce 4 has "CMS support" with a pagebreak plugin that will close parent tags it also accepts custom text as break

https://www.tiny.cloud/docs/plugins/pagebreak/

maybe we can make use of it for Joomla readmore and pagebreak buttons ?

@mbabker
Copy link
Contributor

mbabker commented Dec 10, 2018

We need a solution that's either part of Joomla's Editor JavaScript API or a server-side solution. Relying on editor specific plugins isn't portable; the same problem would just crop up if you don't use the core TinyMCE option.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants