J3 Issue ?

Failure

User tests: Successful: Unsuccessful:

avatar DerinMavi
DerinMavi
12 Feb 2017

Pull Request for Issue # .

Summary of Changes

Issue reported here: #9812, #8591, #5820, #11851 and #8106
Some of the issues were closed because of the pull requests that came but later it was found out either they do not work or they are wrong(they modify default values).

This change fixes the problem while keeping the default values.

Testing Instructions

Please test all 3 use cases:

  1. Use the following code to display a tinymce editor:
    $editor = JEditor::getInstance('tinymce'); echo $editor->display('description', "", '250', '50', '60', '5', false);

  2. Add a new article from backend of the site, try to edit this xml code https://github.com/joomla/joomla-cms/blob/staging/administrator/components/com_content/models/forms/article.xml#L27 , add width and height attributes (see https://docs.joomla.org/Editor_form_field_type for document about editor field type). For example, change it to;
    <field name="articletext" type="editor" label="COM_CONTENT_FIELD_ARTICLETEXT_LABEL" description="COM_CONTENT_FIELD_ARTICLETEXT_DESC" filter="JComponentHelper::filterText" buttons="true" height="50" width="250" />

  3. In the joomla backend go to extensions -> plugins -> tinymce editor -> advanced settings and modify width & height

Expected result

Tinymce editor has height of 50pixels and width of 250pixels

Actual result

Tinymce editor has height of 550pixels

Documentation Changes Required

Votes

# of Users Experiencing Issue
1/1
Average Importance Score
5.00

avatar DerinMavi DerinMavi - open - 12 Feb 2017
avatar DerinMavi DerinMavi - change - 12 Feb 2017
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 12 Feb 2017
Category Front End Plugins
avatar DerinMavi DerinMavi - change - 12 Feb 2017
The description was changed
avatar DerinMavi DerinMavi - edited - 12 Feb 2017
avatar DerinMavi DerinMavi - change - 12 Feb 2017
Labels Added: ?
avatar DerinMavi
DerinMavi - comment - 12 Feb 2017

I don't understand why the Travis CI build failed, any ideas?

avatar infograf768
infograf768 - comment - 12 Feb 2017

Travis says that your formatting is wrong:
FILE: ...home/travis/build/joomla/joomla-cms/plugins/editors/tinymce/tinymce.php


FOUND 2 ERROR(S) AFFECTING 2 LINE(S)


363 | ERROR | Expected "if (...)\n...{...}\n...else\n"; found

 |       | "if(...)\n...{...}\n...else "

371 | ERROR | Expected "if (...)\n...{...}\n...else\n"; found

 |       | "if(...)\n...{...}\n...else\n"

i.e. Instead of

screen shot 2017-02-12 at 10 09 47
You should have

screen shot 2017-02-12 at 10 11 22

(Note: Did not test patch itself)

avatar DerinMavi DerinMavi - change - 12 Feb 2017
The description was changed
avatar DerinMavi DerinMavi - edited - 12 Feb 2017
avatar DerinMavi DerinMavi - edited - 12 Feb 2017
avatar DerinMavi DerinMavi - change - 12 Feb 2017
The description was changed
avatar DerinMavi DerinMavi - edited - 12 Feb 2017
avatar DerinMavi DerinMavi - change - 12 Feb 2017
The description was changed
avatar DerinMavi DerinMavi - edited - 12 Feb 2017
avatar DerinMavi
DerinMavi - comment - 12 Feb 2017

Thanks! Fixed the whitespace issues.

avatar infograf768
infograf768 - comment - 13 Feb 2017

This works, except that the test should be:

$editor = JEditor::getInstance('tinymce'); 
echo $editor->display('description', "", '250', '50', '60', '5', false);

$width comes before $height
See https://github.com/joomla/joomla-cms/blob/staging/libraries/cms/editor/editor.php#L289

avatar infograf768
infograf768 - comment - 13 Feb 2017

I have tested this item successfully on 984c0c7

EDIT: breaks $html_height in params. Changed test to unsuccessful.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/14033.
avatar infograf768 infograf768 - test_item - 13 Feb 2017 - Tested successfully
avatar infograf768
infograf768 - comment - 13 Feb 2017

@Fedik @dgt41
Please test. I guess this can go in.

avatar DerinMavi DerinMavi - edited - 13 Feb 2017
avatar AlexRed
AlexRed - comment - 13 Feb 2017

but the advanced params still not working
http://alexred.com/joomla/2017/tinyMCE-advanced.png


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/14033.

avatar DerinMavi
DerinMavi - comment - 13 Feb 2017

@AlexRed If those settings did not work before, I guess it is a separate issue. This fix is only for 3rd party components that programmatically display the editor.

avatar AlexRed
AlexRed - comment - 13 Feb 2017

Before your patch the "HTML Height" parameter work, after not work :(

avatar infograf768 infograf768 - alter_testresult - 13 Feb 2017 - infograf768: Tested unsuccessfully
avatar infograf768
infograf768 - comment - 13 Feb 2017

I confirm @AlexRed findings and changed my test to negative.

avatar infograf768
infograf768 - comment - 13 Feb 2017

The issue is that $height has always a value.
To test, just add a var_dump before checking height

		var_dump($height);
		die;
		if (isset($height))
		{
			$html_height = $height;
		}
		else
		{
			$html_height = $this->params->get('html_height', '550');
		}

$height will default to 500px

avatar infograf768
infograf768 - comment - 13 Feb 2017

so, tho get this to work (for height only), we would have to use

                if ($height != 500)
		{
			$html_height = $height;
		}
		else
		{
			$html_height = $this->params->get('html_height', '550');
		}
avatar joomdonation
joomdonation - comment - 13 Feb 2017

isset is the wrong command and could not be used here. The code should be changed to:

if ($height)
{
	$html_height = $height;
}
else
{
	$html_height = $this->params->get('html_height', '550');
}

if ($width)
{
	$html_width  = $width;
}
else
{
	$html_width  = $this->params->get('html_width', '');
}

With this change, custom width (and height) if defined will be used instead of the width and height defined in plugins parameter

The easiest way to test is try to add a new article from backend of the site, try to edit this xml code https://github.com/joomla/joomla-cms/blob/staging/administrator/components/com_content/models/forms/article.xml#L27 , add width and height attributes (see https://docs.joomla.org/Editor_form_field_type for document about editor field type). For example, change it to;

<field name="articletext" type="editor"
			label="COM_CONTENT_FIELD_ARTICLETEXT_LABEL" description="COM_CONTENT_FIELD_ARTICLETEXT_DESC"
			filter="JComponentHelper::filterText" buttons="true"  height="300" width="300" />
  1. Before the change, the custom with and height is not used. The width and height parameters defined in editor plugin is used

  2. After changed, the custom height and width passed is used (note, size of editor changed)

  3. Remove width, height attribute from XML code above, the editor again uses the size defined in the plugin parameter

Please note that in case the form has multiple editors, the width and height defined in the first displayed editors will be used. If you define difference size for different editor, it won't have any affect, the size defined in the first displayed editor always win.

avatar infograf768
infograf768 - comment - 13 Feb 2017

@joomdonation
As I wrote above, we always have a default height of 500px, therefore it kills any height defined in the plugin params.

avatar joomdonation
joomdonation - comment - 13 Feb 2017

@infograf768 How did you test it? Please try to change the XML file of add article form as I suggested, you will see that it works

avatar infograf768
infograf768 - comment - 13 Feb 2017

Do not change anything. Do not patch or if you patch, do the var_dump as I have shown above.

var_dump($height);
		die;

We do have a default 500px coming from I don't know where (maybe from tiny js files)

avatar joomdonation
joomdonation - comment - 13 Feb 2017

Ah, see your point. See https://github.com/joomla/joomla-cms/blob/staging/libraries/cms/form/field/editor.php#L204-L205

If we don't define width and height in the form field parameters, default values are passed there instead of using editor parameters. No sure what's the reasons

avatar infograf768
infograf768 - comment - 13 Feb 2017

If we don't define width and height, default values are passed there instead of using editor parameters. No sure what's the reasons

Bingo. it was there

avatar joomdonation
joomdonation - comment - 13 Feb 2017

In this case, I think the best fix we could do is changing the code to:

if ($height && $height != '500px')
{
	$html_height = $height;
}
else
{
	$html_height = $this->params->get('html_height', '550');
}

if ($width && $width != '100%')
{
	$html_width  = $width;
}
else
{
	$html_width  = $this->params->get('html_width', '');
}
avatar infograf768
infograf768 - comment - 14 Feb 2017

Not sure we need
if ($height && $height != '500px')

as $height always exists.

Also, no need for 500px. My test show that 500 is fine`

avatar joomdonation
joomdonation - comment - 14 Feb 2017

Not sure we need
if ($height && $height != '500px')

=> Yes, we need it to give more flexible to developer. For example, when you call

echo $editor->display('description', "", '', '', '60', '5', false);

The editor should be rendered using width and height from plugins parameter

Also, no need for 500px. My test show that 500 is fine`

When no height is being passed, $height value is set to 500px (it is a string). I guess your code works because php doing magic comparison. For example the code below display true when the two value actually not equals

var_dump(500 == '500px');

avatar infograf768
infograf768 - comment - 14 Feb 2017

The editor should be rendered using width and height from plugins parameter

Are you sure it does not pick the default from

			$this->height      = $this->element['height'] ? (string) $this->element['height'] : '500';
			$this->width       = $this->element['width'] ? (string) $this->element['width'] : '100%';
avatar joomdonation
joomdonation - comment - 14 Feb 2017

It picks the default value if we are using editor form field type. It doesn't have default value when you call the $editor->display directly (in the code you gave for testing)

avatar joomdonation
joomdonation - comment - 15 Feb 2017

@DerinMavi Could you please look at my suggested changes and implement it so that we can re-test this PR?

avatar DerinMavi
DerinMavi - comment - 16 Feb 2017

I will try to fix it today.

avatar DerinMavi DerinMavi - change - 19 Feb 2017
The description was changed
avatar joomla-cms-bot joomla-cms-bot - change - 19 Feb 2017
Category Front End Plugins Libraries Front End Plugins
avatar DerinMavi
DerinMavi - comment - 19 Feb 2017

@joomdonation : The problem with your suggestion is that if you set height: '500px' or width:'100%' in the article.xml , your settings will be ignored and they will be overridden by tinymce advanced settings parameters.

The root cause of the problem is that for tinymce there are 2 places where default width/height is provided. Once onDisplay method inside tinymce.php is called it is impossible to know whether the widht/height are default values or user set values.

For this reason, I removed the default values from setup() method in editor.php.

For all editors, default values are set inside onDisplay() methods.

Please @infograf768 and @AlexRed test the new fix.

avatar joomdonation
joomdonation - comment - 19 Feb 2017

@DerinMavi The reason I suggested it because I was worry that if we remove default values in setup() method (like how you did in recent commit), there might be backward compatible issue with third party editor plugins.

Right now, if no width and height provided in the XML form field, third party editor plugins might expect that default width and height provided by setup method and they might not deal with empty value (like you modified for the editor plugin shipped by Joomla). That's why I mentioned in my comment that best fix we could do

avatar DerinMavi DerinMavi - edited - 19 Feb 2017
avatar DerinMavi DerinMavi - change - 19 Feb 2017
The description was changed
avatar DerinMavi DerinMavi - edited - 19 Feb 2017
avatar DerinMavi DerinMavi - edited - 19 Feb 2017
avatar DerinMavi DerinMavi - change - 19 Feb 2017
The description was changed
avatar DerinMavi DerinMavi - edited - 19 Feb 2017
avatar joomla-cms-bot joomla-cms-bot - change - 19 Feb 2017
Category Front End Plugins Libraries Front End Plugins
avatar DerinMavi
DerinMavi - comment - 19 Feb 2017

@joomdonation You are right, I checked couple of 3rd party editors and saw that they'd break if the defaults are not set. I finally settled with your solution. I changed != with !==(so that if someone passes 500 (integer) from the code it'd work as expected). Also changed the default value of 550px to 500px so that it is consistent with previous default value.

avatar infograf768
infograf768 - comment - 19 Feb 2017

We had a good reason for if ($html_width == 750). Shall not we re-add it?

avatar DerinMavi
DerinMavi - comment - 6 Mar 2017

@Fedik @joomdonation So does it cause any problems if that check is not there? You found why it was added but that was before default settings were working. Now it should work as it supposed to be. If you leave the setting as it is(=750) during installation, that value will be used instead of default tinymce value.

Before this change, despise the set value of 750 it'd still use tinymce default setting, which is 550. That would also mean it would not work if the user really wanted to use width as 750 pixels.

avatar DerinMavi DerinMavi - change - 6 Mar 2017
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2017-03-06 22:10:49
Closed_By DerinMavi
avatar DerinMavi DerinMavi - close - 6 Mar 2017
avatar DerinMavi DerinMavi - change - 6 Mar 2017
Status Closed New
Closed_Date 2017-03-06 22:10:49
Closed_By DerinMavi
avatar DerinMavi DerinMavi - change - 6 Mar 2017
Status New Pending
avatar DerinMavi DerinMavi - reopen - 6 Mar 2017
avatar N6REJ
N6REJ - comment - 23 Mar 2017

TinyMCE sizing does not work with form fields either, and I don't mean the new ones..
name="brandText"
class=""
type="editor"
default=""
cols="2"
rows="3"
width="300px"
height="300px"
Will produce a fully screen layout

avatar schnuti
schnuti - comment - 30 Mar 2017

@N6REJ
width="300px"
height="300px"
Numeric! remove px and it should be ok
px is added later

Edit: Sorry, I now saw in the code that it adds px if numeric otherways leaves it as is.

I know the height works in a form as I use it.

avatar studio42
studio42 - comment - 17 Mar 2018

I'm on programatically change the editor height, this is not fixed in Joomla 3.8.6 !
Now more then1 year for this ?
Come on guys, it's not a big deal to check for values and use right ones.

avatar studio42
studio42 - comment - 17 Mar 2018

In /plugins/editors/tinymce/tinymce.php line 408

		if($height) $html_height = $height;
		else $html_height = $this->params->get('html_height', '550');

Set the height correctly.

avatar BilalReffas
BilalReffas - comment - 23 Jul 2018

Same for me testing instruction 1 and 2 works fine on my local machine, but the third instruction seems to fail. I found another layout issue please check the attached screenshot from the dropdown-button behaviour @icampus
bildschirmfoto 2018-07-23 um 13 23 23

avatar Schmidie64 Schmidie64 - test_item - 24 Jul 2018 - Tested unsuccessfully
avatar Schmidie64
Schmidie64 - comment - 24 Jul 2018

I have tested this item 🔴 unsuccessfully on d981c6b

I followed the instructions step by step but the patch didn't work well. Before i applied the patch i could change the editor size in the web config well but not in the xml file. After i applied the patch, i couldn't change anything, not in the web config neither nor in the xml File. So i thing the patch is not completely correct.

@icampus


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/14033.

avatar franz-wohlkoenig franz-wohlkoenig - change - 19 Apr 2019
Title
Fixed: Tiny MCE plugin overrides custom settings
Tiny MCE plugin overrides custom settings
avatar franz-wohlkoenig franz-wohlkoenig - edited - 19 Apr 2019
avatar Quy Quy - close - 21 Oct 2019
avatar Quy Quy - change - 21 Oct 2019
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2019-10-21 19:32:20
Closed_By Quy
avatar Quy
Quy - comment - 21 Oct 2019

Replaced by PR #26755


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/14033.

Add a Comment

Login with GitHub to post a comment