Opened 12 years ago
Closed 12 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
- Add
removePlugins: 'contextmenu'
option to any "bycode" sample. - Open this sample.
- 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 12 years ago by
Owner: | set to Piotrek Koszuliński |
---|---|
Status: | new → assigned |
comment:2 Changed 12 years ago by
comment:3 Changed 12 years ago by
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 12 years ago by
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 12 years ago by
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 12 years ago by
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 12 years ago by
I believe this may be a dup of #9195.
To my knowledge tabletools does not need context menu.
comment:8 Changed 12 years ago by
@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 12 years ago by
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 follow-up: 14 Changed 12 years ago by
@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.
comment:11 Changed 12 years ago by
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 follow-up: 13 Changed 12 years ago by
@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';
comment:13 Changed 12 years ago by
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.
comment:14 Changed 12 years ago by
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 12 years ago by
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:17 Changed 12 years ago by
Status: | review → review_passed |
---|
Minor revert made on unnecessary change.
comment:18 Changed 12 years ago by
Resolution: | → fixed |
---|---|
Status: | review_passed → closed |
Masterised with https://github.com/ckeditor/ckeditor-dev/commit/d6f9f94
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 inremovePlugins
. If not then we need at least clear note about that in doc.Please comment, guys :).