Opened 4 years ago

Closed 4 years ago

#13232 closed Bug (fixed)

AppendText method does not append text in Safari using a Mac

Reported by: Chris Owned by: Piotrek Koszuliński
Priority: Normal Milestone: CKEditor 4.4.8
Component: General Version:
Keywords: Cc:

Description

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.

Change History (10)

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

Milestone: CKEditor 4.4.8
Status: newconfirmed
Version: 4.4.6

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 ) );.

comment:2 Changed 4 years ago by Chris

Ok, I'll give that a try

comment:3 Changed 4 years ago by Chris

That worked, thanks!

comment:4 Changed 4 years ago by Chris

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

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

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

Owner: set to Piotrek Koszuliński
Status: confirmedreview

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.

comment:7 Changed 4 years ago by Artur Delura

Status: reviewreview_failed
  • 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?

comment:8 Changed 4 years ago by Artur Delura

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

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

Status: review_failedreview

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.

comment:10 Changed 4 years ago by Artur Delura

Resolution: fixed
Status: reviewclosed

Cool. Fixed with commit.

Note: See TracTickets for help on using tickets.
© 2003 – 2017 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy