9779 - {dom1}Element::normalize() not supported

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect, P2)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Until bug 9777 and bug 9778 are fixed, use
http://www.fas.harvard.edu/~dbaron/dom/test/one-core-xml/Text_mozilla
instead of http://www.fas.harvard.edu/~dbaron/dom/test/one-core-xml/Text

Element::normalize() doesn't seem to be supported.  See the above test page.
The tricky issue is what to do to the variables holding references to the
existing text nodes when you normalize().

I did check that setting all the variables to null before the normalize didn't
fix the problem.

Observed: last line is red after hitting test button
Expected: should be green.  I'm not sure about the two before it.

Comment 1

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Do we need this for beta1?  It sounds like there's a correctness/spec issue to
decide, in addition to the actual normalize() implmentation.  I'd like to punt
to M15.

Comment 2

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

HTML DOM bugs are M11/P2 for Vidur.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Things aren't quite as I thought.  Normalize is doing a little bit, and it's
written, but it's not working completely.  It's also acting on CDATA sections
even though it shouldn't be.  The code is at:

http://lxr.mozilla.org/seamonkey/source/layout/base/src/nsGenericElement.cpp#583

Test case demonstrating some stuff to be attached.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: test case showing current behavior

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch for this bug (work in progress)

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

This patch fixes the main problem with normalize by inserting the missing
"index--".  Previously, normalize() was only combining pairs of text nodes -
that is, if there were 4 text nodes in sequence, it would stick the second onto
the first, and stick the fourth onto the third, but it wouldn't join the two
pairs.

It was supposed to also fix the problem with normalize() affecting CDATASections
(which was most of the work).  However, that part didn't work.  See the test
cases of bug 9781:
http://www.fas.harvard.edu/~dbaron/dom/test/one-core-xml/CDATASection
http://www.fas.harvard.edu/~dbaron/dom/test/one-core-xml/CDATASection_2

Are CDATA sections returning the wrong NodeType?  Or is there something wrong
with my patch?

(Note:  I'm not completely sure what I'm doing, so check this patch carefully!)

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I just realized (while walking to the library) that the problem might not be as

I thought.  It could be that the text manipulation functions are turning the

CDATA nodes into text nodes before the test script even gets to

pelem.normalize().  I can investigate this easily by adding a bunch of lines to

my test page -- I'll do this when I get home (probably this evening).

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Yup... the problem is in splitText().  When splitText() is called on a
CDATASection, the new node split off is a text node.  I *think* it should be a
CDATASection.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: very slightly revised fix for normalize (doesn't fix splitText)

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Should I split the splitText() issues into a separate bug?

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I split off the splitText() problem into bug 17726, since it's really separate.

Comment 12

[ekrock's old account (dead)]

Moving to M12.  Dang, David, now you fix the C source too?! Awesome!

Comment 13

[vidur (gone)]

Moving off M12 radar for the time being. One or more might get back once I get a
chance to really look at them.

Comment 14

[jst]

David, your patch + the patch for SplitText() I just created (see bug 17726)
seems to make mozilla pass your CDATA tests, could you verify this?

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Intuitively, it should.  I don't have a current tree right now, so it's probably
a bit of a paint to test...

Comment 16

[vidur (gone)]

In an attempt to get my bug list in order again, marking all the bugs I have
currently as ASSIGNED.

Comment 17

[vidur (gone)]

Thanks for the patch David. I made some minor tweaks to it - used nsCOMPtrs and
got rid of the ENTITY_REFERENCE_NODE case (normalize() should just pass over
entity references). I'll check this in when the tree opens for M13.

Note that the DOM spec doesn't mandate what should happen to existing text nodes
after normalization. With that in mind, it's fine for us to reuse text nodes if
we need to. Furthermore, we can't invalidate existing references to text nodes,
even if they aren't used. So the two tests preceded by the line "I'm not sure
about these next two" are invalid.

Thanks again!

Comment 18

[vidur (gone)]

Fixed on 12/21/1999.

Comment 19

[ckritzer (gone)]

Marking VERIFIED FIXED on:
- Linux6 2000-03-07-09 Commercial build
- MacOS9 2000-03-07-08 Commercial build
- Win98 2000-03-07-09 Commercial build