Opened 12 years ago
Closed 12 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:
- Open samples/placeholder.html
- Add new paragraph.
- Click on [P] icon in toolbar.
- Enter placeholder text (I used test) and click on OK.
- 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 12 years ago by
Milestone: | → CKEditor 4.0 |
---|---|
Status: | new → confirmed |
comment:2 Changed 12 years ago by
Owner: | set to Piotrek Koszuliński |
---|---|
Status: | confirmed → assigned |
comment:3 Changed 12 years ago by
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);
- Open placeholder sample.
- Run this code.
- At the beginning
'This is a '
is logged. - Right click on placeholder, so context menu opens.
- Now
'[[sample placeholder]]'
is logged. - Click placeholder option in context menu.
- 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 12 years ago by
Bisect for the rescue - this is first bad commit: https://github.com/ckeditor/ckeditor-dev/commit/44f81a772cbd5fa59acb27579ea8d7178fbd924b
comment:5 Changed 12 years ago by
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 12 years ago by
Owner: | changed from Piotrek Koszuliński to Garry Yao |
---|
comment:7 Changed 12 years ago by
Status: | assigned → review |
---|
On top of the existing branch, I've the last issue mentioned by Piotrek in comment:3, for Webkit.
comment:8 Changed 12 years ago by
Status: | review → review_failed |
---|
I pushed rebased t/9212.
Last commit breaks these tests (at least on Chrome and FF):
comment:9 Changed 12 years ago by
Status: | review_failed → review |
---|
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 12 years ago by
Status: | review → review_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 12 years ago by
Status: | review_failed → review |
---|
Ok, considering the change is getting bigger, opened #9449 for the context menu issue.
comment:12 Changed 12 years ago by
Owner: | changed from Garry Yao to Piotrek Koszuliński |
---|
So I'm putting original part of the branch on review.
comment:13 Changed 12 years ago by
Status: | review → review_passed |
---|
comment:14 Changed 12 years ago by
Resolution: | → fixed |
---|---|
Status: | review_passed → closed |
Masterised with https://github.com/ckeditor/ckeditor-dev/commit/87395e6
This plugin is completely broken. Steps to reproduce: load placeholder sample, double click, see how badly it presents itself on the console.