qtranslate / qtranslate-xt

qTranslate-XT (eXTended) - reviving qTranslate-X multilingual plugin for WordPress. A new community-driven plugin soon. Built-in modules for WooCommerce, ACF, slugs and others.
GNU General Public License v2.0
558 stars 107 forks source link

language swap in backend (pages and articles) not working #946

Closed bossumstore closed 3 years ago

bossumstore commented 3 years ago

Dear qtranslate-xt Team, I'm using latest version of qtranslate-xt 3.9.2 and WPBakery Page Builder 6.5.0 i having problem to switch between the languages - the switch doesnt work anymore. Instead i have the [:it] at the beginning of the page editor for the italian block and follow [:en] for the english block to close with [:]

please see screenshot how the WPBakery Page Builder page editor appears thank you for support and this amazing translation plugin

F6933B25-38B7-48A3-8A93-CF52122244EC

herrvigg commented 3 years ago

Which browser are you using?

bossumstore commented 3 years ago

firefox

herrvigg commented 3 years ago

Can you try with a different browser? I suspect this to be the same issue as #931.

MK-RD commented 3 years ago

I looked at the code and found a problem with an undefined property "h [.title] .contentField". Even if this works now for my customer, this is not a long-term solution!

MK-RD commented 3 years ago

Here is the info for your own troubleshooting!

WP (5.7-RC1-50425) QTX (3.9.2.dev.2) JSF (common.min.js)

[common.min.js]

// (function ($) { ...

    $(window).on('load', function (event) {
        console.log('[MKRD]', 'window.load');
        if (! window.tinyMCE) {
            console.log('[MKRD]', '! tinyMCE');    
        }
        if (! window.tinyMCE.editors) {
            console.log('! tinyMCE.editors');    
        }
        for (var i = 0; i < tinyMCE.editors.length; ++i) {
            var editor = tinyMCE.editors[i];
            __setEditorHooks(editor);    
        }
    });

    function __setEditorHooks(ed) {
        console.log('[MKRD]','__setEditorHooks');
        var id = ed.id;
        if (! id) {
            return;
        }
        /*! ADD ON LINE 231
            this.getContentHooks = function () {
                return contentHooks;
            };
        */        
        var qtx = qTranslateConfig.js.get_qtx();
        var contentHooks = qtx.getContentHooks();
        var h = contentHooks[id];

        if (!h) { return; }
        if (h.mce) { return; /* already initialized */ }
        h.mce = ed;

        /** * Highlighting the translatable fields */
        ed.getContainer().className += ' qtranxs-translatable';
        ed.getElement().className += ' qtranxs-translatable';

        var updateTinyMCEonInit = h.updateTinyMCEonInit;
        if (updateTinyMCEonInit == null) {
            // 'tmce-active' or 'html-active' was not provided on the wrapper
            var text_e = ed.getContent({format: 'html'}).replace(/\s+/g, '');
            var text_h = h.contentField.value.replace(/\s+/g, '');
            /** * @since 3.2.9.8 - this is an ugly trick ... */
            updateTinyMCEonInit = text_e !== text_h;
        }
        if (updateTinyMCEonInit) {
            var text = h.contentField.value;
            if (h.wpautop && window.switchEditors) {
                text = window.switchEditors.wpautop(text);
            }
            h.mce.setContent(text, {format: 'html'});
        }
        return h;
    }    

// ... })(jQuery);
herrvigg commented 3 years ago

I can't tell for this solution yet, need to look better. Meanwhile, can someone also check this from https://github.com/qtranslate/qtranslate-xt/issues/931#issuecomment-786081582 ?

Did someone else encounter the problem on a browser other than Firefox?

The issue is still present in WP 5.6.2: https://core.trac.wordpress.org/ticket/52111#comment:23

From that thread, it seems the jQuery Migration Helper could be a temporary solution: https://wordpress.org/plugins/enable-jquery-migrate-helper/

Could someone else try this?

MK-RD commented 3 years ago

WP (5.7-RC1-50425) QTX (3.9.2.dev.2) FF (85.0.2 64-Bit)

Yes, the jQuery Migration Helper (Legacy 1.12.4-wp) could be a temporary solution!

@herrvigg The script was not meant to be a solution!

MK-RD commented 3 years ago

@herrvigg The updated script (DEV) now works for Firefox. Thanks for your work!

herrvigg commented 3 years ago

@MK-RD what do you mean with DEV script? Is that related to the jQuery Migration helper?

Regarding your solution I don't understand the real difference with the current code. It's mostly the same except you added a call to qTranslateConfig.js.get_qtx(); but what happened to all the rest of the code in common.js? But this new code will surely break many other things if there's just this left. What happened to setTinyMceInit ?

The init sequence in the qTranslate code is such a pure nightmare that i don't dare to touch anything. This comment in the code tells it all...

since 3.2.9.8 - this is an ugly trick. Before this version, it was working relying on properly timed synchronisation of the page loading process, which did not work correctly in some browsers like IE or MAC OS, for example. Now, function setTinyMceInit is called after HTML loaded, before TinyMCE initialization, and it always set tinyMCEPreInit.mceInit, which causes to call this function, setEditorHooks, on TinyMCE initialization of each editor. However, function setEditorHooks gets invoked in two ways:

  1. On page load, when Visual mode is initially on. In this case we need to apply updateTinyMCE, which possibly applies wpautop. Without q-X, WP applies wpautop in this case in php code in /wp-includes/class-wp-editor.php, function 'editor', line "add_filter('the_editor_content', 'wp_richedit_pre');". q-X disables this call in 'function qtranxf_the_editor', since wpautop does not work correctly on multilingual values, and there is no filter to adjust its behaviour. So, here we have to apply back wpautop to single-language value, which is achieved with a call to updateTinyMCE(h) below.

  2. When user switches to Visual mode for the first time from a page, which was initially loaded in Text mode. In this case, wpautop gets applied internally inside TinyMCE, and we do not need to call updateTinyMCE(h) below.

We could not figure out a good way to distinct within this function which way it was called, except this tricky comparison on the next line.

MK-RD commented 3 years ago

@herrvigg Based on your statement, it has now become clear to me that you are not the author of the JavaScript code and therefore you cannot fully understand the inner workings.

Since I feel the same way, the quick solution was only implemented as an independent helper function (DEV) to show that the initialization does not run properly during the loading process.

From a sustainable perspective, it will be difficult if nobody analyzes the code, reduces it and dares to change it.

What do you suggest?

herrvigg commented 3 years ago

There were mainly 2 developers on this project, one for qTranslate and another for qTranslate-X. No sign of them anymore. I'm software engineer, among other things. This project is a useful tool but many parts of the code are extremely messy and not well designed. I've refactored many things but all of this takes time. There's still a huge work to do to make it to a good level of quality.

It's not needed to fully understand everything to make changes. But it's needed to reasonably understand the effect of changes in the current code base. WordPress is a difficult environment for integration and tests. The approach I've been following since the beginning of qTranslate-XT is to make very gradual change, it's slow but I think it's been quite successful so far.

So what we should do here is start from the current code base and just make the minimal changes to fix this issue.

MK-RD commented 3 years ago

WP (5.7-RC1-50440) QTX (3.9.2.dev.2) FF (86.0 64-Bit)

My suggestion for the minimal changes :-)

[NOTE] jQuery 3 - window load inside ready state will not be triggered (#3194)

[ADD][1426]

$(window).on('load', function(event) {
    var qtx = qTranslateConfig.js.get_qtx();
    qtx.addContentHooksTinyMCE(event);
});

[REPLACE][911]

this.addContentHooksTinyMCE = function (event = null) {

[REPLACE][1000-1013]

if (event && event.type && event.type === 'load') {
    if (window.tinyMCE && window.tinyMCE.editors) {
        for (var i = 0; i < tinyMCE.editors.length; ++i) {
            setEditorHooks(tinyMCE.editors[i]);
        }
    }
} else {
    setTinyMceInit(); 
}
herrvigg commented 3 years ago

Oooh thanks for this link: https://github.com/jquery/jquery/issues/3194

This is really helpful. It explains much better what I observed in #931 with events not firing the same every time... This is due to asynchronous firing with jQuery 3. I don't know exactly the relation with WP 5.6 but this starts to make more sense.

Indeed from that point of view, it may be fixed in qTranslate. The problem is that we don't want to add the load listener in the ready function. Let's see if we can find a solution by moving things around to cover the different cases.

herrvigg commented 3 years ago

@MK-RD i pushed a similar solution to master.

The good news, i managed to reproduce the problem and this seems to work fine.

The bad news, it still doesn't work in one case: if you leave the editor in Visual Mode and reload the page, it still fails... But if you leave the editor in Text Mode and switch back to Visual Mode this works. So I'm afraid this doesn't solve everything and perhaps we need a patch from WP for that case with the Visual Mode preselected.

Can you try to use the new version and see if you get the same results?

MK-RD commented 3 years ago

@herrvigg Which environment (version) was used?

herrvigg commented 3 years ago

WP 5.6.2 Firefox 86.0, Google Chrome 88.0.4324.190 (yes i also had the problem with Chrome)

MK-RD commented 3 years ago

Visual Mode is working! Maybe clear the browser cache and remove the cookies?

Tomorrow I will test ACF (Pro) with a clean installation of WordPress. The ACF module is not fully compatible with the current ACF (PRO) version 5.9.5 plug-in!

Microsoft Edge 88.0.705.81 (64-Bit) Mozilla Firefox 86.0 (64-Bit)

WordPress 5.6.2 (jQuery 3.5.1) QTX 3.9.2.p.2

herrvigg commented 3 years ago

That's really weird, it still doesn't work on my local website (localhost) but it works in production (with Visual Mode preselected)! In fact, that fooled me and there is a difference for me in the orders of events between Firefox and Chrome.

So I think the fix is working and that's a very good thing! But I still don't understand why it fails on my localhost. Strangely enough, it's failing both with Firefox and Chrome... likely to be for another reason.

MK-RD commented 3 years ago

Maybe it's the Transport Layer Security (TLS) and the QTX settings?

herrvigg commented 3 years ago

Unfortunately it's not that, the cookies are set correctly and all the rest is working. It's really strange the same issue happens for both browsers, this doesn't seem related to the async firing. I disabled all plugins and can't see a single error. So annoying.

MK-RD commented 3 years ago

It works with my fresh LOCAL installation!

With the already existing content (Hello World!) I had to remove it first. Try it with a new Post first.

Please don't ask me why :-)

Probably because the post was initially created with the Block Editor.

herrvigg commented 3 years ago

Hmm... indeed that also works locally if I create a new post (!).

I don't remember if the problematic post was created with Gutenberg, I don't think so. I checked the raw content and I don't see anything weird. There is no ajax call involved that could load the content differently.

But I observed another strange situation, the same code behaves differently if I run with DEBUG_SCRIPT or not. With DEBUG_SCRIPT it works, without it it doesn't. I don't know if this is due to qTranslate minified or not, or if it comes from another script in WP.

I spent quite some time to debug this and it still have not found the explanation but at least I understand some parts of the code better, especially the loading sequence that differs between the HTML editor vs visual editor.

My hypothesis is that in rare cases (?), the Visual Editor TinyMCE could be initialized before the content value of the textarea is changed for the current language. Then qTranslate changes the value and sets the MCE hook but it's too late. I don't know if this is related to the async events with jQuery3. Or there is some cache issue but I don't see how.

herrvigg commented 3 years ago

In fact it could be related to Gutenberg. If I create a new post with Gutenberg and I switch back to the Classic Editor, the same problem with untranslated content appears again... but only with SCRIPT_DEBUG = false. I suspect something going wrong with a cache but I haven't found it yet.

herrvigg commented 3 years ago

Oh-oh I found the problem... and it's worse than what I thought. Not related to Gutenberg, it looks like a coincidence.

With the Classic Editor, TinyMCE init is called directly right after tinymce.min.js is loaded, not on ready or load (!). But qTranslate initializes its hooks on ready or load (with the last patch). So qTranslate is running too late...

If TinyMCE initializes its editor too fast, it will take the raw (untranslated) content and then we're screwed. I hacked the WP code in wp-class-editor.php to delay the initialization with a timeout and promise. Just 10ms were enough in my case.

In fact it's a general problem when the Visual Mode (tmce) is set, it's just a matter of luck that it worked (!!). When the editor is html and we switch later, it's fine because the qTranslate is already done. With SCRIPT_DEBUG what could happen is that some scripts take longer and it just a matter of luck with the loading sequence.

Now the question is how to solve this. Until now we were delaying qTranslate initializations until the ready event. If I run it before this could create new problems trying to access DOM elements that are not set... But still, considering how the Classic Editor works, we have a possibility with the wp_tiny_mce_init hook in PHP to write some JS code there. But maybe we don't want this when the page is not about the editor...

Another approach would be to check if the editor was initialized too early and "post-fix" it only in that cases but that would be another hack...

This is a tricky situation!

MK-RD commented 3 years ago

Well, then with a status check!

/**/
if (event && event.type && event.type === 'load') {
    loadTinyMceHooks();
}            
/* Usually only once? */
if (! this.initialized_tmce) {
    this.initialized_tmce = true;
    if (! event) {
        loadTinyMceHooks();
    }
    setTinyMceInit();
}            

[UPDATE] With the following two lines (only), both editor modes should work - please test thoroughly!

console.log('qtx.addContentHooksTinyMCE(event)', event);
loadTinyMceHooks(); // Block Editor
setTinyMceInit();

[NOTE] I cannot understand the program logic and the amount of code that was used to accomplish such a simple task. Annoying. I don't even know where to start.

TinyMCE version 4.9.11 July 13, 2020 https://www.tiny.cloud/docs-4x/api/tinymce/root_tinymce/

The editors (Object) can contain more than expected! Collection of editor instances. Deprecated use tinymce.get() instead. ...

MK-RD commented 3 years ago

By the way, how can someone change the language in block editor mode?

trebere commented 3 years ago

By the way, how can someone change the language in block editor mode?

QTX LSB BLOCK EDITOR

You should disable the Full Screen Mode and the admin top bar will appear.

herrvigg commented 3 years ago

/ Usually only once? / if (! this.initialized_tmce)

No, we can't use that. I had the same idea but that does not work. As explained earlier, the problem is that qTranslate runs on ready or load. With the tmce mode, the TinyMCE initialization is done before, just in the flow of the script right after loading the related Tinymce JS script, so it will likely take the untranslated content. In fact I don't even understand how it could work until now, it seems those could run in parallel.

With the tmce mode the best solution would be to initialize qTranslate before the init of TinyMCE. We can't do that when qTranslate is loaded, but we may do it via wp_tiny_mce_init to generate specific JS code for qTranslate.

[UPDATE] With the following two lines (only), both editor modes should work

I don't see how this relates to the first block.

I cannot understand the program logic and the amount of code that was used to accomplish such a simple task. Annoying. I don't even know where to start.

Hmm. What do you mean?

The editors (Object) can contain more than expected! Collection of editor instances. Deprecated use tinymce.get() instead.

Yes i also saw that recently.

herrvigg commented 3 years ago

console.log('qtx.addContentHooksTinyMCE(event)', event); loadTinyMceHooks(); // Block Editor setTinyMceInit();

Note that for Gutenberg, the LSB mode is disabled so we will never execute that code. Because we need much more to manipulate the React internal state, we fallback to a single language mode for the block editor (with the main admin language as shown above) so there's no substitution in JS and therefore no hook for the content. It's all done from PHP.

loadTinyMceHooks is supposed to handle "more TinyMCE editors, which may have been initialized dynamically", but I don't exactly know all the cases that are concerned. When we switch dynamically from html (raw textarea) to tmce (visual mode) the script goes through that case but maybe that could even be handled differently.

Also, from a legacy comment in the code for updateTinyMCEonInit:

since 3.2.9.8 - this is an ugly trick. Before this version, it was working relying on properly timed synchronisation of the page loading process, which did not work correctly in some browsers like IE or MAC OS, for example.

This could be related to this issue of loading synchronization that I just found. Certainly not related to the async firing with jQuery3 since this was already the case in old code with older jQuery that were synchronous. This part seems only related to wpautop also mentioned in #910. I don't use this myself and I thought it was entirely disabled in qTranslate but this part of code was meant to fix that wpautop feature (in a very hacky way). I don't know if this really works.

MK-RD commented 3 years ago

Why does this work for me?

this.addContentHooksTinyMCE = function (event) {
    function setEditorHooks(ed) {...};
    setTinyMceInit = function () {...};
    loadTinyMceHooks = function () {...};

    console.log('qtx.addContentHooksTinyMCE(event)', event);
    loadTinyMceHooks();
    setTinyMceInit();
};

$(window).on('load', function(event) {
    var qtx = qTranslateConfig.js.get_qtx();
    qtx.addContentHooksTinyMCE(event);
});
herrvigg commented 3 years ago

Good question! From what I see now, it's just "by luck". It also works for me in most of the cases but not always. For some reason, when the load event is fired here in all your cases, TinyMCE did not manage to run its initialization yet. So qTranslate runs before, set the hooks, and TinyMCE initializes after with the translated content.

But this should not happen! That code is run before, see here: https://github.com/WordPress/WordPress/blob/52679edbffe1a28cf7fce54ab2077f299c8cf27e/wp-includes/class-wp-editor.php#L1679

That code is executed in the footer right after the script loading. Is that code run in parallel? That would be weird. My understanding is there is some internal event in TinyMCE that may delay the initialization in some cases (or most of?)... and "by luck" it happens after ready giving the chance for qTranslate to do its job. Why? I don't know!

The correct sequence should be the following:

The problem is that the code currently looks like this:

I'm doing experiments now with wp_tiny_mce_init which is a WP action fired right before TinyMCE init. That should solve our problem because it makes the loading sequence synchronous. That would only be for the pages with an editor. The risk is that could disturb some cases where the content hooks needed to delayed up the ready or load event. For the regular WP that should not be a problem, but that is not guaranteed to be true for all the integration with other plugins.

Also, why do we need the load event in first place? With that new sequence perhaps we don't need it anymore.

herrvigg commented 3 years ago

@MK-RD for test, and to understand better what happens, you may add just some logging here: https://github.com/WordPress/WordPress/blob/52679edbffe1a28cf7fce54ab2077f299c8cf27e/wp-includes/class-wp-editor.php#L1679

wp-includes/class-wp-editor.php:

if ( ( $wrap.hasClass( 'tmce-active' ) || ! tinyMCEPreInit.qtInit.hasOwnProperty( id ) ) && ! init.wp_skip_init ) {
  // add just the following line before L1679
  console.log('QTX TinyMce init with content=', jQuery('#content')[0].value);
  tinymce.init( init );

This is expected to be executed before addContentHooksTinyMCE, with the risk to initialize TinyMCE too early. The internal init sequence in TinyMCE is harder to change because WP only provides the minified version.

MK-RD commented 3 years ago

[LOG] `QTX TinyMce init with content= [:de]de[:en]en[:] window.onload Object

herrvigg commented 3 years ago

Yes exactly, that's what i expected. It calls tinymce.init with the untranslated content. So why does it work in this case? It's surely related to that init event fired internally in TinyMCE: https://github.com/tinymce/tinymce/blob/develop/modules/tinymce/src/core/main/js/JqueryIntegration.js#L70

Who fires that event and when?!

MK-RD commented 3 years ago

Well, WordPress of course.

herrvigg commented 3 years ago

That would be surprising, this looks like a purely internal event in TinyMCE.

I found this looking quite similar: https://github.com/tinymce/tinymce/blob/9c01df42d50c4c3a9a89453d9f157afe657a32a9/modules/tinymce/src/core/main/ts/init/InitContentBody.ts#L262

But this is firing 'Init' not 'init' and this is supposed to be case-sensitive 😕

Anyhow this is called here on loaded: https://github.com/tinymce/tinymce/blob/9c01df42d50c4c3a9a89453d9f157afe657a32a9/modules/tinymce/src/core/main/ts/init/InitContentBody.ts#L304

It's quite complicated to follow. Could this be run effectively on the load event? That would mean the first init call does not read the content at that time but also delays it. The question could then be in which order the load handlers are executed...

herrvigg commented 3 years ago

That 'loaded` event is a promise handler that seems related to the loading of the stylesheets... https://github.com/tinymce/tinymce/blob/9c01df42d50c4c3a9a89453d9f157afe657a32a9/modules/tinymce/src/core/main/ts/init/InitContentBody.ts#L309

So I'm not 100% sure (this is not even guaranteed to be the same code as for TinyMCE4 in WP) but if we follow a bit the logic behind the init in TinyMCE, that could finally explain the random behaviors!

MK-RD commented 3 years ago

[304][class-wp-editor.php]

printf( $the_editor, $content );.
MK-RD commented 3 years ago

How can I reproduce the error you are trying to describe or is it about look and feel?

herrvigg commented 3 years ago

I think it's very hard to reproduce since it's likely to depend on the loading of external files.

However my view is that we should not make any assumption on how TinyMCE initializes itself. Let's go a hopefully safer way by anticipating the init of qTranslate before that call. I've just pushed a new patch to master (see above). I think you will understand the idea.

My only fear is that it could have some regressions in tricky integration cases that requires a delay init, but those should be addressed separately. I believe the solution i just pushed is the safest from the TinyMCE / Classic Editor point of view.

herrvigg commented 3 years ago

This code now works for me in all cases. Please try it also on your side.

When we have a TinyMCE editor, this means we don't need ready and maybe we could even get rid of load handler but this part is a bit touchy if there are other external editors initialized differently. It's probably better to leave all the rest in the current state. Moving the load out of ready (the first fix you suggested) was also required to deal with the async events in jQuery3.

MK-RD commented 3 years ago

So far everything seems to be going well. No different behavior can be determined.

`QTX TinyMce init with content= en

herrvigg commented 3 years ago

Good. Now we are sure the init sequence is synchronous between qTranslate and TinyMCE.

TinyMCE version 4.9.11 July 13, 2020 https://www.tiny.cloud/docs-4x/api/tinymce/root_tinymce/

The editors (Object) can contain more than expected! Collection of editor instances. Deprecated use tinymce.get() instead.

This is more annoying than expected. They give a get() but there's nothing returning the list of editors. That supposes we know the editor ID but we don't if they are created dynamically. Perhaps the loop should be rewritten from the content hooks related to MCE content (textarea).

MK-RD commented 3 years ago
loadTinyMceHooks = function () {
    if (window.tinyMCE) {
        // Returns Array of Editors
        tinyMCE.get().forEach(function (item) {
            setEditorHooks(item);
        });
    }
};

Without an argument you get an array! I know this is a strange implementation.

herrvigg commented 3 years ago

Oh wow I didn't even imagine that... Many thanks!

herrvigg commented 3 years ago

Now I also removed the ready handler to simplify a bit this mess :)

With jQuery3, ready and load can be fired asynchronously. We cannot assume ready is fired before load. Since qtx is also initialized in load, no need for ready anymore.

herrvigg commented 3 years ago

More refactoring to simplify all this of mess of nested functions and events: https://github.com/qtranslate/qtranslate-xt/pull/978/

herrvigg commented 3 years ago

Merged #978 in master. The executed code is the same (yet without ready) but it should be much easier to read now!

MK-RD commented 3 years ago

Looks good to me!

herrvigg commented 3 years ago

Released in 3.9.3.