WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
11997
Ranges are not fixed after mutation (affects Acid3 test 13)
https://bugs.webkit.org/show_bug.cgi?id=11997
Summary
Ranges are not fixed after mutation (affects Acid3 test 13)
Rene v Amerongen
Reported
2006-12-27 04:11:28 PST
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.
Attachments
Sample of the error
(1.93 KB, text/html)
2006-12-27 04:13 PST
,
Rene v Amerongen
no flags
Details
another test case
(406 bytes, text/html)
2007-11-07 12:19 PST
,
Alexey Proskuryakov
no flags
Details
work in progress
(139.10 KB, patch)
2008-03-08 22:00 PST
,
Darin Adler
no flags
Details
Formatted Diff
Diff
first round of Range improvements for review
(116.30 KB, patch)
2008-03-09 21:47 PDT
,
Darin Adler
no flags
Details
Formatted Diff
Diff
patch
(83.28 KB, patch)
2008-03-14 00:23 PDT
,
Darin Adler
sam
: review+
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Rene v Amerongen
Comment 1
2006-12-27 04:13:41 PST
Created
attachment 12056
[details]
Sample of the error Sample code
Alexey Proskuryakov
Comment 2
2007-11-07 12:19:08 PST
Created
attachment 17114
[details]
another test case
Ian 'Hixie' Hickson
Comment 3
2008-01-05 00:12:54 PST
The spec may be stupid, but given the spec, it's not clear this is an error.
Alexey Proskuryakov
Comment 4
2008-01-05 02:46:42 PST
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
Comment 5
2008-01-06 15:26:46 PST
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
Comment 6
2008-02-10 12:35:28 PST
***
Bug 16766
has been marked as a duplicate of this bug. ***
Darin Adler
Comment 7
2008-02-10 14:07:08 PST
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
Comment 8
2008-03-08 22:00:29 PST
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
Comment 9
2008-03-09 04:01:04 PDT
(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
Comment 10
2008-03-09 21:47:56 PDT
Created
attachment 19626
[details]
first round of Range improvements for review
Justin Garcia
Comment 11
2008-03-10 15:17:58 PDT
+ 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
Comment 12
2008-03-10 16:10:17 PDT
(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.
mitz
Comment 13
2008-03-11 14:23:16 PDT
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
Comment 14
2008-03-11 14:26:00 PDT
(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 15
2008-03-14 00:19:51 PDT
Comment on
attachment 19626
[details]
first round of Range improvements for review I landed this first round a while ago.
Darin Adler
Comment 16
2008-03-14 00:23:38 PDT
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 17
2008-03-14 13:58:58 PDT
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
Comment 18
2008-03-15 10:27:38 PDT
Committed revision 31075.
Lucas Forschler
Comment 19
2019-02-06 09:03:25 PST
Mass moving XML DOM bugs to the "DOM" Component.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug