plone / plone.app.mosaic

Plone Mosaic main repository
https://pypi.python.org/pypi/plone.app.mosaic
GNU General Public License v2.0
35 stars 26 forks source link

Style selector in tinymce toolbar on inline Richtext Tile broken #465

Closed fredvd closed 4 years ago

fredvd commented 4 years ago

In Plone coredev (5.2.2.coredev) the styles selector in the floating toolbard of the RichtText Tile is broken.

style selector inline richtext mosaic

The problem is a side effect/regression of a fix done for TinyMCE's menus rendering off-position when TinyMCE is loaded in a pat-modal.

I submittted this bugreport in 2019: https://github.com/plone/mockup/issues/867 It was fixed early 2020 with this patch https://github.com/plone/mockup/pull/920

Which I improved in https://github.com/plone/mockup/pull/945 because the original patch didn't take into account there could be more than one TinyMCE on a page (or in a modal).

A while after implementing this patch in customer projects with mosai I noticed that while the 'native' tinymce menu's etc. are now working, there are at least two regressions:

  1. This one where the floating inline tinymce toolbar seems now bound by the container id

  2. 'sub'-modal's from some plugins in TinyMCE that open their own modal now get similar displacement issues: the optional but useful template plugin for TinyMCE as documenten at https://docs.plone.org/adapt-and-extend/config/templating.html#configure-templates has a pull down menu for selecting the template and it is completely broken.

If I revert #920/#945 by removing binding to a container_id in the tinymce pattern.js, the above two problems disappear, so I'm fairly sure these issues are related.

So..... either we have tinymce's own menu's not working in a modal, or we break the floating toolbar and sub-modals.

I've been tinkering with the #940 patch to not only check for .plone-modal-dialog as a parent class, but see if binding to the container also works for the floating toolbar in the richtext tile, but no success so far.

Another direction without any guaranteed return on time invested to fix this, is upgrading TinyMCE itself with the hope that this might be fixed, see https://community.plone.org/t/list-of-tinymce-defects-in-plone-5-2-call-for-fixing-issues-and-participation in the later comments.

@thet I've seen other issues from you where you effectively replace pat-modal with another implementation, is pat-modal root cause here as well? @cillianderoiste ?

fredvd commented 4 years ago

Just found a solution for the floating tinymce toolbar being broken: add another check in the tinymce pattern.js , like was done in https://github.com/plone/mockup/pull/945 for modals

Not only check for .plone-modal-dialog as a class on a parent container and pass that container as the ui_container to tinymce, but also check for .mosaic-grid-cell :

        var modal_container = self.$el.parents(".plone-modal-dialog")
        if (modal_container.length === 0) {
          modal_container = self.$el.parents(".mosaic-grid-cell")
        }
        if (modal_container.length > 0) {
            var random_id = Math.random().toString(36).substring(2, 15) ;
            modal_container.attr("id", "tiny-ui-container-" + random_id);
            tinyOptions['ui_container'] = "#tiny-ui-container-" + random_id;
        }

        tinymce.init(tinyOptions);

It was a bit of trial and error. Only side effect is that opening the Style dropdown shifts the whole grid cell a bit to the left in the window.

fredvd commented 4 years ago

Added checking for .mosaic-grid-cell to the backport of https://github.com/plone/mockup/pull/945

https://github.com/plone/mockup/tree/fredvd_backport_945_tinymce_modal

I don't like the solution of adding conditions to tinymce's pattern.js initialisation to check hardcoded on the context where the pattern is loaded.

Ideally the ui_container should be a (list) parameter of the TinyMCE pattern, where the caller of the pattern can inject ui_container classed/id's which are checked.

petschki commented 4 years ago

@fredvd thanks for that ... got this error last week too and was wondering why. Now it makes sense. But shouldn't we put this fix to the mosaic layout editor, where tiny is initialized instead of bringing addon logic to mockup patterns?

petschki commented 4 years ago

I think this might be a good place to fix it https://github.com/plone/plone.app.mosaic/blob/master/src/plone/app/mosaic/browser/static/js/mosaic.tile.js#L984

fredvd commented 4 years ago

But shouldn't we put this fix to the mosaic layout editor, where tiny is initialized instead of bringing addon logic to mockup patterns?

Exactly. I think the reason the first fix to the ui_container problem of TinyMCE was done mockup in the tinyMCE pattern hard coded because it interfered with another pattern (pat-modal).

But now we have a second and possibly other ui_container we need to pass we need to refactor it as a configurable parameter.

I have found one other positioning error with/inside TinyMCE for which I haven't created an issue yet: https://github.com/plone/mockup/issues/979, also a regression that was introduced after the ui_container fix. : if you add the template plugin, its modal window breaks as well. Oh the stack....

document_view -> mosaic init magic -> pat-modal -> pat-tinymce -> template-plugin-modal -> custom-dropdown

petschki commented 4 years ago

@fredvd hm ... sorry, but today I've realilzed that I did not have the latest plone.app.mosaic version in my installation. After updating to 2.2.2 the error described above is gone ... can you confirm that? So where does this come from ... mockup/staticresource update?

fredvd commented 4 years ago

The problem with the style selector only starts to happen when the ui_container is present in tinymce's config object (inside pat-tinymce 's pattern.js) The last release of plone.staticresources for Plone 5.2.1 is from January 15th. @thet merged the original ui_container fix for the displaced menu's on january 21st (https://github.com/plone/mockup/pull/920)

Unless you patched plone-logged-in.js and use your own bundle, you won't see it yet. Or maybe you saw it while running coredev-5.2? in development mode?

petschki commented 4 years ago

still (or again?) an issue with plone/5.2.2-pending and mosaic/2.2.2 ... don't know why, but it used to work until I tried -pending

fredvd commented 4 years ago

still (or again?) an issue with plone/5.2.2-pending and mosaic/2.2.2 ... don't know why, but it used to work until I tried -pending

Still. See my previous comments. In 5.2.2 lands a fix which solves some positioning problem but also introduces a regression.

I haven’t found time yet to test, but my hope is that the latest Tinymce 4.x or 5 fixes the relative positioning error we see now in different parts of Tinymce’s interface.

Plone 5.2.x still uses a TinyMCE release from 2018. See the Tinymce discussion thread on https://community.plone.org

petschki commented 4 years ago

getting close:

Bildschirmfoto 2020-07-01 um 18 32 44

by setting the ui_container to the same as fixed_toolbar_container i can now see the menus ... though due to absolute positioning at the top behind the toolbar ... I think, I get this done tomorrow ...

petschki commented 4 years ago

btw. ui_container is removed in TinyMCE 5.x https://github.com/tinymce/tinymce/issues/5241 ... this is just for 4.x versions ... maybe it would make more sense to upgrade TinyMCE in mockup ... but that's a lot more work 😄

petschki commented 4 years ago

467 should fix this (at least it does for me right now)

fredvd commented 4 years ago

btw. ui_container is removed in TinyMCE 5.x tinymce/tinymce#5241 ... this is just for 4.x versions ... maybe it would make more sense to upgrade TinyMCE in mockup ... but that's a lot more work 😄

That definetely moves up my priority for trying to get TinyMCE to 5.0 in Plone 5.2.X . There's still relative positioning errors in the template plugin of the current TinyMCE 4.x in Plone if you use tinyMCE in a modal. Removing the ui_container param seems to indicate Tiny has fixed the internal relative position errors in 5. wishfull thinking

TinyMCE 5: plone.staticresources uses this repo of a 'builded' tinyMCE and @frapell already made a 5.0.x release: https://github.com/artursmirnov/tinymce_builded/releases/tag/5.0.1

But the repo itself is abandoned by the original author, maybe we should fork it to the collective?

petschki commented 4 years ago

👍 for forking into the collective.

where do you still have positioning problems? my tiny menus looked good on a mosaic page and its "properties" modal too ... sorry ... just read "template plugin" ... never used that though