Bug 115517 - Selection in non-editable content gets stuck when approaching an editable node
Summary: Selection in non-editable content gets stuck when approaching an editable node
Status: UNCONFIRMED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-05-02 08:21 PDT by Claudio Saavedra
Modified: 2016-01-02 08:35 PST (History)
1 user (show)

See Also:


Attachments
Test case (411 bytes, text/html)
2013-05-02 08:35 PDT, Claudio Saavedra
no flags Details
Patch (7.79 KB, patch)
2013-05-03 08:50 PDT, Claudio Saavedra
no flags Details | Formatted Diff | Diff
Patch (8.08 KB, patch)
2013-05-06 13:28 PDT, Claudio Saavedra
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Claudio Saavedra 2013-05-02 08:21:00 PDT
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.
Comment 1 Claudio Saavedra 2013-05-02 08:35:20 PDT
Created attachment 200317 [details]
Test case
Comment 2 Claudio Saavedra 2013-05-02 09:39:39 PDT
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();
Comment 3 Claudio Saavedra 2013-05-02 09:40:32 PDT
Shouldn't it instead return the last non-editable position before the editable one?
Comment 4 Claudio Saavedra 2013-05-03 08:50:57 PDT
Created attachment 200415 [details]
Patch
Comment 5 Ryosuke Niwa 2013-05-06 11:57:56 PDT
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 6 Claudio Saavedra 2013-05-06 12:07:03 PDT
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.
Comment 7 Claudio Saavedra 2013-05-06 13:28:50 PDT
Created attachment 200772 [details]
Patch
Comment 8 Claudio Saavedra 2013-05-06 13:30:13 PDT
I hope the comments in the new patch clarify all concerns.
Comment 9 Ryosuke Niwa 2013-05-06 14:04:32 PDT
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>
Comment 10 Claudio Saavedra 2013-05-10 09:41:05 PDT
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).
Comment 11 Ryosuke Niwa 2013-05-10 10:36:47 PDT
(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 12 Michael Catanzaro 2016-01-02 08:35:10 PST
Comment on attachment 200772 [details]
Patch

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