fckdialog.html is invalid xhtml

[zagy]

The fckdialog.html file has an XHTML doctype but doesn't validate:

http://validator.w3.org/check?uri=http%3A%2F%2Fwww.fckeditor.net%2Ffckeditor%2Feditor%2Ffckdialog.html&charset=%28detect+automatically%29&doctype=Inline&group=0

This is mainly because there are a lot of & < and > in the inline javascript code. The easiest would be to move the javascript code to a javascript file i guess.

This is a real problem when integrating with zope where the .html files might be parsed before delivery. The parsing breaks because of the invalid html.

[zagy]

BTW: would you like a patch for this?

[zagy]

Patch for valid html

[zagy]

Hi,

the patch is against trunk r1942. I'm not sure if I got all broken pieces but anyway it's a start.

[alfonsoml]

Proposed SVN patch

[alfonsoml]

Proposed SVN patch

[alfonsoml]

That patch sounds too aggressive (and it seems to have some error as it was escaping some html inside javascript)

The proposed patch probably should be enough, as the dialogs mixing html and javascript did work previously. At the same time it does fix a possible problem in the link dialog as it wasn't using the same system to generate the upload iframe.

[zagy]

Replying to alfonsoml:

That patch sounds too aggressive (and it seems to have some error as it was escaping some html inside javascript) The proposed patch probably should be enough, as the dialogs mixing html and javascript did work previously. At the same time it does fix a possible problem in the link dialog as it wasn't using the same system to generate the upload iframe.

They might "work" if the parser is lazy enough to let the invalid things pass. Zope doesn't do that which micht not be of your concern. But the document has an XHTML *strict* document type and doesn't validate. That is not just because of closing </foo> notes in HTML/Javascript/document.write foo. Have you had a look at the URL above? The first thing which actually breaks is

Line 90, Column 25: character "&" is the first character of a delimiter but occurred as data.

if ( FCKBrowserInfo.IsIE && !FCKBrowserInfo.IsIE7 )

Unquoted & in XML is just a syntax error. What in fact might help is adding XML comments inside the <script> tags to avoid parsing. But I don't really see a reason to not put JS into JS files.

[alfonsoml]

That kind of code did exist in the previous versions, and there was no reports about problems with Zope.

There are two differences between the previous versions and the current SVN: those </iframe> that aren't escaped, and the doctype that previously was HTML.

The previous patch did address the first difference, I'm gonna attach a patch to put the javascript of fckdialog.html inside a CDATA comment, but it doesn't require to change all the dialogs and splitting them in two files. The most important reason to avoid it is performance: two files mean two request to the server, and being a javascript file, all the processing will wait until the .js file is downloaded.

[alfonsoml]

updated patch

[martinkou]

Replying to zagy:

Unquoted & in XML is just a syntax error. What in fact might help is adding XML comments inside the <script> tags to avoid parsing. But I don't really see a reason to not put JS into JS files.

I concur with Alfonso's reply above but I'd like to also add a little more information about this issue. If we split the HTML and JavaScript into two files, then not only do we have to load two separate files, we also have to load them serially. That is, the browser would have to read the HTML file first (which takes 1 TCP roundtrip at least), understand it, and read the JavaScript file after that (another TCP roundtrip). TCP roundtrips are very expensive in terms of latency. So if we don't work hard to minimize that in FCKeditor, it would load very slowly in foreign websites despite the fact that you may have a very high throughput with that site. Therefore, we've always been trying not to split files apart for as long as we can manage to.

Back to the patch, there are still some things I don't really understand...

1. Is there a reason behind escaping the "/" in the </iframe> strings? I don't know if I understand this correctly... But for fckdialog.html, the "<![CDATA[" tag should have taken care of the problem, right? The other .html files are not declared to be XHTML compliant anyway.
2. Trying to validate the patched fckdialog.html still gives me one error - the fckLang attribute is not defined in the XHTML transistional schema. Maybe we can put the offending lines to document.write() to make the validator happy?

[alfonsoml]

Replying to martinkou:

Back to the patch, there are still some things I don't really understand... 1. Is there a reason behind escaping the "/" in the </iframe> strings? I don't know if I understand this correctly... But for fckdialog.html, the "<![CDATA[" tag should have taken care of the problem, right? The other .html files are not declared to be XHTML compliant anyway.

I don't think that it's needed in the CDATA section, but I don't think that it does any harm. The escaping in the other files is neccesary (even in HTML pages that code will be flagged as an error). You can see the code also as the ; at the end of javascript lines. It isn't always neccesary, but if later someone tries to package all the content and you haven't put the ; then the code won't work.

2. Trying to validate the patched fckdialog.html still gives me one error - the fckLang attribute is not defined in the XHTML transistional schema. Maybe we can put the offending lines to document.write() to make the validator happy?

I don't think that this change is needed in order to make the page work in zope. Maybe this is a something to think about for CKEditor 3 and the new system, but changing only those two buttons and leaving the rest of the dialogs the same way (and even just using js to hide the fact that we are adding a custom property) seems like cheating to me.

[fredck]

Let me add my bits to this.

Firstly, I don't care about the W3C validator right now. We have never used CDATA before for script blocks, and I think this is not the time to start using them.

All we need to have is to be sure it will work with Zope. And for that, as we have always done before, it should be enough to escape the closing tags, as did Alfonso. So, the fix should include that thing only.

---

I don't really like the current escaping method. I thing we should standardize it, so whenever we write a tag in a js block, we use the same syntax. What about the following?

'<' + 'iframe>' + otherStuff + '<' + '/iframe>'

It would always isolate the "<" char form character sequences or the slash.

[zagy]

No the fix is not enough. It doesn't really have to be valid XHTML, agreed. But it really has to be valid XML. You still cannot put it into an XML parser because *every* <> and & must be escaped by entities (apart from <>& in comments or cdata).

[fredck]

Ok, my fault. My comment about CDATA was not considering that all other pages have the HTML4 DOCTYPE. So, the CDATA addition seems to be valid, and the patch should be correct.

I still leave the Review- just because I think we must also be consistent. At this point, all three <script> blocks available in the file must have the CDATA tags.

[alfonsoml]

Revised patch

[alfonsoml]

I've added the CDATA to all the scripts of fckdialog.

I think that using <\/tag> is the usual way of escaping html inside javascript, so I have leave those that way.

[alfonsoml]

Fixed with [2026]

Reopen if there are still problems with Zope after checking the next nightly builds.

[paftek]

Problem still present in "editor/dialog/fck_paste.html" line 67 : </iframe> have to be replaced by <\/iframe>

[fredck]

This ticket should not be reopened for it, as it has already been fixed and released. A dedicated ticket is needed (#2327).