Ranges are not fixed after mutation (affects Acid3 test 13)

• Component: DOM
• Status: RESOLVED FIXED

[Rene v Amerongen]

When using the supplied code as sample
1. USING: 
>node2.appendChild(rangeAnd2.extractContents());
>rangeAnd2.insertNode(node2);

 you will see that the andRange and the janeDoeRange are damaged and not fixed.
Because of Drosera isn't really helpfull yet with this, I can't realy check what is going on, but it looks like janeRange is pointing to "john"'s content and the visual reference is gone ( whatever that is ).

2. USING: 
>rangeAnd2.surroundContents(node2);

 you will see that the  janeDoeRange is damaged and not fixed.
But also here it looks like janeRange is pointing to "john"'s content ( but in another test it did point to somewhere else ).

See about fixing :http://www.w3.org/TR/DOM-Level-2-Traversal-Range/ranges.html#Level-2-Range-Mutation
About identical results expected see: http://developer.mozilla.org/en/docs/DOM:range.surroundContents

Because some pages are using this, they let safari crash at a later stadium.

[Rene v Amerongen]

Created attachment 12056 [details]
Sample of the error

Sample code

[Alexey Proskuryakov]

Created attachment 17114 [details]
another test case

[Ian 'Hixie' Hickson]

The spec may be stupid, but given the spec, it's not clear this is an error.

[Alexey Proskuryakov]

Ian, are you saying that the original test case may work correctly in WebKit somehow?

The referenced spec seems pretty clear that ranges should be fixed after mutation, which we never do AFAICT. What am I missing?

[Ian 'Hixie' Hickson]

The second test case seems valid.

The first test case expects the "Jane" range to keep being around "Jane", but as far as I can tell that's wrong. As soon as you surround the "&", the text node gets split and as far as i can tell that drops the Jane range on the floor according to the spec. (Seems stupid, and I think we should fix the spec, but that's another problem.) It does seem that the & part should stay in the range though, yes.

[Darin Adler]

*** Bug 16766 has been marked as a duplicate of this bug. ***

[Darin Adler]

I'm getting started implementing this. It's fun. The document's going to keep a set of ranges indexed by their endpoint containers.

[Darin Adler]

Created attachment 19613 [details]
work in progress

I've made a lot of progress on this. The attached patch includes a test case and fixes all the cases tested in that test case, but there's still quite a ways to go. Here are some open issues:

    1) Mozilla differs from the DOM Level 2 specification on handling of modifications that intersect the start position of the range. This patch implements the specification rather than matching Mozilla. I haven't tested IE yet.

    2) One of our existing regression tests fails with this patch because the new automatically-updating ranges break the processContents function; that has to be fixed by making changes to the processContents code.

    3) The test attached covers a lot, but there are many things it does not cover. There are lots of different ways to mutate the DOM and they all should be tested to see if they have the correct behavior when a Range is involved. Including editing operations done with execCommand.

    4) I haven't done any real performance testing or analysis of this patch. It might slow things down too much, and if so I'll have to explore various optimizations for the common cases where there are no ranges or very few ranges that don't intersect many DOM operations.

    5) There are too many new declarations and definitions in Range.h in this patch. The new classes probably should go in a new RangeMutation.h header, since only a couple source files need to see their definitions.

    6) The patch has too much "clean up" mixed in with the substantive change. That probably should be landed first, separately.

The patch *does* fix test 13 in Acid3.

[Maciej Stachowiak]

(In reply to comment #8)
> Created an attachment (id=19613) [edit]
> work in progress
> 
> I've made a lot of progress on this. The attached patch includes a test case
> and fixes all the cases tested in that test case, but there's still quite a
> ways to go. Here are some open issues:

I haven't read the patch yet, but here's some comments on some of the design issues. (Don't forget to blog when you land this, per hyatt's promise...)

>     1) Mozilla differs from the DOM Level 2 specification on handling of
> modifications that intersect the start position of the range. This patch
> implements the specification rather than matching Mozilla. I haven't tested IE
> yet.

I don't think IE implements DOM Level 2 Range at all. I think it is probably ok to go with the spec for now, and back off if we learn of a compatibility issue during the dev cycle.

>     2) One of our existing regression tests fails with this patch because the
> new automatically-updating ranges break the processContents function; that has
> to be fixed by making changes to the processContents code.

Clearly we should fix processContents; do you need specific advice on this?

>     3) The test attached covers a lot, but there are many things it does not
> cover. There are lots of different ways to mutate the DOM and they all should
> be tested to see if they have the correct behavior when a Range is involved.
> Including editing operations done with execCommand.

Some other possible weird cases are changes triggered by the parser (innerHTML, initial parse, document.write).

>     4) I haven't done any real performance testing or analysis of this patch.
> It might slow things down too much, and if so I'll have to explore various
> optimizations for the common cases where there are no ranges or very few ranges
> that don't intersect many DOM operations.

For performance testing of DOM operations I'd suggest:
- PLT
- http://www.quirksmode.org/dom/innerhtml.html
- http://webkit.org/perf/slickspeed/

>     5) There are too many new declarations and definitions in Range.h in this
> patch. The new classes probably should go in a new RangeMutation.h header,
> since only a couple source files need to see their definitions.

I'll have to read the patch to form an opinion. If some of these declarations are for updating possibly-changed Ranges rather than using Ranges, then yes, I think that split would make sense.

>     6) The patch has too much "clean up" mixed in with the substantive change.
> That probably should be landed first, separately.

I agree that separating clean-up that does not alter behavior would be a good idea.

> 
> The patch *does* fix test 13 in Acid3.
> 

[Darin Adler]

Created attachment 19626 [details]
first round of Range improvements for review

[Justin Garcia]

+    RefPtr<Node> container;
+    int posOffset; // to be renamed to offset when we get rid of offset()

Seemed strange to me to call it Position::container, since it can be a text node, which isn't a container in WebCore parlance.  OTOH the DOM spec refers to a Range's start and end as being inside "containers", so perhaps what you've done is better than Position::node.


 bool Range::boundaryPointsValid() const
 {
-    return m_startContainer && m_endContainer && compareBoundaryPoints(m_startContainer.get(), m_startOffset, m_endContainer.get(), m_endOffset) <= 0;
+    return m_start.container && compareBoundaryPoints(m_start.container.get(), m_start.offset, m_end.container.get(), m_end.offset) <= 0;
 }

Can m_end.container be null?


-                ProcessingInstruction* pi= static_cast<ProcessingInstruction*>(m_startContainer.get());
+                ProcessingInstruction* pi= static_cast<ProcessingInstruction*>(m_start.container.get());

You could add a space before the '=' here.


Index: WebCore/editing/VisiblePosition.h
===================================================================
--- WebCore/editing/VisiblePosition.h	(revision 30914)
+++ WebCore/editing/VisiblePosition.h	(working copy)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2004 Apple Computer, Inc.  All rights reserved.
+ * Copyright (C) 2004, 2008 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -26,6 +26,7 @@
 #ifndef VisiblePosition_h
 #define VisiblePosition_h
 
+#include "Node.h"
 #include "Position.h"
 
 namespace WebCore {


Shouldn't you include Node.h from VisiblePosition.cpp instead?


+        Made constructors private, added create functions.

Why is this necessary, why not just register Range's with Document::attachRange() inside Range constructors?

[Darin Adler]

(In reply to comment #11)
> +    RefPtr<Node> container;
> 
> Seemed strange to me to call it Position::container, since it can be a text
> node, which isn't a container in WebCore parlance. OTOH the DOM spec refers to
> a Range's start and end as being inside "containers", so perhaps what you've
> done is better than Position::node.

Yes, I'm changing the nomenclature here to match DOM Range.

>  bool Range::boundaryPointsValid() const
>  {
> -    return m_startContainer && m_endContainer &&
> compareBoundaryPoints(m_startContainer.get(), m_startOffset,
> m_endContainer.get(), m_endOffset) <= 0;
> +    return m_start.container && compareBoundaryPoints(m_start.container.get(),
> m_start.offset, m_end.container.get(), m_end.offset) <= 0;
>  }
> 
> Can m_end.container be null?

It can only be 0 if m_start.container is also 0.

> -                ProcessingInstruction* pi=
> static_cast<ProcessingInstruction*>(m_startContainer.get());
> +                ProcessingInstruction* pi=
> static_cast<ProcessingInstruction*>(m_start.container.get());
> 
> You could add a space before the '=' here.

Will do.

>  #define VisiblePosition_h
> 
> +#include "Node.h"
>  #include "Position.h"
> 
>  namespace WebCore {
>
> Shouldn't you include Node.h from VisiblePosition.cpp instead?

No, it's needed in the header, for 

> +        Made constructors private, added create functions.
> 
> Why is this necessary, why not just register Range's with
> Document::attachRange() inside Range constructors?

It has nothing to do with attachRange(). We're doing this for all classes that are refcounted to avoid problems with the initial refcount 0. See bug 17257 for details.

Comment on attachment 19626 [details]
first round of Range improvements for review

+    if (m_start.offset == 0)

should be written as:
+    if (!m_start.offset)

I don't know the motivation for the unsigned -> int changes.

r=me

[Darin Adler]

(In reply to comment #13)
> I don't know the motivation for the unsigned -> int changes.

Trying to be consistent with the other uses in the same class.

[Darin Adler]

Comment on attachment 19626 [details]
first round of Range improvements for review

I landed this first round a while ago.

[Darin Adler]

Created attachment 19758 [details]
patch

I resolved all the issues to my satisfaction.

Now the patch needs both review and performance testing, not necessarily in that order.

[Sam Weinig]

Comment on attachment 19758 [details]
patch

As I noted in person,  I think all the classes in RangeDocumentMutation.h should have there own header and implementation files.

r=me

[Darin Adler]

Committed revision 31075.

[Lucas Forschler]

Mass moving XML DOM bugs to the "DOM" Component.