joomla / joomla-cms

Home of the Joomla! Content Management System
https://www.joomla.org
GNU General Public License v2.0
4.76k stars 3.64k forks source link

com_fields editor field, editor plugin wierdness #14935

Closed PhilETaylor closed 7 years ago

PhilETaylor commented 7 years ago

Steps to reproduce the issue

Install from joomla-cms/staging Due to bugs in the HEAD Im using commit 1f89fb09d941d3c7d14b366ffe84d880f9347cf7 for this test

Add an editor custom field in com_fields

Edit a content item, click fields tab, see the new editor, then attempt to use an editor plugin from the bottom row

screen shot 2017-03-27 at 16 24 40

Expected result

The features work as expected, and insert their results into the Editor Custom Field area

Actual result

The features popup into a modal, and on selecting their item, the modal stays shown and the results are inserted into the editor on the Content Tab and NOT into the new editor on the field tab.

System information (as much as possible)

https://gist.github.com/PhilETaylor/9d3c6591ed3d7d5273626f8c5d7ae193

Additional comments

Tested on FRONT and BACK end

AlexRed commented 7 years ago

I can confirm the problem


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

Bakual commented 7 years ago

It's actually not related to com_fields, I can reproduce it in my extension (SermonSpeaker) where I use two editors in the same view. So it's an issue with the editor (plugins) itself somewhere. It works fine in 3.6.5.

@dgt41 I think you did some changes to how editors and their plugins work? Do you have a clue where that comes from?

PhilETaylor commented 7 years ago

It's actually not related to com_fields,

com_fields is being shouted about as the next major feature of Joomla.

The user experience of using com_fields is pathetic at best.

Therefore when Joomla starts shouting about its brand new com_fields type feature - the user experience will be crap

Then com_fields will get a bad name

ergo, this is a com_fields issue...

The fact that you have had the issue for (years?) in your extension but no one noticed, and yet within 5 mins of using com_fields I found this bug goes to show that this bug will be uncovered quickly by those attempting to use the new feature of Joomla...

I agree that the CODE that powers this feature is probably not in com_fields, however this IS used by the new (and some have said, exciting) new feature of Joomla - thus if you want to shout about great new features, at least make sure that all dependancies of those new features look and work well.

The new com_fields exposes much more functionality to the end user and super admins than before - and is uncovering a LOT of ancient bugs that need fixing before you can shout about how great a feature custom fields is.

Edit: You = Anonymous royal "you"

Bakual commented 7 years ago

My statement was meant as a help as to where to look for the issue. Not that someone starts looking in com_fields to solve it because it is unrelated to it. Of course, com_fields made it surface and can be used to test it easily (or you can install my SermonSpeaker component and look at the "Speaker" edit view).

The fact that you have had the issue for (years?) in your extension

As I wrote it works fine in 3.6.5 but is broken in staging. It's likely related to some changes in how the editors and their plugins work.

dgrammatiko commented 7 years ago

@PhilETaylor @Bakual I'm afraid there is no way to go around this in 3.x due to B/C. Let me explain: All XTD Buttons in 3.x are using the same function (declared by each editor, that's the problem) to interact with the editor namely jInsertEditorText(tag, editor); This means, due to the way javascript overrides functions, the last registered editor will be the one that will have the above function coupled with it's own code. This problem exist for many years but it's very obvious with com_fields. The solution is already merged: https://github.com/joomla/joomla-cms/pull/12561

BUT AGAIN: due to B/C promise the new functionality cannot be used as a replacement to the old one (please read the code before making assumptions here).

PS: The core buttons should work with different editors, tho, if not, there's a bug

PhilETaylor commented 7 years ago

So this is a "wont fix" in 3.x even though a solution has been merged?

Just another "feature" of com_fields that brings down the whole quality of the new "flagship feature" :-(

dgrammatiko commented 7 years ago

PS: The core buttons should work with different editors, tho, if not, there's a bug

@PhilETaylor core buttons should work, if not, there's a bug in my code!

Check these lines: https://github.com/joomla/joomla-cms/blob/staging/media/com_content/js/admin-article-pagebreak.js#L27-L32

PhilETaylor commented 7 years ago

The buttons I tested are shown in the image in the opening post of this issue.

Those are the features that are currently entering their results into the wrong editor.

Those are core Joomla features, installed from scratch with joomla-cms/staging@1f89fb09d941d3c7d14b366ffe84d880f9347cf7 code.

I have no custom "anything" installed.

So are you now saying that this is a bug that could and should be fixed in 3.x?

dgrammatiko commented 7 years ago

@PhilETaylor make sure that you are testing with https://github.com/joomla/joomla-cms/pull/14902

dgrammatiko commented 7 years ago

So are you now saying that this is a bug that could and should be fixed in 3.x?

Maybe you got me wrong: the solution is not universal, meaning core buttons with core editors will work as they were intended, everything else won't.

I hope this clears things up

PhilETaylor commented 7 years ago

I hope this clears things up

Nope.

Are you saying that the core editors, when loaded in the custom fields, the core buttons in the opening post of this issue (Read More, Article) SHOULD work if multiple editors are on the page? - They Dont, hence this issue being opened

I have told you what I am testing with in my opening post, and again 14 mins ago, the revision used was 1f89fb09d941d3c7d14b366ffe84d880f9347cf7 which is 3 revisions after the merging of your code in commit 2dcfcd8256100632cb3cfefe759aad5ac2911b25

Bakual commented 7 years ago

@dgt41 Then there is a bug in your code because the core buttons don't work that way :smile: They insert always into the first editor on the page and the modal doesn't close. Doesn't matter if it is a custom field or one defined in the form.xml.

dgrammatiko commented 7 years ago

@PhilETaylor I'm an idiot, I should carefully read the descriptions 😓 So you are right the fields button sucks big time as it's using only the olde code https://github.com/joomla/joomla-cms/blob/staging/administrator/components/com_fields/views/fields/tmpl/modal.php#L29-L37

compare it to https://github.com/joomla/joomla-cms/blob/staging/media/com_content/js/admin-article-pagebreak.js#L27-L32

and you have your solution.

PS Please don't use inline scripts!

PhilETaylor commented 7 years ago

Video taken on e5f6dcf884ac04467c4064e33cb69ac8d8583bee (This is the very latest HEAD of staging at the time of writing)

http://screenshot.myjoomla.io/2f2g251m2i0f

dgrammatiko commented 7 years ago

@Bakual is that tinyMCE, or none, codemirror? tinyMCE got a lot of code changed...

dgrammatiko commented 7 years ago

@PhilETaylor can you try with editor none or codemirror to figure out if this is the buttons or the editor code that misbehaves?

Bakual commented 7 years ago

@dgt41 TinyMCE in current staging. Tested using the Module and Menu buttons. It works indeed with CodeMirror. So seems to be related to Tiny.

PhilETaylor commented 7 years ago

@dgt41 Those buttons dont appear (on the fields tab) when you select NONE as an editor. (Another bug!)

When you select CodeMirror you only get the buttons under the Content Editor on the Content Tab:

screen shot 2017-03-28 at 14 50 51

The custom buttons dont load under editors (CodeMirror) on the Fields tab... (another bug?)

screen shot 2017-03-28 at 14 51 07
dgrammatiko commented 7 years ago

@PhilETaylor @Bakual there was a problem with the button configuration, I proposed https://github.com/joomla/joomla-cms/pull/14417 but @Fedik come up with another idea, so the bug must be somewhere there

Bakual commented 7 years ago

So you are right the fields button sucks big time as it's using only the olde code

Umm, those buttons work fine in Codemirror as well, same as all other buttons do. And they break with the second instance of TinyMCE as all other buttons do. The code for them was based on the one for the modules button, so it can't be that it sucks that "big time".

dgrammatiko commented 7 years ago

@Bakual

fieldIns = function(id) {
    window.parent.jInsertEditorText("{field " + id + "}", "' . $editor . '");
    window.parent.jModalClose();
};
fieldgroupIns = function(id) {
    window.parent.jInsertEditorText("{fieldgroup " + id + "}", "' . $editor . '");
    window.parent.jModalClose();
};

There is no way this code to ever work with multiple instances of editors should be something like:

        /** Use the API, if editor supports it **/
        if (window.parent.Joomla && window.parent.Joomla.editors && window.parent.Joomla.editors.instances && window.parent.Joomla.editors.instances.hasOwnProperty(editor)) {
            window.parent.Joomla.editors.instances[editor].replaceSelection(tag)
        } else {
            window.parent.jInsertEditorText(tag, editor);
        }

EDIT: it will work only if all editors in the page are the same e.g. tinyMCE

Bakual commented 7 years ago

There is no way this code to ever work with multiple instances of editors

Then I wonder why it works fine with multiple instances of Codemirror when the editor is specified in the form xml.

Honestly, if that code wouldn't work, then we have a problem with 3.7.0 as there would be a B/C incompatible change. This code HAS to work as it is likely present in many plugins.

dgrammatiko commented 7 years ago

@Bakual so jInsertEditorText() is a function that every editor declares. Due to nature of javascript and to chained calls only the last one will be available. If you have a solution for this please make a PR an revert my approach of multiple instantiation for the editors...

It works for multiple instances of e.g. codemirror because the override function is in fact the same that is overriding ;)

Bakual commented 7 years ago

I have no clue about JavaScript. So I can neither fix nor revert it myself as I wouldn't know what code to change. But I know what our B/C promise says: If we apply changes that break existing functionality, they have to be reverted and delayed to 4.0.

But as I wrote, it works with two instances of CodeMirror. It just doesn't work for TinyMCE for some reason.

dgrammatiko commented 7 years ago

@Bakual then either:

You cannot have multiple instances of different editors in 3.7 as this was never actually working till my PR, which unfortunately is introducing a new method which is not B/C with that crap jInsertEditorText() the existed for a decade...

Pick wisely

AlexRed commented 7 years ago

for example the extension Jdownload use multiple editor and in Joomla 3.6.5 is ok to use the editor plugin "Module" in the second editor instances and the module code is in the second editor content. But in Joomla 3.7.0stagin all the editor plugin go to the first editor instances.
(sorry for my bad english)

PhilETaylor commented 7 years ago

take out the editor field from com_fields

Lol - another "Headline feature" being removed rather than implemented correctly is not the way forward really.

Bakual commented 7 years ago

You cannot have multiple instances of different editors in 3.7 as this was never actually working till my PR, which unfortunately is introducing a new method which is not B/C with that crap jInsertEditorText() the existed for a decade...

I think we misunderstood eachother then here. I was talking about two instances of the same editor. Not mixed ones. So two TinyMCE or two Codemirrors are both working fine in 3.6.5. Two Codemirrors still work fine in staging, but two TinyMCE are broken. Exactly as @AlexRed confirms as well.

  • take out the editor field from com_fields
  • append automatically the same editor as the one that user is using (or the default from configuration)

And again, I'm not talking about com_fields, it's broken also when two editors are specified in the form.xml (like in my extension SermonSpeaker or JDownloads). The custom editor field doesn't allow to specify the editor type anyway. It will use the default one (either user setting or global). So both options wouldn't solve the issue and don't make any sense 😄

dgrammatiko commented 7 years ago

I guess there is a bug somewhere in https://github.com/joomla/joomla-cms/pull/14520

Bakual commented 7 years ago

That one isn't even merged yet and it isn't about the button configuration. The buttons are there and work, just the JS from the modal has the wrong target and the modals don't close.

dgrammatiko commented 7 years ago

I just realised that, that PR is not merged!

@PhilETaylor please test

dgrammatiko commented 7 years ago

@Bakual nope, that's the problem, buttons misbehaving due to bad configuration (js side)

Bakual commented 7 years ago

@dgt41 You're right, that did fix it for me both for custom fields and when specified in the XML. I'm closing this issue since there is a PR.

PhilETaylor commented 7 years ago

PLEASE watch this video http://screenshot.myjoomla.io/232g3O1C0K0B

It seems many of you in this thread misunderstand the reported issue.

14520 DOESNT fix the reported issue.

zero-24 commented 7 years ago

Thanks reopening.

dgrammatiko commented 7 years ago

@PhilETaylor out of curiosity, what are the names of the fields in your test?

brianteeman commented 7 years ago

@dgt41 doubt that matters as I can replicate it

dgrammatiko commented 7 years ago

@brianteeman the reason that I'm asking this is because the solution in #14520 is based on the names of the fields...

PhilETaylor commented 7 years ago

for the record then, my custom fields editors were named

However, if you are expecting things to work based on user inputted field names, then we have lost already and we should all just pack up and go home. :-)

I have tested again and the custom field names make no difference to the issue reported.

Bakual commented 7 years ago

@PhilETaylor That's exactly the issue I was talking about as well and which got fixed for me. After clearing browser cache of course since the JS has changed. The only difference I see is that I named my field just "editor" and I only used one custom editor field.

I can try again later this evening when kids are sleeping.

PhilETaylor commented 7 years ago

Im now home on a different mac.

This is not a cached issue. This is not fixed by #14520 with caching disabled.

This happens with even a single custom editor field called "editor" - the results of the buttons in the bottom row are injected to the wrong editor.

tested just now on the very latest 4ba67b7de454befe851da3d120773d0bbd4855ea

PhilETaylor commented 7 years ago

If interested here is another multiple editors on page bug reported just now #14990

Bakual commented 7 years ago

@PhilETaylor Can you try what happens if you set "Show Buttons" to "Yes" in the editor field plugin? Looks like that setting doesn't work properly with TinyMCE. By default ("Use Plugin") it would be off which would mean no buttons at all, but TincyMCE shows them still but broken. Codemirror properly follows that setting.

So there is another bug in Tiny which needs fixing, but not sure how much related it is.

PhilETaylor commented 7 years ago

So, testing with 4ba67b7de454befe851da3d120773d0bbd4855ea, with #14520 applied, an editor called editor, AND with setting "show buttons" to Yes (as opposed to 'use from plugin' default), a clean cache, I CAN STILL REPLICATE THE REPORTED ISSUE! (this issue, and also #14990)

PhilETaylor commented 7 years ago

I can confirm that with CodeMirror enabled (instead of TinyMCE) this issue doesn't happen. However thats probably because the buttons for the code mirror editor are those under the editor - and for tinymce they are integrated into the tinymce interface.

Bakual commented 7 years ago

That's strange. I really wonder where the difference is between your installation and mine.

PhilETaylor commented 7 years ago

As my videos clearly show - Im resetting hard my git, installing from scratch, and then replicating the issues....

Bakual commented 7 years ago

My installation is pretty much the same. It's an installation only a few days old with sample testing data and updated to current staging. I can install it fresh this evening but I honestly doubt this will make a difference.

dgrammatiko commented 7 years ago

@PhilETaylor try saving the tinyMCE plugin!

PhilETaylor commented 7 years ago

by "tinyMCE plugin" I assume you mean Extensions -> Plugins -> Editor - TinyMCE

Testing with 4ba67b7, with #14520 applied,

I did

No difference - same issue as reporting in the opening comment of this issue (Pagebreak issue #14990 is also not fixed by this test)