Opened 7 years ago

Closed 7 years ago

#9212 closed Bug (fixed)

Placeholder plugin broken

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

Description

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.

Change History (14)

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

Milestone: CKEditor 4.0
Status: newconfirmed

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

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

Owner: set to Piotrek Koszuliński
Status: confirmedassigned

comment:3 Changed 7 years ago by 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 :)

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

comment:5 Changed 7 years ago by 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

comment:6 Changed 7 years ago by Garry Yao

Owner: changed from Piotrek Koszuliński to Garry Yao

comment:7 Changed 7 years ago by Garry Yao

Status: assignedreview

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

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

Status: reviewreview_failed

comment:9 Changed 7 years ago by Garry Yao

Status: review_failedreview

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.

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

Status: reviewreview_failed

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.

comment:11 Changed 7 years ago by Garry Yao

Status: review_failedreview

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

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

Owner: changed from Garry Yao to Piotrek Koszuliński

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

comment:13 Changed 7 years ago by Garry Yao

Status: reviewreview_passed

comment:14 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