Text manipulation should not manipulate nodes out of paragraph range

• Component: HTML Editing
• Status: RESOLVED FIXED

[Sihui Liu]

...

[Sihui Liu]

Created attachment 406445 [details]
Patch

[Wenson Hsieh]

Comment on attachment 406445 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=406445&action=review

> Source/WebCore/ChangeLog:3
> +        Text manipulationshould not manipulate nodes out of paragraph range

Nit - space before “should”

> Source/WebCore/ChangeLog:9
> +        the nodes on the start path but out ouf range correctly, and may change position of those nodes by mistake. For 

Nit - “out of”

> Source/WebCore/editing/TextManipulationController.cpp:840
> +    while (!startTopDownPath.isEmpty()) {
> +        auto lastNode = startTopDownPath.last();
> +        ASSERT(is<ContainerNode>(lastNode.get()));
> +        if (!downcast<ContainerNode>(lastNode.get()).hasOneChild())
> +            break;
> +        nodesToRemove.add(startTopDownPath.takeLast());
> +    }

Perhaps getPath() should just return `Vector<Ref<ContainerNode>>` instead, and change this into something along the lines of:
```
while (!startTopDownPath.isEmpty() && startTopDownPath.last()->hasOneChild())
    nodesToRemove.add(startTopDownPath.takeLast());
```

[Radar WebKit Bug Importer]

<rdar://problem/67375376>

[Ryosuke Niwa]

Is this patch getting landed??

[Sihui Liu]

(In reply to Ryosuke Niwa from comment #4)
> Is this patch getting landed??

Yes, will land soon.

[Sihui Liu]

Comment on attachment 406445 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=406445&action=review

>> Source/WebCore/ChangeLog:3
>> +        Text manipulationshould not manipulate nodes out of paragraph range
> 
> Nit - space before “should”

Okay.

>> Source/WebCore/ChangeLog:9
>> +        the nodes on the start path but out ouf range correctly, and may change position of those nodes by mistake. For 
> 
> Nit - “out of”

Okay.

>> Source/WebCore/editing/TextManipulationController.cpp:840
>> +    }
> 
> Perhaps getPath() should just return `Vector<Ref<ContainerNode>>` instead, and change this into something along the lines of:
> ```
> while (!startTopDownPath.isEmpty() && startTopDownPath.last()->hasOneChild())
>     nodesToRemove.add(startTopDownPath.takeLast());
> ```

I tried this and found we need to make changes to updateInsertions(), which seems unnecessary for fixing the bug. I can do this in a follow-up patch, as getPath should only return ContainerNodes.

[Sihui Liu]

Created attachment 407109 [details]
Patch for landing

[EWS]

Committed r266075: <https://trac.webkit.org/changeset/266075>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 407109 [details].