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
Created attachment 218842 [details] Patch
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.
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?
Created attachment 223454 [details] Patch
(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.
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.
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.
Just to add this patch seems to be got undone by following Blink commit: https://chromium.googlesource.com/chromium/blink/+/0cd4b3864db4fc9c0abfeeee0775c5fcbbca0184
(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?