Bug 7152 – REGRESSION: Select All does not highlight table if it's last in the document

• Status: VERIFIED
• Resolution: FIXED
• Component: HTML Editing

The page footer is not included in the selection when you do Select All in the Enter Bug form.

Created attachment 6404 [details]
Test case reduction

First, just a note that it's just visually that "select all" doesn't select everything, since copying the text after hitting cmd+a gets everything (even the "unselected" part) into the clipboard. 

In the reduction, removing "display: table;" from the div makes it (visually) selectable again.

(In reply to comment #1)

Another great reduction and a good observation! It is also sufficient to add text after the last div in order to make the div highlight correctly.

Based on testing nightly builds, I think this is a regression from r12358, which was a fix for a bunch of editing bugs, including <rdar://problem/4259845> Table editing in design mode is broken.

[David Harrison]

This happens even not in design mode.

[Darin Adler]

Even though this happens outside of editing, it's in the editing-related code to set up selections.

[Darin Adler]

Here's what it looks like to me.

The problem is caused by the fact that there is a visible position after the table, represented as the <div> element of the table with offset 1 (meaning "past the text node inside"). This shows up as the "deep equivalent" of the position, but RenderCanvas::setSelection can't deal with a selection end node that is a node with children that also need to be selected. It expects to see only positions that are "deep equivalents". In the case of this document, that means that the position needs to be at the end of a text node, not in a parent node with an offset past that text node.

I think the long term direction may be to correct functions like RenderCanvas::setSelection to deal with endpoints like these, but it's not clear to me yet what the short term solution should be.

[Darin Adler]

I see now where the regression came from. Position::inRenderedContent now returns true both before and after tables:

    if (isTableElement(node()) || editingIgnoresContent(node()))
        return offset() == 0 || offset() == maxDeepOffset(node());

While I see the sense in this, it does mean that VisiblePosition::deepEquivalent is now returning offsets that aren't really "deep" in the old sense.

[Darin Adler]

Created attachment 6614 [details]
one way to fix this

[Maciej Stachowiak]

Looks like an ok short-term fix, although I will let hyatt, harrison or justing comment. I also wish this kind of code about updating the selection display could be on SelectionController.

[Justin Garcia]

Comment on attachment 6614 [details]
one way to fix this

+        while (endNode && endOffset != 0 && !offsetInCharacters(endNode->nodeType())) {
+            if (NodeImpl* child = startNode->childNode(endOffset)) {
+                endNode = child;
+                endOffset = 0;

Don't you want to use endNode instead of startNode here?

+        if (startNode && !isAtomicNode(startNode))
+            if (NodeImpl* child = startNode->childNode(startOffset)) {
+                startNode = child;
+                startOffset = 0;
+            }

This would only descend one level, why not continue descending until you hit an atomic node, like you did to fix up the end position?

The descent that you did could leave you at a node that doesn't have a renderer.  What you'd need to do is descend to an atomic position, create a visible position with that, then pull out that VPs deepEquivalent and give that to RenderCanvas::setSelection.

But I think that we should fix RenderCanvas::setSelection.

[Justin Garcia]

Comment on attachment 6614 [details]
one way to fix this

r-'ing, i'm going to work on this.

[Alice Liu]

<rdar://problem/4483908>

[Justin Garcia]

Created attachment 7322 [details]
patch

[Darin Adler]

Comment on attachment 7322 [details]
patch

The operation called nextInPreOrder here is called traverseNextNode in the DOM tree. And the one called nextInPreOrderAfterChildren here is called traversNextSibling, and previousInPreOrder is traversePreviousNode. I suggest using the same terminology here.

[Justin Garcia]

> (From update of attachment 7322 [details] [edit])
> The operation called nextInPreOrder here is called traverseNextNode in the DOM
> tree. And the one called nextInPreOrderAfterChildren here is called
> traversNextSibling, and previousInPreOrder is traversePreviousNode.

"Traverse" is a verb but the traverseX functions don't change the node, so I was thinking of changing them to match these.  What do you think?

[Justin Garcia]

(In reply to comment #15)
> > (From update of attachment 7322 [details] [edit] [edit])
> > The operation called nextInPreOrder here is called traverseNextNode in the DOM
> > tree. And the one called nextInPreOrderAfterChildren here is called
> > traversNextSibling, and previousInPreOrder is traversePreviousNode.
> 
> "Traverse" is a verb but the traverseX functions don't change the node, so I
> was thinking of changing them to match these.  What do you think?

I also think these are better since they tell what kind of traversal they do.

[Maciej Stachowiak]

Comment on attachment 7322 [details]
patch

+        (WebCore::RenderObject::nextInPreOrder): Renamed from nextRenderer, cleaned up the logic a little.
+        (WebCore::RenderObject::nextInPreOrderAfterChildren): Added, like Node::traverseNextSibling.

Might be good to call these traverseNext[Renderer] and traverseNextSibling for naming consistency with the Node ones.

+        (WebCore::RenderObject::previousInPreOrder): Renamed from previousRenderer.

Likewise, traversePrevious[Renderer]

On the other hand, maybe your new names are more clear and should be used for Node as well.

+RenderObject* objectAfterPosition(RenderObject* object, unsigned offset)

I would call this rendererAfterPosition or something.

Other than the naming nitpicks, r=me

[Justin Garcia]

> On the other hand, maybe your new names are more clear and should be used for
> Node as well.

I think I want to rename traverseX to match these.

> 
> +RenderObject* objectAfterPosition(RenderObject* object, unsigned offset)
> 
> I would call this rendererAfterPosition or something.

OK.

[Darin Adler]

(In reply to comment #15)
> "Traverse" is a verb but the traverseX functions don't change the node, so I
> was thinking of changing them to match these.  What do you think?

I think you're right!

Verified in r13594 nightly