Syxton / moodle-tiny_sketch

3 stars 7 forks source link

TinyMCE/Sketch: Defining a context menu section #11

Closed toanlamt closed 2 months ago

toanlamt commented 3 months ago

Hi @Syxton

I'm from The Open University, and we have the same problem discussed here.

For the convenience of users and to solve this problem, we need to define the context menu section to control when the context menu will be shown instead of it appearing when right-clicking on a blank area. For reference, please see this documentation: Defining a Context Menu Section

In my commit, I registered the addContextMenu() function to handle this as in the document and it is the same in tiny/plugins/media

alec-gullon commented 3 months ago

Hi @toanlamt. Just wanted to express appreciation for you looking into the problem. Over at Lancaster University, we are running into the same issue. I had literally just finished concluding that there was a problem with the setup of the context menu, then I visited the linked ticket and saw your pull request.

The trouble is, there is also a problem with how Moodle's tiny/plugins/media plugin handles its calls to addContextMenu. And this problem is duplicated by your pull request.

If you look at the linked codepen in this section of the tiny documentation: https://www.tiny.cloud/docs/tinymce/latest/contextmenu/#defining-a-context-menu-section, you can see that the update key expects a function which returns a string as opposed to a bool. The returned string points at the plugin menu item that should be displayed.

Since Moodle have defined their update key to return a bool, their setup does not work properly for images - if a user right-clicks on an image, they do not see a context menu. You can verify this on qa.moodledemo.net by adding an image to a text editor and right-clicking. It probably hasn't been picked up, because a context toolbar is brought up instead which would appear to be correct behaviour, but this is distinct from a context menu. Modify it to use the API correctly, and you see something like this when right clicking on an image:

image

The TL:DR is this: Moodle have a bug which means that their tiny media plugin never brings up a proper context menu. This proposed fix has the same issue. However, it's not the end of the world, because with the proposed fix as is, when you right-click on an image, you get a context toolbar which provides all the functionality the context menu was going to provide anyway!

So I think the correct solution would be to modify the call to addContextMenu to always return the empty string. That prevents the Sketch menu item from ever appearing in the right-click menu - again, not a problem, given the existence of the toolbar. The tiny API is weird, in that if you don't specify anything with addContextMenu, it automatically adds any plugins menu items to the context menu no matter where you right-click. Which honestly could be a design flaw - but I'm not sure, I've only been looking into this stuff for a day.

But it definitely feels like something needs to be done about this, given that two teams at two different universities are both trying to get a fix implemented at the exact same time.

toanlamt commented 3 months ago

Hi @alec-gullon,

I appreciate your feedback. You are correct that the update key expects a function that returns a string, and I realized that ContextMenu is unnecessary because the Sketch item will display on ContextToolbar when clicking on image. image.

I think your approach will be better to resolve this issue. I have updated my code as well and am waiting for the Sketch plugin owner to review it.

Thank you.

Syxton commented 2 months ago

Thank you for looking into this so thoroughly. I have adopted the code change in my latest commits. Thanks again @toanlamt and @alec-gullon