The ASSERT(useDownstream ? (result > vp) : (result < vp)) in WebCore::nextSentenceBoundaryInDirection() is not correct because it does not account for detached positions: <https://trac.webkit.org/browser/trunk/Source/WebCore/dom/Node.cpp?rev=252554#L1667>.
<rdar://problem/57332559>
<https://trac.webkit.org/browser/trunk/Source/WebCore/dom/Node.cpp?rev=252554#L1674>
You can only compare positions whose anchors are connected (in a document) and are in the same tree scope
(In reply to Daniel Bates from comment #3) > You can only compare positions whose anchors are connected (in a document) > and are in the same tree scope Otherwise, the result is non-deterministic by spec! [[ static inline unsigned short compareDetachedElementsPosition(Node& firstNode, Node& secondNode) { // If the 2 nodes are not in the same tree, return the result of adding DOCUMENT_POSITION_DISCONNECTED, // DOCUMENT_POSITION_IMPLEMENTATION_SPECIFIC, and either DOCUMENT_POSITION_PRECEDING or // DOCUMENT_POSITION_FOLLOWING, with the constraint that this is to be consistent. Whether to return // DOCUMENT_POSITION_PRECEDING or DOCUMENT_POSITION_FOLLOWING is implemented by comparing cryptographic // hashes of Node pointers. // See step 3 in https://dom.spec.whatwg.org/#dom-node-comparedocumentposition SHA1::Digest firstHash = hashPointer(&firstNode); SHA1::Digest secondHash = hashPointer(&secondNode); unsigned short direction = memcmp(firstHash.data(), secondHash.data(), SHA1::hashSize) > 0 ? Node::DOCUMENT_POSITION_PRECEDING : Node::DOCUMENT_POSITION_FOLLOWING; return Node::DOCUMENT_POSITION_DISCONNECTED | Node::DOCUMENT_POSITION_IMPLEMENTATION_SPECIFIC | direction; } ]] <https://trac.webkit.org/browser/trunk/Source/WebCore/dom/Node.cpp?rev=252554#L1597>
I was so tempted to use WebCore::comparePositions() to fix this, but it's implementation does not look the same to the detached element position check in Node::compareDocumentPosition() though I didn't dive deep enough to verify.
Created attachment 383898 [details] Patch
Comment on attachment 383898 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=383898&action=review > Source/WebCore/dom/Node.cpp:1613 > +bool areNodesConnectedInSameDocument(const Node* a, const Node* b) This is a misnomer. Not so much same document as same tree scope.
Comment on attachment 383898 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=383898&action=review > Source/WebCore/editing/VisibleUnits.cpp:1780 > + ASSERT(areVisiblePositionsInSameDocument(result, vp) && useDownstream ? (result > vp) : (result < vp)); This is wrong: only want to assert if areVisiblePositionsInSameDocument()
Created attachment 383899 [details] Patch
Thanks for the review!
Committed r252647: <https://trac.webkit.org/changeset/252647>
Comment on attachment 383899 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=383899&action=review > Source/WebCore/dom/Node.cpp:1617 > + // Note that we avoid comparing Attr nodes here, since they return false from isConnected() all the time (which seems like a bug). I don’t think this comment belongs here. I’d move it back where it came from. The code that "avoids" Attr nodes is not here, it’s in compareDocumentPosition, and the name of the function doesn't hide that it calls isConnected.
Comment on attachment 383899 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=383899&action=review >> Source/WebCore/dom/Node.cpp:1617 >> + // Note that we avoid comparing Attr nodes here, since they return false from isConnected() all the time (which seems like a bug). > > I don’t think this comment belongs here. I’d move it back where it came from. The code that "avoids" Attr nodes is not here, it’s in compareDocumentPosition, and the name of the function doesn't hide that it calls isConnected. Yep, I made a goof here. I don't get what you're saying about the name of the function. But the name of this function is also bad and should be changed: it checks if the nodes have the same connectedness.