AppendText method does not append text in Safari using a Mac

[Chris]

Element.appendText() method does not append text to an <a> element in Safari using a Mac.

Environment:

Mac OS X 10.9.1

Safari 7.0.1

Steps to reproduce:

1) Create a new CKEDITOR.dom.element with ('a',editor.document) as params.

2) Attempt to appendText to the element.

3) Try to insert the element to the editor.

Code snippet:

var element = new CKEDITOR.dom.element('a', editor.document);
element.appendText('@');
editor.insertElement(element);

If you were to put a break point after the append, you would see that the text have not been added to the element.

[Piotrek Koszuliński]

It does not work because for some reason on Safari elA.text is an empty string, but appending to it doesn't change anything. Surprise, surprise, it doesn't happen with other element names I tested :D.

The second observation is that we do not use appendText in a single place, so I don't know why it has been added. But it makes the solution trivial - we should keep just the simple this.append( new CKEDITOR.dom.text( text ) );.

[Chris]

Ok, I'll give that a try

[Chris]

That worked, thanks!

[Chris]

Is appendText being deprecated, or will there be plans to fix this issue?

[Piotrek Koszuliński]

Since it exists in the API for a long time and it's a one-liner we'll keep it, even though it's not used by the core plugins. I assigned this ticket to 4.4.8, so it will be fixed in the next minor release (few weeks from now).

[Piotrek Koszuliński]

Turns out there was a reason for this method to use the text property - on IE8 it wasn't possible to append text node to a script tag. Fortunately our tests caught this :)

I pushed ​branch:t/13232.

[Artur Delura]

• Tests are stored in the same commit as solution.
• Also there is no manual test for this (but I think it's not necessary)
• How about checking this.$.text != null && this.$.text != '' instead of browser detection.
• How about adding @since tag to method. To make it officially publish since 4.4.8?

[Artur Delura]

Forgot to mention that I rebased branch to be on the newest master.

[Piotrek Koszuliński]

Tests are stored in the same commit as solution.

And that's totally fine. They go together with the thing they fix. The simplest solution.

Also there is no manual test for this (but I think it's not necessary)

Absolutely unnecessary. This can be fully testes automatically.

How about checking this.$.text != null && this.$.text != instead of browser detection.

This is not going to work. If you use such condition then on IE8 this method will try to append node to an empty script tag (for which text == '').

How about adding @since tag to method. To make it officially publish since 4.4.8?

That would be incorrect. This method does not exist since 4.4.8. Lacking documentation does not mean that we can tell untruth.

[Artur Delura]

Cool. Fixed with ​commit.