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

Problems with tinyMCE width and height, joomla 3.7 #15059

Closed
wants to merge 7 commits into from

Conversation

schnuti
Copy link
Contributor

@schnuti schnuti commented Apr 2, 2017

I think there are some confusions about the computed values of the width and height values for the tinyMCE editor. Personally I categorize this as a bug.

In theory there could be a lot of default settings but this is more simple.

We get the values from user editable system wide parameters (with some funny results like height=750, the tinyMCE default )):

  • tinyMCE editor default values
  • tinyMCE editor settings
  • this overrides the parameters from the form in all components and views.

Article edit
article_is

A second field in article edit as in PR 14520.
article_is2

Article frontend editing.
article frontend

The Joomla editor form field will never get any settings from external sources as default.

My propasal is to set the width hardcoded to 100% (or auto) and the height from:

  • Joomla editor form field default settings
  • the field's settings in the form field XML
  • tinyMCE default setting in the code (if no value available). I'm not quite sure about this.
  • override in a system plug in - onBeforeRender() method

Expected result
article_should

As tinyMCE automatically gets the height values from the hidden textarea field, we don't have to explicit set any values in the tinyMCE options. As well, we don't have to set any editor instance values as in #14520.

If you want a width per field, this has to be added to each editor instance as in PR #14520. As far as I could see there is no automatic loading as with the height.

In this PR I have removed the parameters in the tinyMCE editor, removed the tinyMCE options, added defaults in tinyMCE code and deprecated the language strings. I did not change the legacy part. Width is now overridable with a custom template/layout and height by using a custom system plugin - method onBeforerender().

Test instructions:

  1. Add an extra editor field as in Fix TinyMCE ext-buttons. Respect ext-buttons configuration per an editor instance #14520 with height. The original article text field has no expicit value, default 500px height from the form field.

In the article form XML administrator/components/com_content/models/forms/article.xml add one more editor element (somewhere after line
<fieldset name="basic" label="COM_CONTENT_ATTRIBS_FIELDSET_LABEL">)

<field name="text2" type="editor" hide="menu,module,contact" label="Text2" description="" filter="JComponentHelper::filterText" buttons="true" height="250"/>

  1. Check the result without adding the patch. Play with the tinyMCE editor settings a bit. Go to the tab(s) advanced in tinyMCE and modify Width and Height to what a user could enter. e.g. width 50%, height=200.
    Have a look to the new second field in article edit, backend categories and frontend article edit/creation as well.

  2. Apply this patch.
    There are no settings so just reload the article edit page and .
    Check and compare the results.

@Fedik Is it easy to add the width if !100% as in PR 14520? The width could then be set in the form field. Needed?

Related
#14917
#14835

@joomla-cms-bot joomla-cms-bot added Language Change This is for Translators PR-staging labels Apr 2, 2017
@@ -52,10 +52,6 @@ PLG_TINY_FIELD_FUNCTIONALITY_DESC="Select level of functionality."
PLG_TINY_FIELD_FUNCTIONALITY_LABEL="Functionality"
PLG_TINY_FIELD_HR_DESC="Show or hide the Horizontal Rule button."
PLG_TINY_FIELD_HR_LABEL="Horizontal Rule"
PLG_TINY_FIELD_HTMLHEIGHT_DESC="Height of HTML editor. Only applies in Advanced and Extended mode."
Copy link
Contributor

Choose a reason for hiding this comment

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

Please don't remove the unused language strings. You can mark them as unused but the standard says we don't delete thrm

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK! I was not sure. How do I mark them?

Copy link
Contributor

Choose a reason for hiding this comment

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

Something like

; The following xxx strings are deprecated and will be removed with 4.0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@brianteeman
Thanks. Now I only have to find out how to revert the change and add the new line. I'm only using gitHub occasionally. Any hint?

@Fedik
Copy link
Member

Fedik commented Apr 2, 2017

Is it easy to add the width if !100% as in PR 14520?

sorry, not very understood your question,
14520 already merged

@schnuti
Copy link
Contributor Author

schnuti commented Apr 2, 2017

@Fedik
I wrote - as in. That is handle the width in the same way.
Edit: and add the width to the editor instance if the value is not 100% - this is default.

@AlexRed
Copy link
Contributor

AlexRed commented Apr 2, 2017

I have tested this item ✅ successfully on 122caac

Patch ok for me.
Thanks 😃


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

@schnuti
Copy link
Contributor Author

schnuti commented Apr 2, 2017

@AlexRed
Thanks for testing.
Do you as I, think, it should go into 3.7.0? I guess it's harder to remove the parameters later on. It's late!

@AlexRed
Copy link
Contributor

AlexRed commented Apr 2, 2017

yes, also I hope we can solve the tinyMCE width problem in 3.7.0 or 3.7.1

@schnuti
Copy link
Contributor Author

schnuti commented Apr 4, 2017

@brianteeman I've reverted the language change and added a remark

@ghost ghost mentioned this pull request Apr 4, 2017
@schnuti
Copy link
Contributor Author

schnuti commented Apr 5, 2017

I can't think of any situation where a width <> 100% (see images above) is needed. If you know one, I've now tested a solution using Fediks editor instances that works fine.

@ghost
Copy link

ghost commented Apr 30, 2017

I have tested this item 🔴 unsuccessfully on 8a2f338

Error on "Articles: Edit" in Backend:
bildschirmfoto 2017-04-30 um 08 48 37

System information

3.7.1-dev
macOS Sierra, 10.12.4
Firefox 53 (64-bit)

MAMP 4.1.1

  • PHP 7.0.15
  • MySQLi 5.6.35

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

@schnuti
Copy link
Contributor Author

schnuti commented Apr 30, 2017

@franz-wohlkoenig
Could you please check if you have the JS file in the directory.
media/editors/tinymce/plugins/jdragdrop/plugin-min.js
It has obviously bin moved to media/editors/tinymce/js/plugins/dragdrop/plugin-min.js

As far as I can see this PR has nothing to do with this javascript loading error.
Related? #15057

@ghost
Copy link

ghost commented Apr 30, 2017

media/editors/tinymce/plugins/jdragdrop/plugin-min.js

jdragdrop/plugin-min.js doesn't exist.

media/editors/tinymce/js/plugins/dragdrop/plugin-min.js

Folder exist.

@schnuti
Copy link
Contributor Author

schnuti commented Apr 30, 2017

@franz-wohlkoenig
I think you somehow got out of sync.
see e.g.
https://github.com/joomla/joomla-cms/tree/staging/media/editors/tinymce/js/plugins/dragdrop

@ghost
Copy link

ghost commented Apr 30, 2017

@schnuti
Copy link
Contributor Author

schnuti commented Apr 30, 2017

@franz-wohlkoenig
Do you really find it on your instance? Do you override the tinyMCE settings? (personally I love it!)
As I said "This PR do not change anything regarding Javascript""

@ghost
Copy link

ghost commented Apr 30, 2017

Do you really find it on your instance?

bildschirmfoto 2017-04-30 um 11 28 34

Do you override the tinyMCE settings?

Clean Install of latest nightly Build. After applied PR got Error described in Comment

@schnuti
Copy link
Contributor Author

schnuti commented Apr 30, 2017

@franz-wohlkoenig
That's as far I go, I have my solution - override-. I do NOT need this patch.
By!

@schnuti
Copy link
Contributor Author

schnuti commented Apr 30, 2017

@franz-wohlkoenig
I have to apologize! There are no conflicts above - but there should be,
My PR overrides somehow #15057 that moves the js to an "external" path.
I get no differences with staging.
How can I fix that? New PR?

@ghost
Copy link

ghost commented May 1, 2017

@schnuti sorry, don't know how to fix, maybe @Bakual can help?

@schnuti
Copy link
Contributor Author

schnuti commented May 1, 2017

As I understand this should not be possible.
As I said - when I compare with JoomlaCMS/staging I get " Able to merge." and an update is not possible!
My guess for now is that the PatchTester is the problem copying the old PHP file to the test system and that a commit to Joomla staging would work.
But I would need some advise.

@Bakual
Copy link
Contributor

Bakual commented May 1, 2017

Yep, Patchtester can cause some funny results if the PR branch doesn't have conflicts but is outdated neverthless. Because it copies the files instead of merging the changes.
You can merge (or rebase) the staging branch into your PR branch to fix that issue.

Update from Joomla/cms staging
@schnuti
Copy link
Contributor Author

schnuti commented May 2, 2017

@Bakual
Thanks for the hint. I found a way to merge the staging changes into the PR branch using the GitHub web interface.
@franz-wohlkoenig
Could you give it another try. I've tested and it works for me using the PatchTester.
Thanks for testing and finding the problem in the first place!

@brianteeman brianteeman modified the milestone: Joomla 4.0 Jun 8, 2017
@schnuti schnuti closed this Jun 23, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Language Change This is for Translators
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants