Skip to content

Conversation

gwharton
Copy link
Contributor

@gwharton gwharton commented Aug 12, 2019

Description (*)

Firstly, we need to decide whether we allow <style> tags in TinyMCE editors. HTML5.1 and older forbid inline style tags (<style> is only valid in <head>), although most browsers allow it. HTML5.2 and newer now allow it, so lets get that out of the way and decided first.

Then......

There are two issues that needed to be resolved to allow <style> tags within the TinyMCE 4 editor.

Issue 1

In the decodeVariables function of the magentovariable TinyMCE plugin and in the removeDuplicateAncestorWidgetSpanElement function of the magentowidget TinyMCE plugin, the content of the TinyMCE editor is passed through a DOMParser to convert certain tags. Usually the content parsed through the DOMParser is available in the doc.body.innerHTML field, and is returned by the plugins once alterations had been made. There is an issue however if the content of the TinyMCE editor starts with a <style> tag. The DOMParser splits the style elements off and makes them available in doc.head.innerHTML and the remainder of the TinyMCE contents are parsed into doc.body.innerHTML.

Thus you can see how, in the original function that only processed doc.body.innerHTML, that any <style> tags were getting stripped out. Note that this only happens if the <style> tag is the first item in the editor. <p>test</p><style>test</style> would get passed in its entirety to doc.body.innerHTML and the bug would not happen, but <style>test</style><p>test</p> would get split between doc.head.innerHTML and doc.body.innerHTML and the <style> tags would be lost.

This PR corrects this by concatenating both doc.head.innerHTML and doc.body.innerHTML on return from the respective plugin functions.

Issue 2

The TinyMCE editor enforces strict HTML5 adherence in terms of what tags can go where. Up to HTML5.1 the <style> tag is not allowed in the <body> section of code. It can only be used in the <head> section. This can be confirmed in the source code to TinyMCE 4.9.5 which shows that in the preparation of the HTML5 schema, that <style> is not included as a valid tag within a <body> tag.

HTML5.2 however DOES now allow the <style> section to appear within <body>. This can be confirmed here. There are currently no versions of TinyMCE 4 that have been updated to allow it.

This PR adds <style> tag as valid to occur inside <body> in the TinyMCE config, and thus allows it to be a top level tag within a TinyMCE editor.

Fixed Issues (if relevant)

  1. Magento script and style filtering issue in CMS Blocks and Pages #22867: Magento script and style filtering issue in CMS Blocks and Pages

Manual testing scenarios (*)

  1. <p>test</p><style>test</style>
  2. <style>test</style><p>test</p>
  3. <p>test</p>
  4. <style>test</style>

Questions or comments

As suggested in the original description, there might be some contention as to whether to allow inline <style> tags in the first place.

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • All automated tests passed successfully (all builds are green)

@m2-assistant
Copy link

m2-assistant bot commented Aug 12, 2019

Hi @gwharton. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

  • @magento give me test instance - deploy test instance based on PR changes
  • @magento give me 2.3-develop instance - deploy vanilla Magento instance

In case you'd like to rerun tests use the following comments:

  • @magento run all tests - run all tests
  • @magento run { check name } - run a single test job (check name example: Static Tests build)

For more details, please, review the Magento Contributor Guide documentation.

@magento-engcom-team
Copy link
Contributor

Hi @VladimirZaets, thank you for the review.
ENGCOM-5607 has been created to process this Pull Request
✳️ @VladimirZaets, could you please add one of the following labels to the Pull Request?

Label Description
Auto-Tests: Covered All changes in Pull Request is covered by auto-tests
Auto-Tests: Not Covered Changes in Pull Request requires coverage by auto-tests
Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests

@gwharton
Copy link
Contributor Author

gwharton commented Aug 12, 2019

@VladimirZaets Cheers for the quick review. Static tests were failing due to coding standards failures, so i've just corrected them.

…<script> tag.

These opening tags were being lost due to being stripped out into the <head>
section when parsed through the dom object, and only the body returned, thus
losing any opening <script> tags.

Added <style> as valid child object to <body> in tinymce config. Without this
it is not possible to have a code block that starts with <style>.
@gwharton
Copy link
Contributor Author

gwharton commented Aug 13, 2019

The exact same problem exists with the script tag which are also being stripped. Would you like me to update this PR to encompass script as well or submit new PR?

@sidolov sidolov changed the title Allow TinyMCE to save content with inline <style> tags Allow TinyMCE to save content with inline style tags Aug 15, 2019
@engcom-Delta engcom-Delta self-assigned this Aug 19, 2019
@engcom-Delta
Copy link
Contributor

✔️ QA passed

@m2-assistant
Copy link

m2-assistant bot commented Aug 20, 2019

Hi @gwharton, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

@magento-engcom-team magento-engcom-team added this to the Release: 2.3.4 milestone Aug 20, 2019
@gwharton gwharton deleted the 2.3-develop-tinymcestyle branch August 21, 2019 09:11
@sidolov sidolov added the Auto-Tests: Not Covered Changes in Pull Request requires coverage by auto-tests label Sep 10, 2019
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.

6 participants