Opened 11 years ago

Closed 11 years ago

Last modified 7 years ago

#8363 closed defect (fixed)

[patch][ccla] IE: Malformed/duplicate HTML generated by RichText.getValue() in certain cases.

Reported by: Jared Jurkiewicz Owned by: liucougar
Priority: high Milestone: 1.3
Component: Editor Version: 1.1.1
Keywords: Cc:
Blocked By: Blocking:

Description (last modified by Adam Peller)

IE: Malformed/duplicate HTML generated by RichText?.getValue() in certain cases.

This problem was identified by a large product using editor. What they encountered was in certain cut/paste cases into Editor that on IE when they called getValue(), they would get duplicate tags/corrupted HTML.

They generated a testcase that reproduced the behavior, which was actually using malformed HTML as the input. While that was questionable, it did uncover what the bug in IE was that was causing the problem.

The problem is that IE when it handles certain malformed HTML input (via cut/paste, bad user input, whatever), it would actually construct a malformed HTML DOM in memory. This DOM was no longer a Tree, but a *graph*. Meaning, a node somehow would end up with multiple parents. In other words, say you have nodes:

A, B, C.

You could end up with node A being in both the childNode lists of B and C. This is fundamentally wrong with how DOM should be generated, but such is what it is, and it is IE, so is anyone surprised?

The fix for this turned out to be surprisingly simple. The fix is in:

dijit/_editor/html.js and is a one line change to: dijit._editor.getChildrenHtml. All you have to do is when iterating over the childNodes array is to check that the parentNode of a child node is actually declared as the node that the childNode list came from, or was null. That breaks the graph and allows generation of HTML based on what is the parent/child relationship (and avoids traversing the same node multiple times).

The product tested this fix and verified that it does indeed fix the issues with duplicates they were seeing.

So, effectively, this fix is a workaround for an ugly IE bug when it parses certain types of malformed HTML. Since we cannot control what users input and what cut/paste will try to insert, this is a reasonable way to get the editor code to at least correct for IE's braindeadedness here.

Attachments (6)

test_Editor_malformedHTMLIE.html (4.2 KB) - added by Jared Jurkiewicz 11 years ago.
Simple test that shows the duplications that occur with IE and malformed input when getValue() is called. Put in dijit/tests
dijit._editor.html_IEPATCH_20090106.patch (890 bytes) - added by Jared Jurkiewicz 11 years ago.
Simple patch for IE.
dijit._editor.html_IEPATCH_20090108.patch (868 bytes) - added by Jared Jurkiewicz 11 years ago.
Patch without the paranoia check for unowned node. Testcase still works.
dijit._editor.html_IEPATCH_20090115.patch (1.3 KB) - added by Jared Jurkiewicz 11 years ago.
Another swag at a patch that takes into account a comment by liucougar.
test_Editor_malformedHTMLIE2.html (4.3 KB) - added by Jared Jurkiewicz 11 years ago.
Updated testcase
dijit._editor.html_IEPATCH_20090115-2.patch (1.3 KB) - added by Jared Jurkiewicz 11 years ago.

Download all attachments as: .zip

Change History (15)

comment:1 Changed 11 years ago by Jared Jurkiewicz

Component: GeneralEditor
Owner: changed from anonymous to liucougar
Version: 1.2.31.1.1

I will be attaching a testcase and the patch shortly.

Changed 11 years ago by Jared Jurkiewicz

Simple test that shows the duplications that occur with IE and malformed input when getValue() is called. Put in dijit/tests

Changed 11 years ago by Jared Jurkiewicz

Simple patch for IE.

comment:2 Changed 11 years ago by Adam Peller

Description: modified (diff)
Milestone: tbd1.3
Summary: IE: Malformed/duplicate HMTL generated by RichText.getValue() in certain cases.IE: Malformed/duplicate HTML generated by RichText.getValue() in certain cases.

comment:3 Changed 11 years ago by liucougar

why you need to check for !node.parentNode?

comment:4 Changed 11 years ago by Jared Jurkiewicz

Paranoia check since IE seems to be extremely braindead. Unowned node, basically. Probably isn't needed.

Changed 11 years ago by Jared Jurkiewicz

Patch without the paranoia check for unowned node. Testcase still works.

comment:5 Changed 11 years ago by Jared Jurkiewicz

Attached a patch without that check too and the testcase still works fine..

And I also remember that I was worried what if they were passed the root node of a document or whatnot, one that didn't have a parent. Probably not something worth worrying about.

comment:6 Changed 11 years ago by Jared Jurkiewicz

I should note that IE, with the malformed input would *render* it correctly on a paste in, but when getValue walked it to figure out the HTML code, it would produce bad HTML output. So, it appears IE's rendering engine already does some sort of checking and ignoring or graphed nodes when it does a render. So what this fix does, it basically make our editor a bit more capable in dealing with bad IE structures by trying to ignore the nodes IE ignored when rendering. In essense, this patch helps the editor reconstitute HTML that matches what IE would render in those cases.

comment:7 Changed 11 years ago by Adam Peller

Priority: normalhigh
Summary: IE: Malformed/duplicate HTML generated by RichText.getValue() in certain cases.[patch][ccla] IE: Malformed/duplicate HTML generated by RichText.getValue() in certain cases.

Changed 11 years ago by Jared Jurkiewicz

Another swag at a patch that takes into account a comment by liucougar.

Changed 11 years ago by Jared Jurkiewicz

Updated testcase

Changed 11 years ago by Jared Jurkiewicz

comment:8 Changed 11 years ago by Jared Jurkiewicz

Resolution: fixed
Status: newclosed

(In [16340]) Minor tweak to make IE behave more sanely for odd input. fixes #8363

comment:9 Changed 7 years ago by bill

In [28191]:

Test case for #8363, plus some more AMD cleanup, refs #8363.

Note: See TracTickets for help on using tickets.