Ticket #2056 (closed Bug: fixed)

Opened 7 years ago

Last modified 4 years ago

fckdialog.html is invalid xhtml

Reported by: zagy Owned by: alfonsoml
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

validhtml.patch (70.0 KB) - added by zagy 7 years ago.
Patch for valid html
2056.patch (3.9 KB) - added by alfonsoml 6 years ago.
Proposed SVN patch
2056_2.patch (3.4 KB) - added by alfonsoml 6 years ago.
Proposed SVN patch
2056_3.2.patch (3.8 KB) - added by alfonsoml 6 years ago.
updated patch
2056_4.patch (4.6 KB) - added by alfonsoml 6 years ago.
Revised patch

Change History

comment:1 Changed 7 years ago by w.olchawa

  • Keywords Confirmed added

comment:2 Changed 7 years ago by zagy

BTW: would you like a patch for this?

Changed 7 years ago by zagy

Patch for valid html

comment:3 Changed 7 years ago by zagy

Hi,

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

Changed 6 years ago by alfonsoml

Proposed SVN patch

Changed 6 years ago by alfonsoml

Proposed SVN patch

comment:4 follow-up: ↓ 5 Changed 6 years ago by alfonsoml

  • Owner set to alfonsoml
  • Keywords Review? added

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 ; follow-up: ↓ 7 Changed 6 years ago by 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.

comment:6 Changed 6 years ago by 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.

Changed 6 years ago by alfonsoml

updated patch

comment:7 in reply to: ↑ 5 ; follow-up: ↓ 8 Changed 6 years ago by 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?

comment:8 in reply to: ↑ 7 Changed 6 years ago by 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.

  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 6 years ago by fredck

  • 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 6 years ago by 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).

comment:11 Changed 6 years ago by fredck

  • Milestone set to 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 6 years ago by alfonsoml

Revised patch

comment:12 Changed 6 years ago by alfonsoml

  • 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 6 years ago by fredck

  • Keywords Review+ added; Review? removed

comment:14 Changed 6 years ago by alfonsoml

  • Status changed from new to closed
  • Resolution set to fixed

Fixed with [2026]

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

comment:15 Changed 6 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 6 years ago by testme7

  • Status changed from closed to reopened
  • Resolution fixed deleted

comment:17 Changed 6 years ago by fredck

  • Status changed from reopened to closed
  • Resolution set to fixed

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 – 2012 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy