Bug 115517 – Selection in non-editable content gets stuck when approaching an editable node

• Component: HTML Editing
• Status: UNCONFIRMED

[Claudio Saavedra]

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.

[Claudio Saavedra]

Created attachment 200317 [details]
Test case

[Claudio Saavedra]

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

[Claudio Saavedra]

Shouldn't it instead return the last non-editable position before the editable one?

[Claudio Saavedra]

Created attachment 200415 [details]
Patch

[Ryosuke Niwa]

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.

[Claudio Saavedra]

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.

[Claudio Saavedra]

Created attachment 200772 [details]
Patch

[Claudio Saavedra]

I hope the comments in the new patch clarify all concerns.

[Ryosuke Niwa]

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>

[Claudio Saavedra]

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

[Ryosuke Niwa]

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

[Michael Catanzaro]

Comment on attachment 200772 [details]
Patch

Removing from request queue; Ryosuke's comments need addressed.

[Ahmad Saleem]

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!

[Claudio Saavedra]

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.