Summary: | [cleanup] logic to extract adjacent lists and list children in listifyParagraph and unlistifyParagraph should be isolated | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ryosuke Niwa <rniwa> | ||||
Component: | HTML Editing | Assignee: | Ryosuke Niwa <rniwa> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Blocker | CC: | enrica, eric, justin.garcia, ojan, tkent, tony | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | All | ||||||
OS: | All | ||||||
Bug Depends on: | |||||||
Bug Blocks: | 33668 | ||||||
Attachments: |
|
Description
Ryosuke Niwa
2010-07-22 12:04:35 PDT
Created attachment 62326 [details]
cleanup patch
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? (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. Comment on attachment 62326 [details]
cleanup patch
Ok.
Landed as http://trac.webkit.org/changeset/63941. |