Bug 52642

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.

Created attachment 83415 [details] Patch

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

Created attachment 83881 [details] Patch

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

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.

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.

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.

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.

Created attachment 85102 [details] Patch

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?

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.

Created attachment 85758 [details] Patch

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?

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 :)

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.

Created attachment 85841 [details] Patch

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().

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

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