Opened 7 years ago

Closed 7 years ago

#9284 closed Bug (fixed)

Disabling custom context menu by adding it to removePlugins list doesn't work

Reported by: Piotrek Koszuliński Owned by: Piotrek Koszuliński
Priority: Normal Milestone: CKEditor 4.0
Component: General Version: 4.0
Keywords: Cc:

Description

  1. Add removePlugins: 'contextmenu' option to any "bycode" sample.
  2. Open this sample.
  3. Press rmb.
  • Expected: native context menu
  • Actual: CKEDITOR's context menu

removePlugins works for e.g. basicstyles, so that part is ok.

Change History (18)

comment:1 Changed 7 years ago by Piotrek Koszuliński

Owner: set to Piotrek Koszuliński
Status: newassigned

comment:2 Changed 7 years ago by Piotrek Koszuliński

Intreseting... contextmenu can't be easily removed from plugins list, because it's required by liststyle and tabletools. So it may be disable by adding all these 3 plugins to the list.

It makes sense, but maybe we should make it possible to somehow force disabling the plugin (e.g. by 'plugin!' syntax in removePlugins. If not then we need at least clear note about that in doc.

Please comment, guys :).

comment:3 Changed 7 years ago by Olek Nowodziński

I think that removing a plugin which is required by some others should throw an appropriate error. It's not a big deal as editor already throws errors e.g. when overwriting an element with other instance (inline & themedui creators).

Regardless of what we decide to do, documentation would also be necessary.

comment:4 Changed 7 years ago by Piotrek Koszuliński

Throwing an error may be the best solution, because developer will be notified about why plugin wasn't disabled. On the other hand I'm not sure if this error should stop editor's creation. It's not so critical and white-screen definitely isn't desirable. So non-blocking error?

comment:5 Changed 7 years ago by test1

So I tested also removing "liststyle" and "tabletools" and it did disable the custom context menu but it didn't enable the default one. I worked around this by adding the class "cke_enable_context_menu" to the "cke_wysiwyg_div" on instanceReady and that fixed it for me.

comment:6 Changed 7 years ago by Alfonso Martínez de Lizarrondo

Maybe liststyle and table tools shouldn't force the contextmenu plugin?

Other plugins check for the existence of the contextmenu and then they add the proper entries, so why don't these plugins do the same?

If the answer is "because that's the only way to access this functionality", then maybe these plugins should provide optional toolbar buttons so that they act more like other plugins and they can be accesed from the main toolbar if desired by the user (as many people don't feel right with context menus) and so they aren't tied to the context menu

comment:7 Changed 7 years ago by Jakub Ś

I believe this may be a dup of #9195.

To my knowledge tabletools does not need context menu.

comment:8 Changed 7 years ago by Piotrek Koszuliński

@test1: What do you mean by it didn't enable default one? When I initialize editor with this code: CKEDITOR.replace( 'editor1', { removePlugins: 'contextmenu,tabletools,liststyle' } ); (both - inline and themed editors) and right-click the editor I get native context menu. Tested on Chrome and Firefox @ latest master. Don't you see the same?

@alfonsoml: These two plugins currently don't exist without context menu. The difference between them and other plugins is that all their features requires context menu, thus they should require it now.

However, that's a good point that we may think about moving these functionalities from context menu to toolbar. I saw this trend in some UIs and I believe that it makes sense. Anyway - not in a range of this ticket.

@j.swiderski: Maybe tabletools don't crash without contextmenu, but I think that they have to require it now. It'll be less confusing.

comment:9 Changed 7 years ago by Piotrek Koszuliński

One more thing - in the private talks we agreed that for now silent error (not stopping the initialization, so e.g. in setTimeout's thread) should be thrown.

comment:10 Changed 7 years ago by Jakub Ś

@j.swiderski: Maybe tabletools don't crash without contextmenu, but I think that they have to require it now. It'll be less confusing.

What is worse, to have a bug or less "confusing code"? Besides less confusing to who? Most users don't care about what dialog requires but they sure will care that context menu can't be disabled.

Try the fix from #9195. It worked for CKE 3.6.4 so maybe it will work for CKE 4.

Last edited 7 years ago by Jakub Ś (previous) (diff)

comment:11 Changed 7 years ago by Piotrek Koszuliński

Fix for #9195 just hides the problem. You load tabletools and see nothing, because there's no context menu. For me it's silent failure.

This is why it's better to require context menu and to notify user if he's trying to remove plugin which's required by other.

comment:12 Changed 7 years ago by Jakub Ś

@Reinmar thanks for clarification.

Yes, this is important for v4 where user will be able to choose his set of plugins. This gives user or some auto tool information of what plugins are needed.

This may be a bit of pain for some users, who download contextmenu and tabletools and want to disable contextmenu later, as to disable it they need to remove tabletools as well (I think you will agree that this is not obvious):

config.removePlugins='tabletools,contextmenu';
Last edited 7 years ago by Jakub Ś (previous) (diff)

comment:13 in reply to:  12 Changed 7 years ago by Piotrek Koszuliński

This may be a bit of pain for some users, who download contextmenu and tabletools and >want to disable contextmenu later, as to disable it they need to remove tabletools as >well (I think you will agree that this is not obvious):

config.removePlugins='tabletools,contextmenu';

Yes, that's right. This is why we'll be throwing an error if someone is trying to remove e.g. contextmenu, when other plugins requires it. Then it will be obvious.

Last edited 7 years ago by Jakub Ś (previous) (diff)

comment:14 in reply to:  10 Changed 7 years ago by Olek Nowodziński

Replying to j.swiderski:

@j.swiderski: Maybe tabletools don't crash without contextmenu, but I think that they have to require it now. It'll be less confusing.

What is worse, to have a bug or less "confusing code"? Besides less confusing to who? Most users don't care about what dialog requires but they sure will care that context menu can't be disabled.

Try the fix from #9195. It worked for CKE 3.6.4 so maybe it will work for CKE 4.

The fix you've just mentioned brings more confusion than clarity. We cannot hide important cross-dependencies away from our users as we cannot expect all of them to understand the nature of the problem. If we did so, they would report bugs if something went unexpectedly wrong as a consequence of this patch. And this is something wrong IMO.

Instead of bypassing the problem we should inform the users of the consequences of what they're trying to do. This is what errors have been invented for.

comment:15 Changed 7 years ago by Piotrek Koszuliński

Pushed t/9284 branch @ dev.

I haven't created tests, because catching errors in this case is a little bit tricky (I'd need to override setTimeout (custom or native one)). However, patch may be easily verified by e.g.

var editor = CKEDITOR.inline( 'editable', { removePlugins: 'contextmenu,dialog' } );

... Or maybe I'll create also tests.

comment:16 Changed 7 years ago by Piotrek Koszuliński

Status: assignedreview

Pushed t/9284 branch @ tests.

comment:17 Changed 7 years ago by Frederico Caldeira Knabben

Status: reviewreview_passed

Minor revert made on unnecessary change.

comment:18 Changed 7 years ago by Piotrek Koszuliński

Resolution: fixed
Status: review_passedclosed
Note: See TracTickets for help on using tickets.
© 2003 – 2019 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy