Bug 17125 - Acid3 requires Text.replaceWholeText method from DOM Level 3
Summary: Acid3 requires Text.replaceWholeText method from DOM Level 3
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.5
: P2 Normal
Assignee: Nobody
URL: http://www.w3.org/TR/DOM-Level-3-Core...
Keywords:
Depends on: 11387
Blocks: Acid3
  Show dependency treegraph
 
Reported: 2008-01-31 16:41 PST by Eric Seidel (no email)
Modified: 2019-02-06 09:03 PST (History)
2 users (show)

See Also:


Attachments
First pass at wholeText/replaceWholeText support (12.41 KB, patch)
2008-02-03 22:11 PST, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Updated after Maciej's comments (18.57 KB, patch)
2008-02-05 23:28 PST, Eric Seidel (no email)
darin: review-
Details | Formatted Diff | Diff
Address Darin's concerns (delta from previous patch) (8.28 KB, patch)
2008-02-07 23:52 PST, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Address Darin's concerns (delta from previous patch) (8.29 KB, patch)
2008-02-08 00:05 PST, Eric Seidel (no email)
darin: review-
Details | Formatted Diff | Diff
Add wholeText/replaceWholeText support (final squashed patch, tests all pass) (19.19 KB, patch)
2008-02-08 00:26 PST, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Add wholeText/replaceWholeText support (final squashed patch, tests all pass) (22.05 KB, patch)
2008-02-08 00:53 PST, Eric Seidel (no email)
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2008-01-31 16:41:47 PST
Acid3 requires Text.wholeText method from DOM Level 3

Hyatt said that the :empty test depends on this (and once he lands his :empty) fix we'll start failing as a result.

Looks easy enough to implement.
Comment 1 Dave Hyatt 2008-01-31 18:45:52 PST
It actually requires .replaceWholeText.  I'd expect a patch to implement both methods.

Comment 2 Eric Seidel (no email) 2008-02-03 22:11:38 PST
Created attachment 18899 [details]
First pass at wholeText/replaceWholeText support

 LayoutTests/ChangeLog                              |   21 +++++
 .../core/textreplacewholetext01-expected.txt       |    3 +-
 .../core/textreplacewholetext02-expected.txt       |    3 +-
 .../core/textreplacewholetext03-expected.txt       |    3 +-
 .../core/textreplacewholetext04-expected.txt       |    3 +-
 .../core/textreplacewholetext05-expected.txt       |    3 +-
 .../core/textreplacewholetext06-expected.txt       |    3 +-
 .../core/textreplacewholetext07-expected.txt       |    3 +-
 .../xhtml/level3/core/textwholetext01-expected.txt |    3 +-
 .../xhtml/level3/core/textwholetext02-expected.txt |    3 +-
 .../xhtml/level3/core/textwholetext03-expected.txt |    3 +-
 .../fast/dom/Window/window-properties-expected.txt |    1 +
 WebCore/ChangeLog                                  |   17 ++++
 WebCore/dom/Node.cpp                               |    2 +-
 WebCore/dom/Text.cpp                               |   80 ++++++++++++++++++++
 WebCore/dom/Text.h                                 |    4 +
 WebCore/dom/Text.idl                               |    4 +
 17 files changed, 138 insertions(+), 21 deletions(-)
Comment 3 Eric Seidel (no email) 2008-02-04 08:04:09 PST
Comment on attachment 18899 [details]
First pass at wholeText/replaceWholeText support

Maciej has convinced me to improve handling of EntityReferences as part of this patch (and add some test cases to make sure we make them ReadOnly nodes).  I'm gone until Wed. night, at which point I'll look at finishing this.
Comment 4 Eric Seidel (no email) 2008-02-05 23:28:10 PST
Created attachment 18955 [details]
Updated after Maciej's comments

 LayoutTests/ChangeLog                              |   27 ++++++
 .../core/textreplacewholetext01-expected.txt       |    3 +-
 .../core/textreplacewholetext02-expected.txt       |    3 +-
 .../core/textreplacewholetext03-expected.txt       |    3 +-
 .../core/textreplacewholetext04-expected.txt       |    3 +-
 .../core/textreplacewholetext05-expected.txt       |    3 +-
 .../core/textreplacewholetext06-expected.txt       |    3 +-
 .../core/textreplacewholetext07-expected.txt       |    3 +-
 .../xhtml/level3/core/textwholetext01-expected.txt |    3 +-
 .../xhtml/level3/core/textwholetext02-expected.txt |    3 +-
 .../xhtml/level3/core/textwholetext03-expected.txt |    3 +-
 .../readonly-exceptions-expected.txt               |   23 +++++
 .../dom/EntityReference/readonly-exceptions.html   |   13 +++
 .../dom/EntityReference/resources/TEMPLATE.html    |   13 +++
 .../resources/readonly-exceptions.js               |   33 +++++++
 .../fast/dom/Window/window-properties-expected.txt |    1 +
 WebCore/ChangeLog                                  |   23 +++++
 WebCore/dom/Document.cpp                           |    7 ++-
 WebCore/dom/Node.cpp                               |    2 +-
 WebCore/dom/Text.cpp                               |   89 ++++++++++++++++++++
 WebCore/dom/Text.h                                 |    4 +
 WebCore/dom/Text.idl                               |    4 +
 22 files changed, 247 insertions(+), 22 deletions(-)
Comment 5 Darin Adler 2008-02-06 06:40:09 PST
Comment on attachment 18955 [details]
Updated after Maciej's comments

+        if (n->nodeType() == Node::ENTITY_REFERENCE_NODE) {
+            // We would need to visit EntityReference child text nodes if they existed
+            ASSERT(!n->hasChildNodes());
+            break;
+        }
+        if (!n->isTextNode())
+            break;

The code above is (slightly) unnecessarily inefficient. The code to handle EntityReference could simply be an assertion. And if not, and you want to retain the nodeType check, then it's better to not call both nodeType() and isTextNode(). If you have the node type, you can check if something's a text node without a second virtual function call.

Because DOM mutation events, specifically the DOMNodeRemovedEvent, can do arbitrary operations, the replaceWholeText function needs some work to be robust in the face of event handler functions that do strange things.

+    for (Node* n = startText; n != this;) {

This loop variable needs to be a RefPtr. The removeChild function could remove the last reference to the parent, so the node "n" could be deleted unless someone has ref'd it.

Similarly, the current node ("this") needs to go into a RefPtr before calling functions that can in turn invoke mutation event handlers.

+        parentNode()->removeChild(nodeToRemove, ignored);
+        ASSERT(!ignored);

This assertion is incorrect. For example, the mutation function could remove the node inside the function, which will result in a NOT_FOUND_ERR. So it may be OK to ignore the error, but it's not OK to assert there was no error.

+        Node* onePastEndText = endText->nextSibling();
+        for (Node* n = nextSibling(); n != onePastEndText;) {

Same thing here. Both onePastEndText and n need to be RefPtr.

+            parentNode()->removeChild(nodeToRemove, ignored);
+            ASSERT(!ignored);

And this assertion is also incorrect.

+    setData(newText, ignored);
+    ASSERT(!ignored);

To make this assertion correct, you'll need to reset "ignored" to 0 just before calling setData since the earlier functions could have changed ignored to a non-0 value. Or you could remove the assertion. If you removed all the assertions, then you could leave ignored uninitialized.

+    Text* replaceWholeText(const String&, ExceptionCode&);

The return value of the function needs to be a PassRefPtr. It's possible that mutation event handlers will remove the text node from tree. If they do, it's important that we don't return a deleted object. The usual way we fix this elsewhere is to use PassRefPtr even though in the normal case the object will be in the DOM tree. This is why the return value from Node::appendChild, for example, is PassRefPtr and not a raw pointer.

+    String wholeText();

Could we make this a const member?

+shouldBe("entityReference.textContent", "''"); // change to '<' when we support EntityReference nodes
+shouldBe("childrenBeforeFailedAppend", "0"); // Change to 1 if Entity node support is added.
+shouldBe("childrenBeforeFailedAppend", "0"); // Change to 1 if Entity node support is added.

I'd prefer that the shouldBe results reflect correct behavior, rather than the current behavior of our engine. The expected test results will still regression-test our current behavior, because FAIL and the current values will be in the test output.
Comment 6 Eric Seidel (no email) 2008-02-07 23:10:51 PST
+        Node* onePastEndText = endText->nextSibling();
+        for (Node* n = nextSibling(); n != onePastEndText;) {

Same thing here. Both onePastEndText and n need to be RefPtr.

If we're protecting ourselves from insane mutation event handlers... couldn't a mutation event handler end up removing onePastEndText from the sibling list?  Thus if onePastEndText != 0, we'd crash, no?
Comment 7 Eric Seidel (no email) 2008-02-07 23:15:15 PST
Bah!  This is insane.  Mutation event handlers could do all sorts of crazy things, including transplanting the node we're next going to traverse to somewhere else in the document.  Thus making it impossible to simply traverse the sibling list.  We'd end up just removing random nodes until we walked off the end of whatever node-list we got transplanted into!
Comment 8 Eric Seidel (no email) 2008-02-07 23:15:43 PST
CCing darin so he can see my comments about the insanity that is trying to protect ourselves from rogue mutation event handlers.
Comment 9 Eric Seidel (no email) 2008-02-07 23:49:44 PST
I originally had wholeText() const, but thought all the const and const_cast crud was ugly.  I added it back in my forthcoming patch however.
Comment 10 Eric Seidel (no email) 2008-02-07 23:52:25 PST
Created attachment 18996 [details]
Address Darin's concerns (delta from previous patch)

 .../readonly-exceptions-expected.txt               |    6 +-
 .../resources/readonly-exceptions.js               |    6 +-
 WebCore/dom/Text.cpp                               |   70 +++++++++++---------
 WebCore/dom/Text.h                                 |    4 +-
 4 files changed, 46 insertions(+), 40 deletions(-)
Comment 11 Eric Seidel (no email) 2008-02-07 23:57:31 PST
Comment on attachment 18996 [details]
Address Darin's concerns (delta from previous patch)

I can upload a combined patch if you need.
Comment 12 Eric Seidel (no email) 2008-02-08 00:05:23 PST
Created attachment 18997 [details]
Address Darin's concerns (delta from previous patch)

 .../readonly-exceptions-expected.txt               |    6 +-
 .../resources/readonly-exceptions.js               |    6 +-
 WebCore/dom/Text.cpp                               |   70 +++++++++++---------
 WebCore/dom/Text.h                                 |    4 +-
 4 files changed, 46 insertions(+), 40 deletions(-)
Comment 13 Darin Adler 2008-02-08 00:08:46 PST
(In reply to comment #6)
> If we're protecting ourselves from insane mutation event handlers... couldn't a
> mutation event handler end up removing onePastEndText from the sibling list? 
> Thus if onePastEndText != 0, we'd crash, no?

Our goal is to protect ourselves well enough from insane event handlers to prevent them from causing a crash or infinite loop. That may be an impossible challenge though. We should at least do the basics for now and think more deeply about how to handle the tougher issues.
Comment 14 Darin Adler 2008-02-08 00:14:46 PST
Comment on attachment 18997 [details]
Address Darin's concerns (delta from previous patch)

I would have preferred to review a combined patch:

+        case Node::CDATA_SECTION_NODE:
+            t = static_cast<const Text*>(n);

The above is incorrect. A CDATA section will be a CharacterData node and *not* a Text node. So it's an error to cast to Text*. Text is derived from CharacterData, so if we want to handle both we should be using the type CharacterData everywhere, and not Text.

I feel bad for spreading the const disease into this code with my suggestion of marking wholeText() const. Sorry. I probably would have put the const_cast calls at the call sites inside Text::wholeText and left the helper functions themselves alone.

+            RefPtr<Node> nodeToRemove(n.release());
             n = n->nextSibling();

This doesn't look like it will work. Calling release() will set n to 0 and you'll crash on the n->nextSibling() call. You should re-run the tests.
Comment 15 Eric Seidel (no email) 2008-02-08 00:18:48 PST
Yes, the RefPtr nodeToRemove(n.release()) was bogus.  I'll fix.

I think you're confused about CDATA_SECTION_NODE.  That means its a CDataSection, which is a subclass of Text (which is a subclass of CharacterData).

class CDATASection : public Text


Comment 16 Darin Adler 2008-02-08 00:22:42 PST
(In reply to comment #15)
> I think you're confused about CDATA_SECTION_NODE.  That means its a
> CDataSection, which is a subclass of Text (which is a subclass of
> CharacterData).
> 
> class CDATASection : public Text

Got it. My bad.
Comment 17 Eric Seidel (no email) 2008-02-08 00:26:16 PST
Created attachment 18999 [details]
Add wholeText/replaceWholeText support (final squashed patch, tests all pass)

 LayoutTests/ChangeLog                              |   27 ++++++
 .../core/textreplacewholetext01-expected.txt       |    3 +-
 .../core/textreplacewholetext02-expected.txt       |    3 +-
 .../core/textreplacewholetext03-expected.txt       |    3 +-
 .../core/textreplacewholetext04-expected.txt       |    3 +-
 .../core/textreplacewholetext05-expected.txt       |    3 +-
 .../core/textreplacewholetext06-expected.txt       |    3 +-
 .../core/textreplacewholetext07-expected.txt       |    3 +-
 .../xhtml/level3/core/textwholetext01-expected.txt |    3 +-
 .../xhtml/level3/core/textwholetext02-expected.txt |    3 +-
 .../xhtml/level3/core/textwholetext03-expected.txt |    3 +-
 .../readonly-exceptions-expected.txt               |   23 +++++
 .../dom/EntityReference/readonly-exceptions.html   |   13 +++
 .../dom/EntityReference/resources/TEMPLATE.html    |   13 +++
 .../resources/readonly-exceptions.js               |   33 +++++++
 .../fast/dom/Window/window-properties-expected.txt |    1 +
 WebCore/ChangeLog                                  |   23 +++++
 WebCore/dom/Document.cpp                           |    7 ++-
 WebCore/dom/Node.cpp                               |    2 +-
 WebCore/dom/Text.cpp                               |   95 ++++++++++++++++++++
 WebCore/dom/Text.h                                 |    4 +
 WebCore/dom/Text.idl                               |    4 +
 22 files changed, 253 insertions(+), 22 deletions(-)
Comment 18 Eric Seidel (no email) 2008-02-08 00:53:38 PST
Created attachment 19000 [details]
Add wholeText/replaceWholeText support (final squashed patch, tests all pass)

 LayoutTests/ChangeLog                              |   33 +++++++
 .../core/textreplacewholetext01-expected.txt       |    3 +-
 .../core/textreplacewholetext02-expected.txt       |    3 +-
 .../core/textreplacewholetext03-expected.txt       |    3 +-
 .../core/textreplacewholetext04-expected.txt       |    3 +-
 .../core/textreplacewholetext05-expected.txt       |    3 +-
 .../core/textreplacewholetext06-expected.txt       |    3 +-
 .../core/textreplacewholetext07-expected.txt       |    3 +-
 .../xhtml/level3/core/textwholetext01-expected.txt |    3 +-
 .../xhtml/level3/core/textwholetext02-expected.txt |    3 +-
 .../xhtml/level3/core/textwholetext03-expected.txt |    3 +-
 .../readonly-exceptions-expected.txt               |   23 +++++
 .../dom/EntityReference/readonly-exceptions.html   |   13 +++
 .../dom/EntityReference/resources/TEMPLATE.html    |   13 +++
 .../resources/readonly-exceptions.js               |   33 +++++++
 .../fast/dom/Text/replaceWholeText-expected.txt    |   13 +++
 LayoutTests/fast/dom/Text/replaceWholeText.html    |   13 +++
 LayoutTests/fast/dom/Text/resources/TEMPLATE.html  |   13 +++
 .../fast/dom/Text/resources/replaceWholeText.js    |   16 ++++
 .../fast/dom/Window/window-properties-expected.txt |    1 +
 WebCore/ChangeLog                                  |   23 +++++
 WebCore/dom/Document.cpp                           |    7 ++-
 WebCore/dom/Node.cpp                               |    2 +-
 WebCore/dom/Text.cpp                               |   91 ++++++++++++++++++++
 WebCore/dom/Text.h                                 |    4 +
 WebCore/dom/Text.idl                               |    4 +
 26 files changed, 310 insertions(+), 22 deletions(-)
Comment 19 Darin Adler 2008-02-08 00:57:55 PST
Comment on attachment 19000 [details]
Add wholeText/replaceWholeText support (final squashed patch, tests all pass)

r=me
Comment 20 Eric Seidel (no email) 2008-02-08 01:05:55 PST
Landed in r30088.
Comment 21 Lucas Forschler 2019-02-06 09:03:46 PST
Mass moving XML DOM bugs to the "DOM" Component.