Opened 9 years ago

Closed 9 years ago

Last modified 7 years ago

#2056 closed Bug (fixed)

fckdialog.html is invalid xhtml

Reported by: Christian Zagrodnick Owned by: Alfonso Martínez de Lizarrondo
Priority: Normal Milestone: FCKeditor 2.6.1
Component: General Version: FCKeditor 2.6 Beta
Keywords: Confirmed Review+ Cc:

Description

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.

Attachments (5)

validhtml.patch (70.0 KB) - added by Christian Zagrodnick 9 years ago.
Patch for valid html
2056.patch (3.9 KB) - added by Alfonso Martínez de Lizarrondo 9 years ago.
Proposed SVN patch
2056_2.patch (3.4 KB) - added by Alfonso Martínez de Lizarrondo 9 years ago.
Proposed SVN patch
2056_3.2.patch (3.8 KB) - added by Alfonso Martínez de Lizarrondo 9 years ago.
updated patch
2056_4.patch (4.6 KB) - added by Alfonso Martínez de Lizarrondo 9 years ago.
Revised patch

Download all attachments as: .zip

Change History (22)

comment:1 Changed 9 years ago by Wojciech Olchawa

Keywords: Confirmed added

comment:2 Changed 9 years ago by Christian Zagrodnick

BTW: would you like a patch for this?

Changed 9 years ago by Christian Zagrodnick

Attachment: validhtml.patch added

Patch for valid html

comment:3 Changed 9 years ago by Christian Zagrodnick

Hi,

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

Changed 9 years ago by Alfonso Martínez de Lizarrondo

Attachment: 2056.patch added

Proposed SVN patch

Changed 9 years ago by Alfonso Martínez de Lizarrondo

Attachment: 2056_2.patch added

Proposed SVN patch

comment:4 Changed 9 years ago by Alfonso Martínez de Lizarrondo

Keywords: Review? added
Owner: set to Alfonso Martínez de Lizarrondo

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.

comment:5 in reply to:  4 ; Changed 9 years ago by Christian Zagrodnick

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.

comment:6 Changed 9 years ago by Alfonso Martínez de Lizarrondo

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.

Changed 9 years ago by Alfonso Martínez de Lizarrondo

Attachment: 2056_3.2.patch added

updated patch

comment:7 in reply to:  5 ; Changed 9 years ago by Martin Kou

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?

comment:8 in reply to:  7 Changed 9 years ago by Alfonso Martínez de Lizarrondo

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.

  1. 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.

comment:9 Changed 9 years ago by Frederico Caldeira Knabben

Keywords: Review- added; Review? removed

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.

comment:10 Changed 9 years ago by Christian Zagrodnick

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

comment:11 Changed 9 years ago by Frederico Caldeira Knabben

Milestone: FCKeditor 2.6.1

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.

Changed 9 years ago by Alfonso Martínez de Lizarrondo

Attachment: 2056_4.patch added

Revised patch

comment:12 Changed 9 years ago by Alfonso Martínez de Lizarrondo

Keywords: Review? added; Review- removed

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.

comment:13 Changed 9 years ago by Frederico Caldeira Knabben

Keywords: Review+ added; Review? removed

comment:14 Changed 9 years ago by Alfonso Martínez de Lizarrondo

Resolution: fixed
Status: newclosed

Fixed with [2026]

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

comment:15 Changed 9 years ago by paftek

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

comment:16 Changed 9 years ago by testme7

Resolution: fixed
Status: closedreopened

comment:17 Changed 9 years ago by Frederico Caldeira Knabben

Resolution: fixed
Status: reopenedclosed

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

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