Bug 125399 – Make indent command to work with positional CSS selector, e.g. :first-child, with visibility

• Component: HTML Editing
• Status: NEW

[Ryosuke Niwa]

Consider merging https://chromium.googlesource.com/chromium/blink/+/18413a6b5636dd13ee749cbfe63c3b347563b35d

This patch changes to compute range of contents in list item before inserting list element in IndentOutdentCommand::tryIndentingAsListItem() to avoid visibility change resulted by inserting list element.

The crash, assertion isn't Element, is caused by CompositeEditCommand::cloneParagraphUnderNewElement(start, end, outerNode, blockElement) tries to move nodes in ancestries until Document rather than limited to outerNode. This wrong |start| is passed as node before list element in VisiblePosition of start of list item contents. When list item is visibile, |start| is list item. Although, in this case, the list item is hidden, because the list item is second child element.

Here is sample HTML before calling CompositeEditCommand::moveParagraphWithClones(), which calls cloneParagraphUnderNewElement() in IndentOutdentCommand::tryIndentingAsListItem()

abcd<ul><UL></UL><li>xyz</li></ul>
- <UL></UL> is inserted list
- <li>xyz</li> is the list item to move

[László Langó]

Created attachment 218842 [details]
Patch

[Andreas Kling]

Ryosuke/Enrica/Darin: Could one of you have a look at this patch? I'd like to review it but I don't feel qualified.

[Ryosuke Niwa]

Comment on attachment 218842 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        Make indent command to work with positional CSS selector, e.g. :first-child, with visibility.

The bug title should be modified to reflect the fact this is a crash fix.

> Source/WebCore/editing/IndentOutdentCommand.cpp:82
> +    // We should calculate visible range in list item because inserting new
> +    // list element will change visibility of list item, e.g. :first-child
> +    // CSS selector.
> +    VisiblePosition startOfMove(start);
> +    VisiblePosition endOfMove(end);
> +

This comment doesn't explain why this is the right thing to do.
It appears to me that we should be failing out before calling moveParagraphWithClones
in the case where start & end up moving to unexpected places.

> LayoutTests/editing/execCommand/indent-with-first-child-crash.html:1
> +<style>

We can't have DOCTYPE here?

[László Langó]

Created attachment 223454 [details]
Patch

[László Langó]

(In reply to comment #3)
> (From update of attachment 218842 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=218842&action=review
> 
> This comment doesn't explain why this is the right thing to do.
> It appears to me that we should be failing out before calling moveParagraphWithClones
> in the case where start & end up moving to unexpected places.

I tried to debug that why is this wrong after the 'insertNodeBefore'. I tried to find a way how to check that the validity of 'start' and 'end' positions after  'insertNodeBefore'. I didn't getting closer. If you have some idea how can we do that, then I can do it. I couldn't figure out better fix then this for that assertion.

[Michael Catanzaro]

Comment on attachment 223454 [details]
Patch

Hi,

Apologies that your patch was not reviewed in a timely manner. Since it's now quite old, I am removing it from the review request queue. Please consider rebasing it on trunk and resubmitting.

To increase the chances of getting a review, consider using 'Tools/Scripts/webkit-patch upload --suggest-reviewers' to CC reviewers who might be interested in this bug.

[Ahmad Saleem]

This still compiles but we have imported test case from Blink here:

https://searchfox.org/wubkat/source/LayoutTests/imported/blink/editing/execCommand/indent-with-first-child-crash.html

and we don't crash it.

Do we need to do anything here?

NOTE - I tried merging locally and it merge and compiles. If needed, I am happy to do PR.

[Ahmad Saleem]

Just to add this patch seems to be got undone by following Blink commit:

https://chromium.googlesource.com/chromium/blink/+/0cd4b3864db4fc9c0abfeeee0775c5fcbbca0184

[Ahmad Saleem]

(In reply to Ahmad Saleem from comment #8)
> Just to add this patch seems to be got undone by following Blink commit:
> 
> https://chromium.googlesource.com/chromium/blink/+/
> 0cd4b3864db4fc9c0abfeeee0775c5fcbbca0184

Merging this:

```

// We should clone all the children of the list item for indenting purposes. However, in case the current
    // selection does not encompass all its children, we need to explicitally handle the same. The original
    // list item too would require proper deletion in that case.
    if (end.anchorNode() == selectedListItem.get() || end.anchorNode()->isDescendantOf(selectedListItem->lastChild()))
        moveParagraphWithClones(start, end, newList.get(), selectedListItem.get());
    else {
        moveParagraphWithClones(start, positionAfterNode(selectedListItem->lastChild()), newList.get(), selectedListItem.get());
        removeNode(*selectedListItem);
    }

```

leads to one WPT progression and matching with Blink / Chromium:

> https://wpt.fyi/results/editing/run/indent.html?label=master&label=experimental&aligned=&q=indent.html

This one:

>> [["indent",""]] "<ol><li>[foo]<br>bar<li>baz</ol>" compare innerHTML	

This one is passing in other browsers while failing only in WebKit / Safari - so it would be good to merge this and align with others: https://wpt.fyi/results/editing/run/indent.html?label=master&label=experimental&aligned=&q=safari%3Afail+chrome%3Apass+firefox%3Apass

Any thoughts?