Placeholder plugin broken

[jeffreydev]

When editing (by double clicking) a placeholder, the plugin triggers an error.

To recreate:

1. Open samples/placeholder.html
2. Add new paragraph.
3. Click on [P] icon in toolbar.
4. Enter placeholder text (I used test) and click on OK.
5. Double click the newly inserted placeholder.

Expected result: Opens the dialog so you can change the placeholder name.

Current result: Javascript error is being thrown, no dialog is opened.

The error:

TypeError: current.is is not a function	

if ( current.is( 'body' ) || !current.isReadOnly() )

core/selection.js line 960.

[Piotrek Koszuliński]

This plugin is completely broken. Steps to reproduce: load placeholder sample, double click, see how badly it presents itself on the console.

[Piotrek Koszuliński]

Created branches t/9212 on dev&tests.

I've fixed two issues I encountered when double clicking on placeholder.

Unfortunately, I can't find solution for the last issue - opening placeholder dialog by context menu. It looks like just before call to CKEDITOR.plugins.placeholder.getSelectedPlaceHolder selection is lost on Chrome. I was testing this by:

setInterval(function() { console.log(CKEDITOR.instances.editor1.getSelection().getRanges()[0].startContainer.getText()) }, 2000);

1. Open placeholder sample.
2. Run this code.
3. At the beginning 'This is a ' is logged.
4. Right click on placeholder, so context menu opens.
5. Now '[[sample placeholder]]' is logged.
6. Click placeholder option in context menu.
7. Exception is thrown because range is lost:

getSelectedPlaceHolder: function( editor ) {
    var range = editor.getSelection().getRanges()[ 0 ];
    // range == "^This is a..."

I can't find what's causing this lost. I can't even tell if that's Chrome's fault or ours. Help needed :)

[Piotrek Koszuliński]

Bisect for the rescue - this is first bad commit: ​https://github.com/ckeditor/ckeditor-dev/commit/44f81a772cbd5fa59acb27579ea8d7178fbd924b

[Piotrek Koszuliński]

Quick fix for this issue - remove this condition for Webkit ​https://github.com/ckeditor/ckeditor-dev/blob/master/plugins/floatpanel/plugin.js#L389

[Garry Yao]

On top of the existing branch, I've the last issue mentioned by Piotrek in comment:3, for Webkit.

[Piotrek Koszuliński]

I pushed rebased t/9212.

Last commit breaks these tests (at least on Chrome and FF):

• ​http://ckeditor4.t/dt/core/focusManager/editor.html
• ​http://ckeditor4.t/dt/core/focusManager/focus.html
• ​http://ckeditor4.t/dt/plugins/font/font.html

[Garry Yao]

Due to the above failures, I've proposed another approach to fix the context menu which doesn't requires remaining editor focus. Force pushed t/9212 for review.

[Piotrek Koszuliński]

Failing on Chrome: ​http://ckeditor4.t/dt/plugins/table/table.html

I'm also not sure if implementing new API just to satisfy this need is a good way.

PS. There's t/9212@tests.

[Garry Yao]

Ok, considering the change is getting bigger, opened #9449 for the context menu issue.

[Piotrek Koszuliński]

So I'm putting original part of the branch on review.

[Piotrek Koszuliński]

Masterised with ​https://github.com/ckeditor/ckeditor-dev/commit/87395e6