I'll attach a test case but this is a brief explanation of the bug. Say you have the following: <div id="test"><b>[]text.</b></div> With [] representing the current selection. Calling window.getSelection.modify("extend", "forward", "word") twice renders: <div id="test"><b>[text.]</b></div> Now, if the test div is followed by a contenteditable div, doing the same renders this instead: <div id="test"><b>[text].</b></div> <div contentEditable>Another div</div> This is broken. Selection should work as in the first case for both.
Created attachment 200317 [details] Test case
Seems to be caused by the following in VisiblePosition::honorEditingBoundaryAtOrAfter(): // Return empty position if this position is non-editable, but pos is editable // FIXME: Move to the next non-editable region. if (!highestRoot) return VisiblePosition();
Shouldn't it instead return the last non-editable position before the editable one?
Created attachment 200415 [details] Patch
Comment on attachment 200415 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=200415&action=review > Source/WebCore/editing/VisiblePosition.cpp:457 > + // Move to previous non-editable region if this position is non-editable, but pos is editable This is not a what comment. We need to explain why we want to move back to the previous position. > Source/WebCore/editing/VisiblePosition.cpp:458 > + // FIXME: We skip non-editable regions inside the lowestEditableAncestor of pos for simplicity. I don’t understand this FIXME. It seems like this is just stating the current behavior. FIXME should mention what needs to be fixed. > Source/WebCore/editing/VisiblePosition.cpp:492 > + // Move to the next non-editable region if this position is non-editable, but pos is editable > + // FIXME: We skip non-editable regions inside the lowestEditableAncestor of pos for simplicity. > + if (!highestRoot) { Ditto.
Comment on attachment 200415 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=200415&action=review >> Source/WebCore/editing/VisiblePosition.cpp:458 >> + // FIXME: We skip non-editable regions inside the lowestEditableAncestor of pos for simplicity. > > I don’t understand this FIXME. It seems like this is just stating the current behavior. > FIXME should mention what needs to be fixed. Current behavior is to stop and return a null VisiblePosition. The new behavior is to move to the next editable region *but* we skip non-editable regions that might be inside the lowestEditableAncestor, like for instance in "<span editablecontent><span id="non-editable" editablecontent=false>foo</span></span><span id="next-editable" contenteditable>bar</span>", we skip "non-editable" and just move to "next-editable". What needs to be done to remove the new FIXME, is to traverse lowestEditableAncestor() and find the first non-editable that's a descendant of it, before skipping to its next/previous sibling.
Created attachment 200772 [details] Patch
I hope the comments in the new patch clarify all concerns.
Comment on attachment 200772 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=200772&action=review > Source/WebCore/editing/VisiblePosition.cpp:465 > + Node* previousNonEditable = lowestRoot->previousSibling(); > + while (previousNonEditable && previousNonEditable->rendererIsEditable()) > + previousNonEditable = previousNonEditable->previousSibling(); We either want to call Position::upstream or do something like positionBeforeNode(lowestEditableAncestor(pos.deepEquivalent().deprecatedNode())).previous(). Walking the DOM manually like this is almost never right. e.g. this fails if we had a structure like <div contenteditable><div><div contenteditable=false></div></div><div contenteditable=true>start here</div></div>
But we still need a similar while() iteration, no? Because of structures like <span contenteditable>foo</span><span contenteditable>bar</span>, getting the next position after current node will not always be enough to find the next non-editable one. I see that there is a method firstEditablePositionAfterPositionInRoot(const Position& position, Node* highestRoot). Do you think that working in an analogous method, firstNonEditablePositionAfterPositionInRoot() would make sense to solve this in a more elaborate way (and probably find non-editable positions that are in descendant nodes to an editable one as well).
(In reply to comment #10) > But we still need a similar while() iteration, no? We need to do that implicitly via other Position, htmlediting, & VisibleUnits functions.
Comment on attachment 200772 [details] Patch Removing from request queue; Ryosuke's comments need addressed.
Created attachment 459991 [details] Safari 15.5 matches Chrome but differs from Firefox I am not clear one expected behavior of attached "test case" but in Safari 15.5 on macOS 12.4, the behavior is similar to Chrome Canary 104 (as shown in attached screenshot) while it differs from Firefox Nightly 103. If I am testing it incorrectly, please retest it accordingly. Thanks!
IIRC at the time I filed this bug the underlying code where this bug happens was shared between Chromium and WebKit, so it's likely that the behavior is still the same if this has not been fixed in Chromium/Blink either.