Opened 10 years ago

Closed 10 years ago

#2916 closed Task (fixed)

Implement Flash dialog.

Reported by: Martin Kou Owned by: Martin Kou
Priority: Normal Milestone: CKEditor 3.0
Component: UI : Dialogs Version: SVN (FCKeditor) - Retired
Keywords: Review+ Cc:

Description

Need to port the Flash dialog from v2 to v3.

Attachments (2)

2916.patch (34.5 KB) - added by Martin Kou 10 years ago.
2916_2.patch (35.5 KB) - added by Martin Kou 10 years ago.

Download all attachments as: .zip

Change History (9)

comment:1 Changed 10 years ago by Martin Kou

Status: newassigned

Changed 10 years ago by Martin Kou

Attachment: 2916.patch added

comment:2 Changed 10 years ago by Martin Kou

Keywords: Review? added

comment:3 Changed 10 years ago by Frederico Caldeira Knabben

Keywords: Review- added; Review? removed

Core stuff. I'm a bit stricter here:

  • Please rename the "prefix" parameter to "namespace" in element::getElementsByTag.
  • scopeName in IE will always return 'HTML' uppercased. So, the toLowerCase overhead is unnecessary.
  • Let's simplify the code a bit in writeHtml, having the fragment.js code ignoring any <?xml:namespace> tag.
  • Hey Mr. Programmer... please change the "x must be an integer" messages to "x must be a number" :)

I'll not go that deep on the dialog code. I'll be focused on checking the dialog usage instead.

  • When opening a valid flash object, the preview box remains blank. Actually, it looks like it doesn't work at all, as I was not able to see that in any case.

Looks good for the rest.

Changed 10 years ago by Martin Kou

Attachment: 2916_2.patch added

comment:4 Changed 10 years ago by Martin Kou

Keywords: Review? added; Review- removed

comment:5 Changed 10 years ago by Martin Kou

The toLowerCase() function after scopeName is still there in the new patch. The reason being for namespaces other than HTML, IE would still always return in uppercase. Our source code has been assuming everything related to tags and namespaces are in lowercase, however. So I've kept the toLowerCase() there.

comment:6 in reply to:  5 Changed 10 years ago by Frederico Caldeira Knabben

Keywords: Review+ added; Review? removed

Replying to martinkou:

The toLowerCase() function after scopeName is still there in the new patch. The reason being for namespaces other than HTML, IE would still always return in uppercase. Our source code has been assuming everything related to tags and namespaces are in lowercase, however. So I've kept the toLowerCase() there.

Good one... so, let's make the if comparison in uppercase, and move the toLowerCase function right after it, having:

var scopeName = this.$.scopeName;
if ( scopeName != 'HTML' )
	nodeName = scopeName.toLowerCase() + ':' + nodeName;

The getName function is a critical feature for our code.

comment:7 Changed 10 years ago by Martin Kou

Resolution: fixed
Status: assignedclosed

Fixed with [3110].

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