Bug 24576 – Selection rendering code uses DOM offsets as if they were render tree offsets

• Status: ASSIGNED
• Component: Layout and Rendering

[Darin Adler]

I noticed that the selection code uses DOM offsets as if they were render tree child offsets. I will be fixing this.

[Darin Adler]

There is one case of this in Range.cpp and a few in RenderView.cpp.

I've got a patch underway for this now; it's going well. What I知 not so sure about yet is what regression tests I知 going to write to demonstrate the bug.

[Justin Garcia]

This can only be a problem if the DOM position is [container, n] where n is not zero but we have very few editing positions like that (which is what is used to feed RenderView::setSelection).  

It would be easier to write a regression test for the fix in Range::addLineBox rects (assuming you exposed that function) since you can use whatever end position you want.

[Alexey Proskuryakov]

Will fix fix bug 8855? It has a test case that triggers (used to trigger?) an assertion in debug builds, and caused some weird behavior in release.

[Darin Adler]

(In reply to comment #3)
> Will fix fix bug 8855? It has a test case that triggers (used to trigger?) an
> assertion in debug builds, and caused some weird behavior in release.

I'll check.

[Darin Adler]

Created attachment 28649 [details]
work in process -- can't land until after PositionIterator issues are fixed

This is a work in progress. I think the changes are all correct, but they are causing regression test failures. Some of those failures are progressions. But at least one of them is a problem, caused by the PositionIterator problem in the blocking bug.

[Justin Garcia]

-    RenderObject* stop = end->childAt(m_end.offset());
-    if (!stop)
-        stop = end->nextInPreOrderAfterChildren();
+    RenderObject* stop = 0;
+    for (Node* stopNode = pastLastNode(); stopNode; stopNode = stopNode->traverseNextNode()) {
+        if ((stop = stopNode->renderer()))
+            break;
+    }

we tried this and ran into a problem when the range we were iterating over ended in the last text node inside a form element (or in some other shadow tree).  

in this case pastLastNode() is null, so both stopNode and stop are null.  since shadow trees are in the main document's render tree, we iterate all the way to the end of the document w/o stopping.

[Darin Adler]

(In reply to comment #6)
> -    RenderObject* stop = end->childAt(m_end.offset());
> -    if (!stop)
> -        stop = end->nextInPreOrderAfterChildren();
> +    RenderObject* stop = 0;
> +    for (Node* stopNode = pastLastNode(); stopNode; stopNode =
> stopNode->traverseNextNode()) {
> +        if ((stop = stopNode->renderer()))
> +            break;
> +    }
> 
> we tried this and ran into a problem when the range we were iterating over
> ended in the last text node inside a form element (or in some other shadow
> tree).  
> 
> in this case pastLastNode() is null, so both stopNode and stop are null.  since
> shadow trees are in the main document's render tree, we iterate all the way to
> the end of the document w/o stopping.

OK. Simple to solve. We'll need to add code that knows how to go out of the shadow tree since the render tree covers both the shadow DOM and the real DOM.

[Darin Adler]

Thought the modern day editing experts might be interested in my struggle from three years ago. This was never resolved.