Allow multiple (different) editors in one page #12561

dgrammatiko · 2016-10-25T22:21:56Z

Pull Request for Issue #12555

XTD-Buttons will NOT work if more than one (and different) editors are rendered on the page

Steps to reproduce

Make sure that the default editor is tinyMCE

Edit administrator/components/com_content/models/forms/article.xml and add after

    <fieldset addfieldpath="/administrator/components/com_categories/models/fields" >

this:

        <field
                name="test2"
                label ="Test Field"
                type="editor"
                width="300"
                filter="safehtml"
                editor="none"
                buttons="true"/>
        <field
                name="test3"
                label ="Test Field"
                type="editor"
                width="300"
                filter="safehtml"
                editor="codemirror"
                buttons="true"/>

Edit administrator/components/com_content/views/article/tmpl/edit.php and add after

                <fieldset class="adminform">
                    <?php echo $this->form->getInput('articletext'); ?>
                </fieldset>

this:

                <fieldset class="adminform">
                    <?php echo $this->form->getInput('test2'); ?>
                </fieldset>
                <fieldset class="adminform">
                    <?php echo $this->form->getInput('test3'); ?>
                </fieldset>

Then try to use the buttons to insert some images, contacts, etc. Oops it's broken for all except the last one.

Summary of Changes

Introducing a simple API which is just some setters and some getters.
This is more informative:

// An object to hold each editor instance on page, only define if not defined.
Joomla.editors.instances = Joomla.editors.instances || {
        /**
         * Editors MUST register, per instance, the following:
         *
         * getValue         Type  Function
         *          Should return the complete data from the editor
         *          Example: function () { return this.element.value; }
         * setValue         Type  Function
         *          Should replace the complete data of the editor
         *          Example: function (text) { return this.element.value = text; }
         * replaceSelection  Type  Function
         *          Should replace the selected text of the editor
         *          If nothing selected, will insert the data at the cursor
         *          Example: function (text) { return insertAtCursor(this.element, text); }
         *
         * USAGE (jform_articletext is the editor id)
         * getValue:
         *      Joomla.editors.instances['jform_articletext'].getValue();
         * setValue:
         *      Joomla.editors.instances['jform_articletext'].setValue('Joomla! rocks');
         * replaceSelection:
         *      Joomla.editors.instances['jform_articletext'].replaceSelection('Joomla! rocks')
         *
         * *********************************************************
         * ANY INTERACTION WITH THE EDITORS SHOULD USE THE ABOVE API
         * *********************************************************
         */
    };

Testing Instructions

Apply this patch
Go to back end and edit/create an article
On purpose there are all the editors in that page
Try the buttons in all of them. Cool eh?

Documentation Changes Required

The comment above comes from core.js but needs to be published also in the form->editor page.

TL;DR

Some images then:

Calling @Fedik @andrepereiradasilva @ggppdk @mbabker

This change is

dgrammatiko · 2016-10-25T22:57:18Z

@okonomiyaki3000 can you check the changes I did in codemirror here?

okonomiyaki3000 · 2016-10-26T01:05:09Z

I'm not sure what this media/system/js/editor-codemirror.js is about but I don't like it.

dgrammatiko · 2016-10-26T01:13:50Z

it's just the initiator script, this way there is no inline script at all

okonomiyaki3000 · 2016-10-26T01:26:32Z

I don't think there's anything wrong with inline script. The problem with this is that it seems to be part of Joomla core instead of the CodeMirror plugin and it can't be overridden like the current version can (via layouts).

dgrammatiko · 2016-10-26T01:30:08Z

@okonomiyaki3000 not quite true, you can still use the layouts although a tiny script will be missing (the one that comes from code mirror.php). But if it breaks B/C I can revert it and redo this in J4

okonomiyaki3000 · 2016-10-31T07:14:36Z

Sorry, I can't get behind putting files used by the editors in media/system/js/. They just don't belong there. The editors should be thought of as optional plugins, decoupled from the Joomla core.

dgrammatiko · 2016-10-31T07:18:03Z

@okonomiyaki3000 I will revert that part, but this PR is NOT about the initialisation of the editors, it solves a very fundamentally wrong architectural concept that limits joomla to only one editor per page...

okonomiyaki3000 · 2016-10-31T07:38:16Z

@dgt41 Yes, I'm aware of the limitation. It limits Joomla to only one type of editor per page. You may still have multiple instances of the same type. It's quite a glaring flaw and I'm glad you're fixing it but we must first do no harm.

dgrammatiko · 2016-11-01T21:18:49Z

@ggppdk can you test this one?

ggppdk · 2016-11-01T21:21:28Z

@dgt41

will do tomorrow, thanks for the work on this one

dgrammatiko · 2016-11-01T23:29:12Z

@okonomiyaki3000 I've restored the inline initialisation script for codemirror...

okonomiyaki3000 · 2016-11-02T00:17:38Z

I don't know, I'm still seeing media/system/js/editor-codemirror.js. What about other editors?

dgrammatiko · 2016-11-02T00:20:31Z

What about other editors?

Other editors also using static scripts in media/editors/whatever/js. Also code mirror should move to static script in J4

okonomiyaki3000 · 2016-11-02T01:32:37Z

Isn't media/editors/whatever/js just the editor's distro code? Is Joomla-specific code meant to go in there too?

dgrammatiko · 2016-11-02T01:37:53Z

Right now the folders ``media/editor/whateverare maintained/manipulated by joomla, in J4 the aim is that foreign code should live inmedia/vendor/` folder and there should be an automated way to manage the updates (code is already written for that).
So here is a glimpse from the future (j4):

okonomiyaki3000 · 2016-11-02T01:42:17Z

I see, that makes sense I guess. Does that mean that the libraries under vendor must be used as distributed? What if we prefer custom builds?

dgrammatiko · 2016-11-02T01:47:17Z

@okonomiyaki3000 I guess this will manage only the joomla distributed code, I have no clue if this will be extended somehow for 3rd PD (my guess is no, 3rd PD could provide their custom code in their own directory). Or you can manipulate or override the grunt script and get whatever you want, with the added step to re-run the script every time joomla updates these assets...

infograf768 · 2016-11-02T17:25:06Z

Conflicts

yvesh · 2016-11-02T17:40:02Z

administrator/components/com_content/models/forms/article.xml

@@ -28,6 +28,24 @@
 			label="COM_CONTENT_FIELD_ARTICLETEXT_LABEL" description="COM_CONTENT_FIELD_ARTICLETEXT_DESC"
 			filter="JComponentHelper::filterText" buttons="true" />

+<field


@dgt41 Can you look into the formatting here? The indentation seems to be broken, same for closing tag. And you have some white space at the label.

@yvesh this whole section is here only for testing, will be and should be removed before merging, thus the wrong indentation

yvesh · 2016-11-02T17:42:34Z

administrator/components/com_content/views/articles/tmpl/modal.php

@@ -111,8 +123,9 @@
 							<span class="<?php echo $iconStates[$this->escape($item->state)]; ?>"></span>
 						</td>
 						<td>
-							<a href="javascript:void(0);" onclick="if (window.parent) window.parent.<?php echo $this->escape($function); ?>('<?php echo $item->id; ?>', '<?php echo $this->escape(addslashes($item->title)); ?>', '<?php echo $this->escape($item->catid); ?>', null, '<?php echo $this->escape(ContentHelperRoute::getArticleRoute($item->id, $item->catid, $item->language)); ?>', '<?php echo $this->escape($lang); ?>', null);">
-								<?php echo $this->escape($item->title); ?></a>
+							<a class="select-link" href="javascript:void(0)" data-function="<?php echo $this->escape($onclick); ?>" data-id="<?php echo $item->id; ?>" data-title="<?php echo $this->escape(addslashes($item->title)); ?>" data-cat-id="<?php echo $this->escape($item->catid); ?>" data-uri="<?php echo $this->escape(ContentHelperRoute::getArticleRoute($item->id, $item->catid, $item->language)); ?>" data-language="<?php echo $this->escape($lang); ?>">


Any way to make this more readable and less long? :)

Not use as many attributes or start formatting our HTML the same way our XML files are formatted (and I find THAT ugly and painful to keep up with personally).

@mbabker would you prefer something like:

<?php $attribs = 'data-function="' . $this->escape($onclick) . '"' . ' data-id="' . $item->id . '"' . ' data-title="' . $this->escape(addslashes($item->title)) . '"' . ' data-cat-id="' . $this->escape($item->catid) . '"' . ' data-uri="' . $this->escape(ContentHelperRoute::getArticleRoute($item->id, $item->catid, $item->language)) . '"' . ' data-language="' . $this->escape($lang) . '"'; ?> <a class="select-link" href="javascript:void(0)" <?php echo $attribs; ?>>

Actually, for this you could just as usually use JHtml::_('link', 'javascript:void(0)', $this->escape($item->title), $attribsArray); and it not be unbearable. What you have works too, I'm just honestly not the biggest fan of some of our code style rules with line concatenation and splitting elements on separate lines in funny places.

Until recently, I seriously thought our iframe and link HTML helpers were silly as all can be. But then I started thinking that they're actually pretty useful for elements where you're adding a lot of data in some form (in this case data attributes) so you can throw those elements in an array and pass them into a function which converts it to the proper string. End result is still the same; clean PHP code and clean HTML output.

/** * Write a `<a>` element * * @param string $url The relative URL to use for the href attribute * @param string $text The target attribute to use * @param array|string $attribs Attributes to be added to the `<a>` element * * @return string * * @since 1.5 */ public static function link($url, $text, $attribs = null)

I've learned something today 😄

yvesh · 2016-11-02T17:44:59Z

media/com_contact/js/admin-contacts-modal.js

+		editor = Joomla.getOptions('xtd-contacts').editor;
+
+		if (lang !== '')
+		{


You are mixing php and js code style here ;)

copy-paste...

yvesh · 2016-11-02T17:45:33Z

media/com_contact/js/admin-contacts-modal.js

+	 * Javascript to insert the link
+	 * View element calls jSelectContact when a contact is clicked
+	 * jSelectContact creates the link tag, sends it to the editor,
+	 * and closes the select frame.


Actually it would be nice to have since tags in js too.. Any thoughts on that?

There might be a way, have to do some testing

@yvesh so what is the @since supposed to be for all these scripts, because ok now they are static files but the code comes from way back?

yvesh · 2016-11-02T17:46:57Z

media/com_contact/js/admin-contacts-modal.js

+			hreflang = ' hreflang = "' + lang + '"';
+		}
+
+		tag = '<a' + hreflang + ' href="' + link + '">' + title + '</a>';


Personally i would prefer when you do a document.createElement('a') here etc. then hardcoding a link into a string.

you're right

Too fast, we do support IE8, so not possible

Hm document.createElement should work in IE8?

yvesh · 2016-11-02T17:48:38Z

media/com_contact/js/admin-contacts-modal.js

+		// Get the elements
+		var elements = document.querySelectorAll('.select-link');
+
+		for(var i = 0, l = elements.length; l>i; i++) {


Just out of curiosity, why are you saving elements length in a variable? ;) cs btw.

need for speed 😄

itar in phpstorm create those...

yvesh · 2016-11-02T17:49:42Z

media/com_content/js/admin-article-pagebreak.js

+		title  = (title != '') ? 'title="' + title + '"' : '';
+		alt    = (alt != '') ? 'alt="' + alt + '"' : '';
+
+		tag = '<hr class="system-pagebreak" ' + title + ' ' + alt + '/>';


Same as above, wouldn't document.createElement be nicer?

Hm it should work fine in IE8? Shouldn't it? We use document.createElement in a lot of other JS files

Actually it shouldn't be any problem, just checked...

@yvesh checking this again, I am not in favour as we are passing a string to the editor and I have no clue if all editors out there will be happy with an element instead of a string

yvesh · 2016-11-02T17:50:22Z

media/com_content/js/admin-article-readmore.js

+	content = (new Function('return ' + options.editor))();
+
+	if (content.match(/<hr\s+id=("|')system-readmore("|')\s*\/*>/i)) {
+		alert(options.exists);


An alert? :-)

it's what was there, what else can it be?

I don't know why it is there any ways, but not a fan of alerts at all ;) Maybe use Joomla.renderMessages instead?

I guess is the same story as the buttons of the toolbar, where also an alert is utilised...
Honestly I don't mind changing it to Joomla.renderMessages()

@yvesh This was a great idea generator, so J4 will come with some shiny new thingy that will cover those cases

anibalsanchez · 2017-01-14T15:37:58Z

I have tested this item ✅ successfully on 3550213

Test OK