Opened 6 years ago

Closed 6 years ago

#11165 closed Bug (fixed)

The filebrowser plugin cannot be removed from the editor

Reported by: Wiktor Walc Owned by: Marek Lewandowski
Priority: Must have (possibly next milestone) Milestone: CKEditor 4.3.1
Component: General Version: 4.0
Keywords: Cc:

Description

  1. Download the standard package
  2. Modify the replacebycode sample to include just a set of plugins:
    CKEDITOR.replace( 'editor1', {
    	plugins: 'enterkey,entities,toolbar,wysiwygarea,image',
    	filebrowserBrowseUrl : '/ckfinder/ckfinder.html'
    } );
    
  3. Open the sample, press the Image button. See that the ""Browse Server" button is available, although the filebrowser plugin isn't really enabled.

The reason why is it happening is the way how the plugin interacts with the editor:

CKEDITOR.on( 'dialogDefinition' is executed always when this plugin is merged into ckeditor.js (in release process). However, the init method of a plugin is not executed, because it is not enabled.

One of possible solutions for this issue could be to check by the plugin itself inside CKEDITOR.on( 'dialogDefinition' if I'm really enabled.

Change History (9)

comment:1 Changed 6 years ago by Olek Nowodziński

cc

comment:2 Changed 6 years ago by Olek Nowodziński

Milestone: CKEditor 4.3.1

comment:3 Changed 6 years ago by Piotrek Koszuliński

Priority: NormalHigh
Status: newconfirmed

I agree with Olek that this may be extremely confusing.

comment:4 Changed 6 years ago by Marek Lewandowski

Owner: set to Marek Lewandowski
Status: confirmedassigned

comment:5 Changed 6 years ago by Marek Lewandowski

Status: assignedreview

Fixed in t/11165 dev.

comment:6 Changed 6 years ago by Frederico Caldeira Knabben

Status: reviewreview_failed

The idea is good... some remarks about the implementation though:

  • Couldn't "!( 'filebrowser' in evt.editor.plugins )" be replace with the more compact "!evt.editor.plugins.filebrowser"?
  • Any chance to have a test for it?

comment:7 Changed 6 years ago by Marek Lewandowski

Status: review_failedreview

You're right about shortening this statement.
As for tests i didn't want to sacrifice too much time at the first time, however now i got quick idea how to write short TC. Unfortunately this test depends on other plugin. I've chosen flash, since image will be replaced with image2 soon.

Pushed to t/11165 dev and t/11165 tests.

comment:8 Changed 6 years ago by Olek Nowodziński

Status: reviewreview_passed

Rebased both branches.

  • I pushed a very minor commit to dev branch to standardize the comment.
  • I pushed a not very minor commit to tests:
    • Fixed wrong <title> of the test HTML.
    • Used a generic dialog instead of flash dialog. This test should not depend on another, unrelated plugin which may mutate or even be removed in the future.
    • Closed dialog after the assertion.
      • Used try,catch,finally because in case of failure dialog would remain open and distort other tests.

comment:9 Changed 6 years ago by Marek Lewandowski

Resolution: fixed
Status: review_passedclosed

Masterized with git:cb474052e in dev and a5ae4293f in tests.

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