parttio / tinymce-for-flow

TinyMCE wrapper for Vaadin 10+
Other
8 stars 6 forks source link

Errors working with TinyMce in a Dialog #27

Closed SonReimer closed 8 months ago

SonReimer commented 8 months ago
SonReimer commented 8 months ago

TestCase-TinyMce4_1_1.java.txt

mstahv commented 8 months ago

Something related to @PreserveOnRefresh thingie? I never use that myself and probably the component is not well tested with it. Are you sure you need that in your app?

mstahv commented 8 months ago

Added some fixes. Your test case ~ as such in https://github.com/parttio/tinymce-for-flow/blob/master/src/test/java/org/vaadin/tinymce/PreserveOnRefreshBug27.java

The Dialog component seems to have weird behaviour if you use it like in your original code snippet, that you attach it but don't call show method. That makes its children "attached" on the server side, but nothing is really attached on the client -> "everything broken". I have never done that myself as calling open method does that for me. Thus I removed that form the code as well. Without it "all" seems to work now.

I also removed the boolean parameter from the constructor. I have no idea if that hack is needed anymore with the most recent Vaadin version 🤷‍♂️

I'll cut a bugfix release, will be in central soonish, but will leave this open in case I get inspired to look into that usage of dialog in your way. But that most likely will escalate into Dialog bugs...

mstahv commented 8 months ago

Some issue in Sonatype OSS server. Re-igniting the release build at some point: https://status.maven.org

SonReimer commented 8 months ago

You are right. The dialog should only be attached by calling open(). That was a mistake by myself. And I wondered, why the behavior was another in the showcase than in my real application. After removing the dialog from the main view and applying your fixes temporarily everything worked fine. The only thing that's left is the fact, that the editor gets editable after refresh, when using @PreserverOnRefresh that we rely on. A possible workaround could be to not writing the value back when it's disabled/readOnly to avoid security issues.

SonReimer commented 8 months ago

I could fix the @PreserveOnRefresh-Bug. If you additionally call adjustEnabledState(); at the end of the onAttach() method the enabled state gets preserved on F5(Refresh) Perhaps you could add it to your fix !?

mstahv commented 8 months ago

Ah, I didn't even figure out that that was broken. You fix sounds good, I can land that, but OSS sonatype seems to have issue still so an actual release might take a while still.

mstahv commented 8 months ago

A slightly different solution in the repo now, I'll check if we can cut that release tomorrow!

SonReimer commented 8 months ago

Thanks for the release. Things mentioned here are now working for me. But I detected another two things when using the TinyMce in a dialog.

  1. I cannot set the focus in a dialog, neither by keyboard nor programmatic
  2. Now sometimes on opening and mostly on closing the dialog it shows a content only presentation for a short time (flickering)
mstahv commented 8 months ago

Hi, would you create other issues of those? Would help if all things are not in the same.

SonReimer commented 8 months ago

Yes you are right.

mstahv commented 8 months ago

@SonReimer Using this channel to ask one more question regarding Dialog (as you seem to use them a lot). Do you happen to use TinyMCE only in dialogs or both in dialogs and normal content? We (well, @TatuLund ) just landed a change enabling styling that makes the editor fit better (can respect) Lumo theme variables. A tiny issue in it is that it doesn't either work in Dialog, because of this shadow dom thingie...

Wondering, if we should:

mstahv commented 8 months ago

I'll check if there are easy fixes for your new issues, before cutting a release.

TatuLund commented 8 months ago

According to my testing the use cases work well with the newest development version and Shadow DOM use is no longer needed. Hence also Lumo styling works well with Dialog as well.

mstahv commented 8 months ago

@TatuLund Noticed you deprecated the "wrap in shadowroot" option and change the EditorInDialog test. Does the menu work for you if you make the Dialog modal (uncomment setModal(false)) ? In Safari at least the menu seems to be non-functional then. Toolbar works fine.

SonReimer commented 8 months ago

@mstahv We are using TinyMCE in dialogs and normal content. But according to @TatuLund 's comment this answer may be already outdated.

TatuLund commented 8 months ago

@mstahv You are correct, the modal Dialog breaks the menu. And I see the reason is that the aux element used for the menu is on a body level and not a child of the overlay, thus modality curtain blocks.

mstahv commented 8 months ago

I was testing it bit further as well. The sad thing is that the constructor enabling shadow dom only seems to help Safari (and there it breaks the new Lumo theme).

So the only reasonable way to use TinyMCE in Dialog is to make call setModal(false) for Dialog 🤷‍♂️ Unless some uber hacks would fix it.

mstahv commented 8 months ago

@TatuLund made a tiny workaround that seems to fix menus now for all browsers in modal dialogs 💪 I didn't even dare to try that as I was pretty sure it wouldn't work, but it indeed does 🥳

I think we finally now have proper Dialog support on all browsers. The tiny flickering is still there, but not sure if we can get rid of that. 4.2.0 version is currently in build pipeline.