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

[3.9] Make TinyMCE options smaller & more readable. #19719

Conversation

Sophist-UK
Copy link
Contributor

@Sophist-UK Sophist-UK commented Feb 18, 2018

Summary of Changes

TinyMCE options are included in page HTML as inline JSON. This PR cleans up the script by replacing \r\n\t with spaces making the resulting HTML both smaller and more readable.

Testing Instructions

View HTML of existing page with TinyMCE.
Apply PR.
Compare revised HTML.

Previous result e.g.

script": [ "!(function(){\n\t\t\t\tvar getBtnOptions = new
Function(\"return {handler: \\'iframe\\', size: {x: 800, y:
500}}\"),\n\t\t\t\t\tbtnOptions = getBtnOptions(),\n\t\t\t\t\tmodalWidth
= btnOptions.size && btnOptions.size.x ?  btnOptions.size.x :
null,\n\t\t\t\t\tmodalHeight = btnOptions.size && btnOptions.size.y ?
btnOptions.size.y : null;\n\t\t\t\teditor.addButton(\"button-0Module\",
{\n\t\t\t\t\ttext: \"Module\",\n\t\t\t\t\ttitle:
\"Module\",\n\t\t\t\t\ticon: \"none icon-file-add\",\n\t\t\t\t\tonclick:
function () {\n\t\t\t\t\t\t\tvar modalOptions = {\n\t\t\t\t\t\t\t\ttitle
: \"Module\",\n\t\t\t\t\t\t\t\turl :
'http:\/\/www.longwinter.co.uk\/administrator\/index.php?option=com_modules&view=modules&layout=modal&tmpl=component&editor=jform_intro_text&780ac24446fbf734ef8b34ddbd2b92a1=1',\n\t\t\t\t\t\t\t\tbuttons:
[{\n\t\t\t\t\t\t\t\t\ttext   : \"Close\",\n\t\t\t\t\t\t\t\t\tonclick:
\"close\"\n\t\t\t\t\t\t\t\t}]\n\t\t\t\t\t\t\t}\n\t\t\t\t\t\t\tif(modalWidth){\n\t\t\t\t\t\t\t\tmodalOptions.width
=
modalWidth;\n\t\t\t\t\t\t\t}\n\t\t\t\t\t\t\tif(modalHeight){\n\t\t\t\t\t\t\t\tmodalOptions.height
=
modalHeight;\n\t\t\t\t\t\t\t}\n\t\t\t\t\t\t\teditor.windowManager.open(modalOptions);\n\t\t\t\t\t}\n\t\t\t\t});\n\t\t\t})();"]

New result

script": [ "!(function(){\nvar getBtnOptions = new Function(\"return
{handler: \\'iframe\\', size: {x: 800, y: 500}}\"),\nbtnOptions =
getBtnOptions(),\nmodalWidth = btnOptions.size && btnOptions.size.x ?
btnOptions.size.x : null,\nmodalHeight = btnOptions.size &&
btnOptions.size.y ? btnOptions.size.y : null;\neditor.addButton(\"button-0Module\", {\ntext: \"Module\",\ntitle:
\"Module\",\nicon: \"none icon-file-add\",\nonclick: function () {\nvar
modalOptions = {\ntitle : \"Module\",\nurl :
'http:\/\/www.longwinter.co.uk\/administrator\/index.php?option=com_modules&view=modules&layout=modal&tmpl=component&editor=jform_intro_text&780ac24446fbf734ef8b34ddbd2b92a1=1',\nbuttons: [{\ntext : \"Close\",\nonclick: \"close\"\n}]\n}\nif(modalWidth){\nmodalOptions.width = modalWidth;\n}\nif(modalHeight){\nmodalOptions.height
= modalHeight;\n}\neditor.windowManager.open(modalOptions);\n}\n});\n})();",]

e.g. from:
```
script": [ "!(function(){\n\t\t\t\tvar getBtnOptions = new
Function(\"return {handler: \\'iframe\\', size: {x: 800, y:
500}}\"),\n\t\t\t\t\tbtnOptions = getBtnOptions(),\n\t\t\t\t\tmodalWidth
= btnOptions.size && btnOptions.size.x ?  btnOptions.size.x :
null,\n\t\t\t\t\tmodalHeight = btnOptions.size && btnOptions.size.y ?
btnOptions.size.y : null;\n\t\t\t\teditor.addButton(\"button-0Module\",
{\n\t\t\t\t\ttext: \"Module\",\n\t\t\t\t\ttitle:
\"Module\",\n\t\t\t\t\ticon: \"none icon-file-add\",\n\t\t\t\t\tonclick:
function () {\n\t\t\t\t\t\t\tvar modalOptions = {\n\t\t\t\t\t\t\t\ttitle
: \"Module\",\n\t\t\t\t\t\t\t\turl :
'http:\/\/www.longwinter.co.uk\/administrator\/index.php?option=com_modules&view=modules&layout=modal&tmpl=component&editor=jform_intro_text&780ac24446fbf734ef8b34ddbd2b92a1=1',\n\t\t\t\t\t\t\t\tbuttons:
[{\n\t\t\t\t\t\t\t\t\ttext   : \"Close\",\n\t\t\t\t\t\t\t\t\tonclick:
\"close\"\n\t\t\t\t\t\t\t\t}]\n\t\t\t\t\t\t\t}\n\t\t\t\t\t\t\tif(modalWidth){\n\t\t\t\t\t\t\t\tmodalOptions.width
=
modalWidth;\n\t\t\t\t\t\t\t}\n\t\t\t\t\t\t\tif(modalHeight){\n\t\t\t\t\t\t\t\tmodalOptions.height
=
modalHeight;\n\t\t\t\t\t\t\t}\n\t\t\t\t\t\t\teditor.windowManager.open(modalOptions);\n\t\t\t\t\t}\n\t\t\t\t});\n\t\t\t})();"]
```
to:
```
script": [ "!(function(){ var getBtnOptions = new Function(\"return
{handler: \\'iframe\\', size: {x: 800, y: 500}}\"), btnOptions =
getBtnOptions(), modalWidth = btnOptions.size && btnOptions.size.x ?
btnOptions.size.x : null, modalHeight = btnOptions.size &&
btnOptions.size.y ? btnOptions.size.y : null;
editor.addButton(\"button-0Module\", { text: \"Module\", title:
\"Module\", icon: \"none icon-file-add\", onclick: function () { var
modalOptions = { title : \"Module\", url :
'http:\/\/www.longwinter.co.uk\/administrator\/index.php?option=com_modules&view=modules&layout=modal&tmpl=component&editor=jform_intro_text&780ac24446fbf734ef8b34ddbd2b92a1=1',
buttons: [{ text : \"Close\", onclick: \"close\" }] } if(modalWidth){
modalOptions.width = modalWidth; } if(modalHeight){ modalOptions.height
= modalHeight; } editor.windowManager.open(modalOptions); } }); })();",]
```
@@ -844,7 +844,7 @@ private function tinyButtons($name, $excluded)
$btnsNames[] = $name . ' | ';

// The array with code for each button
$tinyBtns[] = $tempConstructor;
$tinyBtns[] = preg_replace('/\s+(?=([^"]*"[^"]*")*[^"]*$)/m',' ',$tempConstructor);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a space after each comma.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@Quy
Copy link
Contributor

Quy commented Feb 18, 2018

After applying the patch, the editor is no longer in WYSIWYG mode. I have to toggle the editor. Also the editor switched to Set 1 from Set 0.

@Sophist-UK
Copy link
Contributor Author

That will teach me to make changes without fully testing.

This is the old issue about the javascript parser inserting missing semicolons when needed when there is a line break. Obviously you cannot simply replace every line break with a semi-colon, nor follow every } with a semi-colon either.

I will change this to replace only \t and spaces and not all \s characters.

@Sophist-UK
Copy link
Contributor Author

Not as nice looking as before but it remains functional which is more important.

@dgrammatiko
Copy link
Contributor

@Sophist-UK ok this crappy output is my fault but there is a much better solution here:
convert the tempConstructor to and array and add each line of the script there, then implode($tempConstructor, ''); instead of the regex you used here. Also should be more performant and sorry for this piece of code 😞

@Sophist-UK
Copy link
Contributor Author

That might be a better solution - and I know it is a standard approach for HTML. However it would be likely to suffer from the same issues as noted earlier i.e. the removal of line endings might cause issues with implied semi-colons that are inserted by the parser i.e. when you have "}\n something" the parser inserts a ";" to make it "};\n something". When you remove the "\n" to make "} something", the semi-colon is not inserted and you get JS errors.

So IMO we should stick with this solution which starts from well indented and hence readable JS.

@dgrammatiko
Copy link
Contributor

That might be a better solution

Well no way! I mean we are constructing some html and then we are processing what we already constructed few lines above, that's plain wrong! Construct the html/js correctly in the first place.

Just to give you the history behind this mess:
When I added the xtd-button to tinymce, the editor was using an inline script for initialisation. Later on @Fedik did some great work for the customisation part and the inline script was converted to static file. So when this code was merged the tabs and returns was kept intensionally to output a prettified output. What we never did was to convert this code to be uglified as now it's passed in the JSON string (which by the way is also totally wrong). Long story sort please fix this in the construction time don't do it through regex.

@dgrammatiko
Copy link
Contributor

This should be ok

							// Now we can built the script
							$tempConstructor[] = '!(function(){';
							$tempConstructor[] = 'editor.addButton("' . $name . '", {';
							$tempConstructor[] = 'text: "' . $title . '",';
							$tempConstructor[] = 'title: "' . $title . '",';
							$tempConstructor[] = 'icon: "' . $icon . '",';
							$tempConstructor[] = 'onclick: function () {';

							if ($href || $button->get('modal'))
							{
								$tempConstructor[] = 'var modalOptions = {';
								$tempConstructor[] = 'title: "' . $title . '",';
								$tempConstructor[] = 'url: "' . $href . '",';
								$tempConstructor[] = 'buttons: [{text: "Close",onclick:"close"}]};';
								$tempConstructor[] = 'modalOptions.width=parseInt(' . intval($options["width"]) . ', 10);';
								$tempConstructor[] = 'modalOptions.height=parseInt(' . intval($options["height"]) . ', 10);';
								$tempConstructor[] = 'editor.windowManager.open(modalOptions);';

								if ($onclick && ($button->get('modal') || $href))
								{
									$tempConstructor[] = $onclick;
								}
							}
							else
							{
								$tempConstructor[] = $onclick;
							}

							$tempConstructor[] = '}';
							$tempConstructor[] = '});';
							$tempConstructor[] = '})();';

							// The array with the toolbar buttons
							$btnsNames[] = $name . ' | ';

							// The array with code for each button
							$tinyBtns[] = implode($tempConstructor, '');
							break;

@Sophist-UK
Copy link
Contributor Author

Ok.

@dgrammatiko
Copy link
Contributor

Just to be safe please use $tempConstructor[] = $onclick . ';'; instead of $tempConstructor[] = $onclick; in both places

@Sophist-UK
Copy link
Contributor Author

Sorry - but I am not going to make this change.

It involves essentially a rewrite of the javascript, and I do not have the time to check that what you have written - or my own alternative - is functionally correct and to adequately test the changed code.

@Sophist-UK Sophist-UK closed this Feb 18, 2018
@Sophist-UK Sophist-UK deleted the make_tinymce_options_smaller_more_readable branch February 18, 2018 21:55
@dgrammatiko
Copy link
Contributor

By the way the code that i posted above is tested/working as I tested it so all you have to do is copy paste

@Sophist-UK
Copy link
Contributor Author

Sorry - but if it is not my code I cannot take responsibility for it. Since you have the code working and tested, perhaps you can submit a replacement PR.

@Sophist-UK
Copy link
Contributor Author

P.S. If you submit a PR I will happily undertake a formal test for it.

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

Successfully merging this pull request may close these issues.

4 participants