Bug 52642 – Get rid of firstDeepEditingPositionForNode and lastDeepEditingPositionForNode

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

[Ryosuke Niwa]

firstDeepEditingPositionForNode and lastDeepEditingPositionForNode need to be removed because they produce legacy editing positions.

[Levi Weintraub]

Created attachment 82877 [details]
Work in progress

There are still various places in the code I'm uncovering that handle legacy positions incorrectly. I'm addressing them as they become apparent.

[Levi Weintraub]

Created attachment 83415 [details]
Patch

[Levi Weintraub]

This patch will fail without the changes in the bugs it's listed to depend on.

[Levi Weintraub]

Created attachment 83881 [details]
Patch

[Levi Weintraub]

Re-uploading fixing the conflicts so it'll build.

[Ryosuke Niwa]

Comment on attachment 83881 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=83881&action=review

Thanks for tackling this hard bug!  It seems like we need a few more iterations before landing this one.

> Source/WebCore/dom/Position.h:194
> +    return a.anchorNode() == b.anchorNode() && a.deprecatedEditingOffset() == b.deprecatedEditingOffset() && a.anchorType() == b.anchorType();

Should we also check whether values of m_isLegacyEditingPosition are same?

> Source/WebCore/editing/TypingCommand.cpp:574
> -            setEndingSelection(VisibleSelection(endingSelection().end(), lastDeepEditingPositionForNode(downstreamEnd.deprecatedNode()), DOWNSTREAM));
> +            setEndingSelection(VisibleSelection(endingSelection().end(), positionAfterNode(downstreamEnd.deprecatedNode()), DOWNSTREAM));

Why not firstPositionInOrAfterNode?

> Source/WebCore/editing/VisibleSelection.cpp:92
> -    return VisibleSelection(firstDeepEditingPositionForNode(node), lastDeepEditingPositionForNode(node), DOWNSTREAM);
> +    return VisibleSelection(firstPositionInNode(node), lastPositionInNode(node), DOWNSTREAM);

Is it always safe to return position inside the node?  It seems like this is always called for anchor element?  We should probably assert that so that we don't start generating positions inside other elements / nodes.

> Source/WebCore/editing/VisibleSelection.cpp:501
> -                p = lastDeepEditingPositionForNode(shadowAncestor);
> +                p = lastPositionInNode(shadowAncestor);

Why does this make sense?  If I understand correctly, shadow ancestor is nodes like input, textarea, etc... that contains shadow DOM, right?  We should always position before or after that node.  r- because of this.

> Source/WebCore/editing/htmlediting.cpp:281
> +    if (comparePositions(position, firstPositionInNode(highestRoot)) == -1 && highestRoot->isContentEditable())
> +        return firstPositionInOrBeforeNode(highestRoot);

I don't get this change.  If you know you can always get a position inside the highestRoot, then why would you call firstPositionInOrBeforeNode when you return?  r- because of this.

> Source/WebCore/editing/htmlediting.cpp:287
> -            p = lastDeepEditingPositionForNode(shadowAncestor);
> +            p = lastPositionInOrAfterNode(shadowAncestor);

I don't think it makes any sense to use a position inside a shadow ancestor.  You should always call positionAfterNode here.

> Source/WebCore/editing/htmlediting.cpp:725
> -    VisiblePosition firstInListChild(firstDeepEditingPositionForNode(listChildNode));
> -    VisiblePosition lastInListChild(lastDeepEditingPositionForNode(listChildNode));
> +    VisiblePosition firstInListChild(firstPositionInNode(listChildNode));
> +    VisiblePosition lastInListChild(lastPositionInNode(listChildNode));

I don't think this is right. listChildNode can be BR or whatever node under ul / ol, and it's not necessarily li.

> Source/WebCore/editing/visible_units.cpp:749
> -        return firstDeepEditingPositionForNode(startNode);
> +        return positionBeforeNode(startNode);

Why not firstPositionInOrBeforeNode?

> Source/WebCore/editing/visible_units.cpp:808
> -        return lastDeepEditingPositionForNode(startNode);
> +        return positionAfterNode(startNode);

Ditto.

[Levi Weintraub]

Comment on attachment 83881 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=83881&action=review

>> Source/WebCore/dom/Position.h:194
>> +    return a.anchorNode() == b.anchorNode() && a.deprecatedEditingOffset() == b.deprecatedEditingOffset() && a.anchorType() == b.anchorType();
> 
> Should we also check whether values of m_isLegacyEditingPosition are same?

Perhaps we should only consider anchorType to matter if both are new positions? I certainly don't want to and m_isLegacyEditingPosition onto the expression. Two otherwise equivalent positions in a text node shouldn't be considered different only because one was created using a legacy method.

>> Source/WebCore/editing/TypingCommand.cpp:574
>> +            setEndingSelection(VisibleSelection(endingSelection().end(), positionAfterNode(downstreamEnd.deprecatedNode()), DOWNSTREAM));
> 
> Why not firstPositionInOrAfterNode?

This is only for tables, and we actually want to use the position after it, not inside.

>> Source/WebCore/editing/VisibleSelection.cpp:92
>> +    return VisibleSelection(firstPositionInNode(node), lastPositionInNode(node), DOWNSTREAM);
> 
> Is it always safe to return position inside the node?  It seems like this is always called for anchor element?  We should probably assert that so that we don't start generating positions inside other elements / nodes.

It should always be safe, and therefor is definitely worthy of an assertion, thanks :)

>> Source/WebCore/editing/VisibleSelection.cpp:501
>> +                p = lastPositionInNode(shadowAncestor);
> 
> Why does this make sense?  If I understand correctly, shadow ancestor is nodes like input, textarea, etc... that contains shadow DOM, right?  We should always position before or after that node.  r- because of this.

Good catch, you're right. I confused shadowAncestor with shadowRoot, which we never want to be before/after.

>> Source/WebCore/editing/visible_units.cpp:749
>> +        return positionBeforeNode(startNode);
> 
> Why not firstPositionInOrBeforeNode?

We don't want positions inside tables, images, or horizontal rules.

[Levi Weintraub]

Comment on attachment 83881 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=83881&action=review

>> Source/WebCore/editing/htmlediting.cpp:725
>> +    VisiblePosition lastInListChild(lastPositionInNode(listChildNode));
> 
> I don't think this is right. listChildNode can be BR or whatever node under ul / ol, and it's not necessarily li.

Correct me if I'm wrong, but it appears enclosingListChild will only return li, ul, ol, and dl elements. This function is attempting to determine if there's an empty list item that contains the visible position, so I don't think we'd want to compare that position to positions before or after that containing list/list item.

[Levi Weintraub]

Comment on attachment 83881 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=83881&action=review

>>> Source/WebCore/editing/htmlediting.cpp:725
>>> +    VisiblePosition lastInListChild(lastPositionInNode(listChildNode));
>> 
>> I don't think this is right. listChildNode can be BR or whatever node under ul / ol, and it's not necessarily li.
> 
> Correct me if I'm wrong, but it appears enclosingListChild will only return li, ul, ol, and dl elements. This function is attempting to determine if there's an empty list item that contains the visible position, so I don't think we'd want to compare that position to positions before or after that containing list/list item.

Ryosuke is totally right. This can return non-list items. Fixing this too.

[Levi Weintraub]

Created attachment 85102 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 85102 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=85102&action=review

Great! Most of changes look good to me. I have a few questions:

> Source/WebCore/dom/Position.cpp:167
>          return Position(m_anchorNode, 0, PositionIsOffsetInAnchor);

Isn't this just firstPositionInNode?

> Source/WebCore/dom/Position.cpp:328
> -    return m_offset <= 0;
> +    return m_anchorType == PositionIsBeforeAnchor || m_offset <= 0;

I don't think this is right.  If we're before/after node, then I don't think we're at first editing position in the (container) node.

> Source/WebCore/dom/Position.cpp:335
> -    return m_offset >= lastOffsetForEditing(deprecatedNode());
> +    return m_anchorType == PositionIsAfterAnchor || m_offset >= lastOffsetForEditing(deprecatedNode());

Ditto.

> Source/WebCore/dom/Position.cpp:786
> -        return !m_offset && !nodeIsUserSelectNone(deprecatedNode()->parentNode());
> +        return !m_offset && m_anchorType != PositionIsAfterAnchor && !nodeIsUserSelectNone(deprecatedNode()->parentNode());

Why are we adding this check here?

> Source/WebCore/editing/ApplyBlockElementCommand.cpp:196
> -            start = positionBeforeNode(start.deprecatedNode());
> +            start = firstPositionInNode(start.deprecatedNode());

How do we know that start can always contain a position?

> Source/WebCore/editing/ApplyBlockElementCommand.cpp:227
> -                start = positionBeforeNode(end.deprecatedNode()->previousSibling());
> +                start = firstPositionInNode(end.deprecatedNode()->previousSibling());

Ditto about end.

> Source/WebCore/editing/VisibleSelection.cpp:93
> -    return VisibleSelection(firstDeepEditingPositionForNode(node), lastDeepEditingPositionForNode(node), DOWNSTREAM);
> +    ASSERT(!editingIgnoresContent(node));
> +    return VisibleSelection(firstPositionInNode(node), lastPositionInNode(node), DOWNSTREAM);

How do we know that it's okay to have a position inside node?

[Levi Weintraub]

Comment on attachment 85102 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=85102&action=review

Thanks for the review!

>> Source/WebCore/dom/Position.cpp:328
>> +    return m_anchorType == PositionIsBeforeAnchor || m_offset <= 0;
> 
> I don't think this is right.  If we're before/after node, then I don't think we're at first editing position in the (container) node.

This definitely deserves a fixme and bug to get rid of at[First/last]EditingPositionForNode.

>> Source/WebCore/dom/Position.cpp:786
>> +        return !m_offset && m_anchorType != PositionIsAfterAnchor && !nodeIsUserSelectNone(deprecatedNode()->parentNode());
> 
> Why are we adding this check here?

After positions aren't considered candidates, but through javascript br's can still have children.

I'll add a comment.

>> Source/WebCore/editing/ApplyBlockElementCommand.cpp:196
>> +            start = firstPositionInNode(start.deprecatedNode());
> 
> How do we know that start can always contain a position?

This is a mistake. It should have been firstPositionInOrBefore, thanks!

>> Source/WebCore/editing/ApplyBlockElementCommand.cpp:227
>> +                start = firstPositionInNode(end.deprecatedNode()->previousSibling());
> 
> Ditto about end.

Ditto :D

>> Source/WebCore/editing/VisibleSelection.cpp:93
>> +    return VisibleSelection(firstPositionInNode(node), lastPositionInNode(node), DOWNSTREAM);
> 
> How do we know that it's okay to have a position inside node?

That's why I added the assert. Currently all callers ensure this.

[Levi Weintraub]

Created attachment 85758 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 85758 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=85758&action=review

> Source/WebCore/dom/Position.cpp:328
> -    return m_offset <= 0;
> +    return m_anchorType == PositionIsBeforeAnchor || m_offset <= 0;

Wait, didn't you say you'll add comment here?

> Source/WebCore/dom/Position.cpp:786
> -        return !m_offset && !nodeIsUserSelectNone(deprecatedNode()->parentNode());
> +        return !m_offset && m_anchorType != PositionIsAfterAnchor && !nodeIsUserSelectNone(deprecatedNode()->parentNode());

Ditto.

> Source/WebCore/editing/htmlediting.cpp:302
> +        return lastPositionInOrAfterNode(highestRoot);

?? Why lastPositionInOrAfterNode?  It should be lastPositionInNode, right?

[Levi Weintraub]

Comment on attachment 85758 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=85758&action=review

>> Source/WebCore/dom/Position.cpp:328
>> +    return m_anchorType == PositionIsBeforeAnchor || m_offset <= 0;
> 
> Wait, didn't you say you'll add comment here?

I put a fixme in the header

>> Source/WebCore/dom/Position.cpp:786
>> +        return !m_offset && m_anchorType != PositionIsAfterAnchor && !nodeIsUserSelectNone(deprecatedNode()->parentNode());
> 
> Ditto.

I thought about it more and thought this code really did speak for itself, as writing a comment seemed to just be rehashing.

>> Source/WebCore/editing/htmlediting.cpp:302
>> +        return lastPositionInOrAfterNode(highestRoot);
> 
> ?? Why lastPositionInOrAfterNode?  It should be lastPositionInNode, right?

Good catch :)

[Ryosuke Niwa]

Comment on attachment 85758 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=85758&action=review

>>> Source/WebCore/dom/Position.cpp:786
>>> +        return !m_offset && m_anchorType != PositionIsAfterAnchor && !nodeIsUserSelectNone(deprecatedNode()->parentNode());
>> 
>> Ditto.
> 
> I thought about it more and thought this code really did speak for itself, as writing a comment seemed to just be rehashing.

I still think we need a FIXME here.

[Levi Weintraub]

Created attachment 85841 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 85841 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=85841&action=review

> Source/WebCore/dom/Position.cpp:244
> +    // FIXME: Explicitly handle before/after positions

I don't think these comments are valuable.  It's clear from the fact it calls deprecatedEditingOffset().

[Levi Weintraub]

Committed r81165: <http://trac.webkit.org/changeset/81165>

[WebKit Review Bot]

http://trac.webkit.org/changeset/81165 might have broken GTK Linux 32-bit Debug and GTK Linux 64-bit Debug