Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jdelaune’s picture

Title: data.node object on present in TinyMCE » data.node object only present in TinyMCE

fixed typo

TwoD’s picture

Status: Active » Postponed (maintainer needs more info)

See fckeditor-2.6.js line 101-103 and ckeditor-3.0.js line 152-154.

Is something wrong with that?

The data object isn't perfect as it's not easy to get all editors to agree exactly on what is selected, but we try our best...

jdelaune’s picture

Status: Needs review » Postponed (maintainer needs more info)

Ok thanks for the clarification.

In ckeditor-3.0.js if you change line 152 to:

var data = { format: 'html', node: editor.getSelection().getSelectedElement() ? editor.getSelection().getSelectedElement().$ : editor.getSelection().getSelectedElement() };

It returns the same as tinymce-3.js. Tested in Safari 4 and Firefox 3.5 on Mac OS 10.6.

I'll look int FCKeditor a bit more...

jdelaune’s picture

Status: Postponed (maintainer needs more info) » Needs review

And for fckeditor-2.6.js change line 101 to:

var data = { format: 'html', node: instance.FCKSelection.GetSelectedElement() };

Tested in Firefox 3.5 on Mac OS 10.6, couldn't test in Safari due to this bug #497654: Drupal plugin buttons disabled in WebKit based browsers.

TwoD’s picture

Status: Postponed (maintainer needs more info) » Needs review

Hmm, I don't remember why I went with GetParentElement() instead of GetSelectedElement() for FCKeditor. Might have had something to do with partially selected elements...

Anyway, I'll look into this as soon as I can. We could really need some automated tests for things like this...

For later versions, maybe even 3.x, I think we should wrap methods like these so there's a proper API method to *get* the selection instead of only being passed it, but that's another issue.

And yes, that Webkit bug is very annoying, not to mention a UX disaster. :(

TwoD’s picture

I got some time to look into this again and now I know why I used GetParentElement; GetSelectedElement returned null when the selection wasn't exactly one node, which doesn't match what TinyMCE does at all.
I should probably have done data.node = ...GetSelectedElement() || ...GetParentElement(); instead (both for isNode and invoke calls).
We need to look over data.content tho. In TinyMCE it looks like it's either the outer HTML of the selected node, or just the selected text, and i see no official API call for that in FCKeditor. This is assuming we model the data object after TinyMCE of course.

I'll look into this a bit more tonight and compare it too a few other editors as well.

TwoD’s picture

Status: Needs review » Needs work

Forgot the status.

EugenMayer’s picture

Well for me ( FF 3.5, Linux ) i have a data.node in FCKeditor, but it differs in the isNode method and in the invoke method. In both the data.node is set, but to different objects.
While the "isNode" check succeeds, the same check (exactly the same) fails in invoke. In invoke, the data.node object seems to be wrapped with a

<p>

So the same $(data.node).is('img') fails there, which worked in isNode(data) { $(data.node).is('img') }.

Thanks for all your effort on this outstanding module!

TwoD’s picture

That's because isNode gets the result from GetSelectedElement (which can sometimes be null) while invoke gets the result from GetParentElement. Fixing that is easy, getting data.content right for invoke is a bit more tricky since FCKeditor doesn't provide a method specifically for that, and it seems semi-broken up until a few versions ago. I have not had time to look into other editors yet.

EugenMayer’s picture

Well guys, beside that it is critical to provide a method in the WYSWYG api, which enable you to move the selection on a specific element in the editors DOM. This is very critical, as thats enabling you fixing browser and editor specific bugs regarding the selection of object.

If you agree i open a new feature request for that

TwoD’s picture

We're going to extend the API for 3.x, things like editor.getRange(), editor.getSelection(), editor.getSelectedElement() etc might be a part of that, we haven't discussed the details yet. I think the plans for 3.x sun posted on g.d.o mentions this.

sun’s picture

Status: Needs work » Active

AFAICS, only #6 contains something that could be remotely understood as a "patch" ;)

jdelaune’s picture

Well I'm happy to make my suggestions into patch however it doesn't sound like it will get implemented because of further implications and future plans.

It solved my issue but who knows it could break something else.

sun’s picture

3.x will still take some time. Could we try to combine #3, #4, and #6 into a patch, so we have something everyone can test?

EugenMayer’s picture

I would jump in as a tester, what patch-base are you gonna use. The current 2.x-dev branch or the stable 2.0 release?

sun’s picture

Patches and testing of patches is always based on CVS - in this case on the branch DRUPAL-6--2

sun’s picture

Title: data.node object only present in TinyMCE » data.node object not present in FCKeditor/CKEditor
Status: Active » Needs review
FileSize
2.06 KB
EugenMayer’s picture

Status: Needs review » Fixed

That one is solved in 6--2-1 already.

TwoD’s picture

Status: Fixed » Needs work

No it's not.

Marking #748888: isNode() is not called in CKEditor a duplicate of this issue, data.node can't be present if isNode isn't called.

I have a patch which makes TinyMCE, FCKeditor and CKEditor agree on when a single element is selected, and calls isNode in CKEditor to toggle the state of the command. The downsides are that TinyMCE's API method returns the common ancestor of multiple selected elements, while the others don't, and that FCK/CK does not have a way to retrieve the selected text snippet. I've changed my mind about using instance.FCKSelection.GetSelectedElement() || instance.FCKSelection.GetParentElement() for FCKeditor as it would always return a paragraph when the cursor is in it, which would mean we're telling plugins that this whole paragraph will be replaced by the contents they return, while in reality it would just be inserted somewhere inside that paragraph. CKeditor does not have an equivalent method either so data.node would be null there.

Using just getSelectedElement for both CKeditor and FCKeditor would leave TinyMCE as the odd case and plugins could be more certain the element sent to them is indeed the only selected element.

We do have another problem to consider, and that is Webkit based browsers. Both Safari and Crome have a major problem when it comes to selecting images (and probably other block elements) that were inserted using execCommand('inserthtml', ...). When selected, the elements are treated as part of a text-type selection, making it impossible to select just that element and get an element-type selection. To the user, this makes the selection have that familiar blueish overlay extending both left and right of the image though only the image was clicked. Trying to select the image using the keyboard is difficult to say the least, as the cursor sometimes jumps straight back to the start of the text.

This can be avoided by wrapping the image in a p-tag before inserting it - something each plugin would have to do unless they are just inserting text - or by manually splitting elements and placing actual DOM elements between them via the browser's selection range object - something we can't do today as we must strings from the plugins. Internally, the editors deal with this by having "insertHTML" and "insertElement" methods. "insertHTML" is often used for tasks which replace all of the editor contents, while "insertElement" is the rangebased element-splitting one used when dealing with selections.

We can actually get a reference to an image which has been inserted via a plugin when a user clicks on it, but then we have to do that by taking the first element from the selection, if getSelectedElement returns nothing. This is however not optimal as we'd be fooling plugins into thinking that only a single object has been selected, while in reality there may be more elements in the same range.

How to best deal with this now, I don't know. Maybe the best option is to use getSelected element in FCKeditor and CKEditor and if that returns NULL assume no single element has been selected unless we're in Webkit, in which case we'll take the first element from the range (if any)?
I don't think we want to deal with text-selections in Drupal plugins now. Only TinyMCE seems to support this via a simple method call (for the other editors, you need to deal with their abstractions of the selection/range objects).

Sorry for the wall of text. I've been pulling my hair out over this issue for weeks now and needed to get it out, though it became a bit more confusing than I had intended.

EugenMayer’s picture

isNode() is never called and as it is only returning a bolean and not setting any internal member, invoke MUST get a new object (which is computed elsewhere). Those 2 methods have nothing to do with eachother.

isNode() is used for marking the icon as "selected" if a node is selected, which i think in my isNode() implementation is "mine", like an image for wyswiyg_imageuplaod.

while invoke() actually starts the whole process.

And for the CKEditor isseu actually, isNode() is not called, while inoke() has a correct data.node object.

--

To that selection mess...iam completely with you. i have fixed that problems in the tinyMCE in the invoke method, which resulted in 1 zillion special cases which tend to randomly break in Browser XYZ :)

TwoD’s picture

I agree with you that isNode is just for determining when an element is "mine", but I'm not sure what you mean about invoke getting a new object?
Normally, isNode() is the one called first, when the user selects an element. If that element is "mine", the button is highlighted/activated to indicate this to the user. When the user clicks the buttons, the very same element should be passed to invoke() so I can potentially invoke my own isNode implementation to check if the element(s) the user wishes to replace/update is really "mine", instead of having to write one test to see when to activate the button, and another one to see if I can get attributes from the element and prepopulate my "edit/update" dialog.

Maybe this is what you meant, but I misunderstood your post.

EugenMayer’s picture

My post is about, that the other issue is _not_ a dublicate of this one. I wont reopen it - i let the decision up to you.

the data.node is correctly computed, in CKeditor, FCKeditor and TinyMCE editor. So when invoke is called, its the correct selected element BUT ....

isNode() is never called! That means, i dont have a chance to tell it, its "mine" :)

TwoD’s picture

Ok, considering the title and focus of this issue, and the IRC discussion we had, I'm reopening #748888: isNode() is not called in CKEditor but note that the node/element sent to it must be kept in sync with the one sent when this issue is fixed. isNode() for CKEditor will not be implemented as part of this issue.
(This issue still needs work though.)

EugenMayer’s picture

Status: Needs work » Needs review
diff --git editors/js/ckeditor-3.0.js editors/js/ckeditor-3.0.js
index ad695e6..8a4f715 100644
--- editors/js/ckeditor-3.0.js
+++ editors/js/ckeditor-3.0.js
@@ -152,9 +152,9 @@ Drupal.wysiwyg.editor.instance.ckeditor = {
         if (typeof Drupal.wysiwyg.plugins[pluginName].invoke == 'function') {
           var pluginCommand = {
             exec: function(editor) {
-              var data = { format: 'html', node: editor.getSelection().getSelectedElement() };
+              var data = { format: 'html', node: editor.getSelection().getStartElement() };
               // @todo This is NOT the same as data.node.
-              data.content = data.node ? data.node.innerHTML : '';
+              data.content = data.node ? data.node.getHtml() : '';
               Drupal.wysiwyg.plugins[pluginName].invoke(data, pluginSettings, editor.name);
             }
           };

Well thats my fix to the CKeditor. getSelectedElement seems not to work properly, innerHTML does not either. using getStartElement with getHtml() seems to work robust here.

EugenMayer’s picture

Status: Needs review » Needs work

Well this fix does not work out. It always selects the whole paragraph, so if we have

<p> Some fof _thats selected_ here some text </p>

and "_thats selected_" is selected, using my approach, the whole text would be part of the selection. Using the current implementation, no text would be in the selection.

EugenMayer’s picture

Finally

--- editors/js/ckeditor-3.0.js
+++ editors/js/ckeditor-3.0.js
@@ -152,9 +152,20 @@ Drupal.wysiwyg.editor.instance.ckeditor = {
         if (typeof Drupal.wysiwyg.plugins[pluginName].invoke == 'function') {
           var pluginCommand = {
             exec: function(editor) {
-              var data = { format: 'html', node: editor.getSelection().getSelectedElement() };
+              var sel = editor.getSelection();
+              var data = { format: 'html', node: sel.getSelectedElement() };
               // @todo This is NOT the same as data.node.
-              data.content = data.node ? data.node.innerHTML : '';
+              if(sel.getType() == CKEDITOR.SELECTION_TEXT) {
+                if (CKEDITOR.env.ie) {
+                  data.content = sel.getNative().createRange().text;
+                } else {

That worked for Firefox, Opera, Chrome and with the new sepecial case also for out love IE

EugenMayer’s picture

index 4f1f26d..4c18d1e 100644
--- editors/js/ckeditor-3.0.js
+++ editors/js/ckeditor-3.0.js
@@ -152,10 +152,21 @@ Drupal.wysiwyg.editor.instance.ckeditor = {
         if (typeof Drupal.wysiwyg.plugins[pluginName].invoke == 'function') {
           var pluginCommand = {
             exec: function(editor) {
-              var data = { format: 'html', node: editor.getSelection().getSelectedElement() };
-              // @todo This is NOT the same as data.node.
               var sel = editor.getSelection();
-              data.content = sel ? editor.getSelection().getNative().toString() : '';
+              var data = { format: 'html', node: sel.getSelectedElement() };
+              // @todo This is NOT the same as data.node.
+              if(sel.getType() == CKEDITOR.SELECTION_TEXT) {
+                if (CKEDITOR.env.ie) {
+                  data.content = sel.getNative().createRange().text;
+                } else {
+                  data.content = sel ? sel.getNative().toString() : '';
+                }
+              }
+              else {
+                data.content = sel ? data.node.innerHTML : '';
+              }
+
+              data.node = data.node ? data.node.$ : null;
               Drupal.wysiwyg.plugins[pluginName].invoke(data, pluginSettings, editor.name);
             }
           };

Well now we add the fix for data.node != data.node.$ and we should get this thing more robust in general

EugenMayer’s picture

Status: Needs work » Needs review

well works for me now, tested it in several cases.

sun’s picture

Status: Needs review » Needs work

mmm, no patch here to review or commit. I'm not sure what to make out of the last comments. Can someone roll the required changes as patch?

TwoD’s picture

Status: Needs work » Needs review
FileSize
1.54 KB

From #27 with some minor changes.
Works pretty good but data.content will be empty when say an image is selected because it's innerHTML is empty. The value of data.content will just be a textual representation in other cases though, and won't always match exactly what gets replaced, so I guess it doesn't matter much.

The data.node object will be correct.

Will try to deal with FCKeditor later.

sun’s picture

sun’s picture

So how about this?

TwoD’s picture

+++ editors/js/ckeditor-3.0.js	20 Dec 2010 00:31:21 -0000
@@ -166,10 +166,26 @@ Drupal.wysiwyg.editor.instance.ckeditor 
+              var selection = editor.getSelection();

Looks like getSelection() can return null, need to check for that before calling getType().

+++ editors/js/ckeditor-3.0.js	20 Dec 2010 00:31:21 -0000
@@ -166,10 +166,26 @@ Drupal.wysiwyg.editor.instance.ckeditor 
+                data.content = data.node.$.parentNode.innerHTML;

We could use

var parentNode = data.node.getParent();
data.content = parentNode ? parentNode.$.innerHTML : '';

to make sure there really is a parent node and it's of node type (not a document or text node), but that seems very unlikely and I've not been able to break it yet.

Tested modifications with HEAD but should apply to the others as well.

Powered by Dreditor.

sun’s picture

sun’s picture

Status: Needs review » Fixed

Thanks for reporting, reviewing, and testing! Committed to all branches.

A new development snapshot will be available within the next 12 hours. This improvement will be available in the next official release.

EugenMayer’s picture

Good work :)

TwoD’s picture

Version: 6.x-2.x-dev » 7.x-2.x-dev
Priority: Normal » Major
Status: Fixed » Needs review
FileSize
556 bytes

Last patch was broken, see #1004400: Teaser break doesn't work in CK Editor (data.node = null).
When there's a selection, but it doesn't match a node, selection.getSelectedElement() returns null.

Fix it by reintroducing the ternary operator?

sun’s picture

+++ editors/js/ckeditor-3.0.js
@@ -183,7 +183,7 @@ Drupal.wysiwyg.editor.instance.ckeditor = {
                   // content is supposed to contain the "outerHTML".
                   data.content = data.node.$.parentNode.innerHTML;
                 }
-                data.node = data.node.$;
+                data.node = data.node ? data.node.$ : null;

This does not catch the other data.node. Revised patch attached.

Powered by Dreditor.

TwoD’s picture

Right, should have added it to both places, but #38 never sets data.content for text selections. ;)

Not at home right now so I can't upload patches...
How about this?

            exec: function (editor) {
-             var data = { format: 'html' };
+             var data = { format: 'html', content: '' };
              var selection = editor.getSelection();
              if (selection) {
                data.node = selection.getSelectedElement();
+               data.node = data.node ? data.node.$ : null;
                if (selection.getType() == CKEDITOR.SELECTION_TEXT) {
                  if (CKEDITOR.env.ie) {
                    data.content = selection.getNative().createRange().text;
                  }
                  else {
                    data.content = selection.getNative().toString();
                  }
                }
-               else {
+               else if (data.node) {
                  // content is supposed to contain the "outerHTML".
-                 data.content = data.node.$.parentNode.innerHTML;
+                 data.content = data.node.parentNode.innerHTML;
                }
-               data.node = data.node.$;
              }
-             else {
-               data.content = '';
-             }

Gets the native node right from the start since we don't use CKEditor's abstraction for anything anyway.
Default data.content to an empty string so we won't have to explicitly set it wherever we don't find what we're looking for.
Also makes it more obvious that we only do the "outerHTML" thing if there's a node to do it on.

TwoD’s picture

The above as a proper patch. Also included an alternate version which doesn't use the ternary operator.
Both tested with HEAD.

sun’s picture

Status: Needs review » Fixed
FileSize
1.79 KB

Thanks for reporting, reviewing, and testing! Committed to attached patch.

A new development snapshot will be available within the next 12 hours. This improvement will be available in the next official release.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

EugenMayer’s picture

Status: Closed (fixed) » Needs review
FileSize
1006 bytes

I dont think this is suffently fixed and can be a bigger issue later on, so i would suggest it for 6.2.3

data.node = data.node.$

is only exectuted if an selection has been found. While this is fine for most cases, code checking for the value of data.node will have to always check if the property exists even. We will se a lot bugs arround this later on, so better setting data.node = null; if no selection available (important for stuff like isNode())

in addition i seem to have better results on "what is selected in using data.node

data.node.parentNode.innerHTML;

instead of data.node.$
In the current code, data.node = data.node.$ is executed before. Thats what i changed.

Patch attached

EugenMayer’s picture

Well after some debugging sessions, i cant really see a lot difference between my patched version and yours. It seems like the code is rather never exectuted before isNode .. so its completly useless anyway. Did somethign change in the CKEditor API maybe? to be more descriptive:

- exec is executed when you press any button ( to get the selection )
- exec is not executed for isNode ( or rather before ) making isNode pretty useless, as node is likely null in all cases

Is that behavior known / wanted?

sun’s picture

Status: Needs review » Closed (fixed)

@EugenMayer: Let's open a separate issue for the isNode/selectionChange event issue (and link to this issue from it). The code looks similar, but may or may not require a different resolution.

Reverting status.

EugenMayer’s picture

Thats fine with me sun, i think this issue i really a seperate one