Bug 42841 – [cleanup] logic to extract adjacent lists and list children in listifyParagraph and unlistifyParagraph should be isolated

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Ryosuke Niwa]

To simplify the patch for the bug 33668, we have to extract the code to find adjacent lists in listifyParagraph and list children in unlistifyParagraph.

[Ryosuke Niwa]

Created attachment 62326 [details]
cleanup patch

[Kent Tamura]

Comment on attachment 62326 [details]
cleanup patch

> @@ -268,8 +281,6 @@ PassRefPtr<HTMLElement> InsertListComman
>          // Update the start of content, so we don't try to move the list into itself.  bug 19066
>          if (insertionPos == start.deepEquivalent())
>              start = startOfParagraph(originalStart);
> -        previousList = outermostEnclosingList(previousPosition.deepEquivalent().node(), enclosingList(listElement.get()));
> -        nextList = outermostEnclosingList(nextPosition.deepEquivalent().node(), enclosingList(listElement.get()));
>      }
>  
>      moveParagraph(start, end, VisiblePosition(Position(placeholder.get(), 0)), true);
> @@ -281,6 +292,8 @@ PassRefPtr<HTMLElement> InsertListComman
>          listElement = m_listElement;
>  
>      if (listElement) {
> +        previousList = listElement->previousElementSibling();
> +        nextList = listElement->nextElementSibling();
>          if (canMergeLists(previousList, listElement.get()))
>              mergeIdenticalElements(previousList, listElement.get());
>          if (canMergeLists(listElement.get(), nextList))

I don't understand why these changes for previousList/nextList are correct.  Would you explain it please?

[Ryosuke Niwa]

(In reply to comment #2)
> (From update of attachment 62326 [details])
> > @@ -268,8 +281,6 @@ PassRefPtr<HTMLElement> InsertListComman
> >          // Update the start of content, so we don't try to move the list into itself.  bug 19066
> >          if (insertionPos == start.deepEquivalent())
> >              start = startOfParagraph(originalStart);
> > -        previousList = outermostEnclosingList(previousPosition.deepEquivalent().node(), enclosingList(listElement.get()));
> > -        nextList = outermostEnclosingList(nextPosition.deepEquivalent().node(), enclosingList(listElement.get()));
> >      }
> >  
> >      moveParagraph(start, end, VisiblePosition(Position(placeholder.get(), 0)), true);
> > @@ -281,6 +292,8 @@ PassRefPtr<HTMLElement> InsertListComman
> >          listElement = m_listElement;
> >  
> >      if (listElement) {
> > +        previousList = listElement->previousElementSibling();
> > +        nextList = listElement->nextElementSibling();
> >          if (canMergeLists(previousList, listElement.get()))
> >              mergeIdenticalElements(previousList, listElement.get());
> >          if (canMergeLists(listElement.get(), nextList))
> 
> I don't understand why these changes for previousList/nextList are correct.  Would you explain it please?

If we're inserting a list element, then previousList must be the previous element sibling of the list element we've just inserted because that's the only case we can merge the two lists.  Notice that the only reason we're obtaining previous and next lists are to merge them with the list we've inserted.  But we can't merge the lists unless they're siblings of each other since if had some other elements in between, we shouldn't be deleting that element. The argument for nextList is similar.

While the original code may obtain elements that are not siblings of listElement, conditions in canMergeLists prevented such elements to be merged with listElement.

[Kent Tamura]

Comment on attachment 62326 [details]
cleanup patch

Ok.

[Ryosuke Niwa]

Landed as http://trac.webkit.org/changeset/63941.