Opened 8 years ago

Closed 8 years ago

#4343 closed New Feature (fixed)

Implement BrowserContextMenuOnCtrl

Reported by: Alfonso Martínez de Lizarrondo Owned by: Garry Yao
Priority: Normal Milestone: CKEditor 3.1
Component: UI : Context Menu Version: 3.0
Keywords: Review+ Cc:

Description

Port that setting from FCKeditor

Attachments (3)

4343.patch (1.8 KB) - added by Garry Yao 8 years ago.
4343_2.patch (1.9 KB) - added by Garry Yao 8 years ago.
4343_3.patch (1.9 KB) - added by Garry Yao 8 years ago.

Download all attachments as: .zip

Change History (10)

Changed 8 years ago by Garry Yao

Attachment: 4343.patch added

comment:1 Changed 8 years ago by Garry Yao

Keywords: Review? added
Owner: set to Garry Yao
Status: newassigned

comment:2 Changed 8 years ago by Frederico Caldeira Knabben

Keywords: Review- added; Review? removed
  • The browserContextMenuOnCtrl is plugin specific and should not be in config.js.
  • In any case, it's not needed to declare the above config. It can be a "documentation only" thing, just like the dialog settings.
  • The context menu plugin should be used in the future to activate context menus in other places, like when right clicking on element path tags to act over them, or even into the toolbar to customize it. So, it's better to not change it's signature to make it editor area specific. Other than that, the contextMenu class already receives the editor as a constructor parameter, so passing it to addTarget is redundant.

What about an additional parameter to addTarget, which tells whether to show the browser context menu on CTRL?

Changed 8 years ago by Garry Yao

Attachment: 4343_2.patch added

comment:3 Changed 8 years ago by Garry Yao

Keywords: Review? added; Review- removed

Fixing the above mentioned issue and update the patch with #4530.

comment:4 Changed 8 years ago by Frederico Caldeira Knabben

Keywords: Review- added; Review? removed

It's almost perfect. The only problem is that now you're not considering the default value associated to the setting. In this case, it's "true". In effect, the feature is not available by leaving the config file empty.

The pattern for that is the following, when retrieving the setting:

var browserContextMenuOnCtrl = editor.config.browserContextMenuOnCtrl;
browserContextMenuOnCtrl = ( typeof browserContextMenuOnCtrl == 'undefined' || browserContextMenuOnCtrl );

Changed 8 years ago by Garry Yao

Attachment: 4343_3.patch added

comment:5 Changed 8 years ago by Garry Yao

Keywords: Review? added; Review- removed

comment:6 Changed 8 years ago by Frederico Caldeira Knabben

Keywords: Review+ added; Review? removed

comment:7 Changed 8 years ago by Garry Yao

Resolution: fixed
Status: assignedclosed

Fixed with [4540].

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