[SCM] WebKit Debian packaging branch, debian/experimental, updated. upstream/1.3.3-9427-gc2be6fc
rniwa at webkit.org
rniwa at webkit.org
Wed Dec 22 11:35:44 UTC 2010
The following commit has been merged in the debian/experimental branch:
commit 620e8d1f8f2ca0e69dcac6aec73c409119b37117
Author: rniwa at webkit.org <rniwa at webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Date: Fri Jul 30 06:39:13 2010 +0000
InsertOrderedList does not switch the list type properly when it has an inner list.
https://bugs.webkit.org/show_bug.cgi?id=43166
Reviewed by Darin Adler.
WebCore:
The bug was caused by forcedCreateList was not set to true when the start and the end
of the selection lies in the same list. Added selectionHasListOfType to fix this problem.
WebKit used not to convert the outer lists even when the list is fully selected.
Corrected this behavior by converting the entire list at once when the list is fully selected.
To decide whether or not a list is fully selected, added currentSelection argument to doApplyForSingleParagraph.
Tests: editing/execCommand/switch-list-type-with-inner-list.html
editing/execCommand/switch-list-type-with-orphaned-li.html
* editing/InsertListCommand.cpp:
(WebCore::InsertListCommand::mergeWithNeighboringLists): Extracted the code to merge lists.
(WebCore::InsertListCommand::selectionHasListOfType): attachment.cgi
(WebCore::InsertListCommand::doApply): Calls selectionHasListOfType.
(WebCore::InsertListCommand::doApplyForSingleParagraph): See above.
(WebCore::InsertListCommand::listifyParagraph): Calls mergeWithNeighboringLists.
* editing/InsertListCommand.h:
* editing/htmlediting.cpp:
(WebCore::canMergeLists): Ensures lists being merged are instances of HTMLElement.
(WebCore::isNodeVisiblyContainedWithin): Works properly when one end is inside the range.
LayoutTests:
Added a test to convert an unordered list with a nested list to an ordered list.
Also added a test to convert multiple lists with an orphaned list to an ordered list.
The behavior of WebKit is changed not to flatten the lists when inserting ordered/unordered
lists on nested lists.
* editing/execCommand/5207369-expected.txt: No longer flattens the list to match Firefox and MSIE.
* editing/execCommand/5207369.html: Updated.
* editing/execCommand/remove-list-items-expected.txt: Removed one whitespace.
* editing/execCommand/switch-list-type-with-inner-list-expected.txt: Added.
* editing/execCommand/switch-list-type-with-inner-list.html: Added.
* editing/execCommand/switch-list-type-with-orphaned-li-expected.txt: Added.
* editing/execCommand/switch-list-type-with-orphaned-li.html: Added.
git-svn-id: http://svn.webkit.org/repository/webkit/trunk@64337 268f45cc-cd09-0410-ab3c-d52691b4dbfc
diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
index fd85902..2e433af 100644
--- a/LayoutTests/ChangeLog
+++ b/LayoutTests/ChangeLog
@@ -1,3 +1,23 @@
+2010-07-29 Ryosuke Niwa <rniwa at webkit.org>
+
+ Reviewed by Darin Adler.
+
+ InsertOrderedList does not switch the list type properly when it has an inner list.
+ https://bugs.webkit.org/show_bug.cgi?id=43166
+
+ Added a test to convert an unordered list with a nested list to an ordered list.
+ Also added a test to convert multiple lists with an orphaned list to an ordered list.
+ The behavior of WebKit is changed not to flatten the lists when inserting ordered/unordered
+ lists on nested lists.
+
+ * editing/execCommand/5207369-expected.txt: No longer flattens the list to match Firefox and MSIE.
+ * editing/execCommand/5207369.html: Updated.
+ * editing/execCommand/remove-list-items-expected.txt: Removed one whitespace.
+ * editing/execCommand/switch-list-type-with-inner-list-expected.txt: Added.
+ * editing/execCommand/switch-list-type-with-inner-list.html: Added.
+ * editing/execCommand/switch-list-type-with-orphaned-li-expected.txt: Added.
+ * editing/execCommand/switch-list-type-with-orphaned-li.html: Added.
+
2010-07-29 Sheriff Bot <webkit.review.bot at gmail.com>
Unreviewed, rolling out r64313.
diff --git a/LayoutTests/editing/execCommand/5207369-expected.txt b/LayoutTests/editing/execCommand/5207369-expected.txt
index 31c1942..7633850 100644
--- a/LayoutTests/editing/execCommand/5207369-expected.txt
+++ b/LayoutTests/editing/execCommand/5207369-expected.txt
@@ -1,9 +1,9 @@
-This tests for a crash when trying to flatten a list containing a sub-list at the end. You shouldn't get a crash and there should be a single level list below.
+This tests for a crash when trying to switch the list type of an inner list at the end. You shouldn't get a crash. This test used to flatten the list but this behavior was correct to match Firefox and MSIE in the bug 43166.
One
Two
Three
Four
-<ul><li>One</li><li>Two</li><li>Three</li><li>Four</li></ul>
+<ul><li>One</li><li>Two</li><ul><li>Three</li><li>Four</li></ul></ul>
PASS
diff --git a/LayoutTests/editing/execCommand/5207369.html b/LayoutTests/editing/execCommand/5207369.html
index 2caa321..4639d4f 100644
--- a/LayoutTests/editing/execCommand/5207369.html
+++ b/LayoutTests/editing/execCommand/5207369.html
@@ -1,4 +1,4 @@
-<p>This tests for a crash when trying to flatten a list containing a sub-list at the end. You shouldn't get a crash and there should be a single level list below.</p>
+<p>This tests for a crash when trying to switch the list type of an inner list at the end. You shouldn't get a crash. This test used to flatten the list but this behavior was correct to match Firefox and MSIE in the bug 43166.</p>
<div id="div" contenteditable="true"><ol><li>One</li><li>Two</li><ul><li>Three</li><li>Four</li></div>
<p id="console"></p>
diff --git a/LayoutTests/editing/execCommand/remove-list-items-expected.txt b/LayoutTests/editing/execCommand/remove-list-items-expected.txt
index e80ea5a..39cf8a2 100644
--- a/LayoutTests/editing/execCommand/remove-list-items-expected.txt
+++ b/LayoutTests/editing/execCommand/remove-list-items-expected.txt
@@ -38,4 +38,4 @@ This should not be in a list.
bar
This should not be in a list.
This should not be in a list.
-<ol style="border: 1px solid red;"> <li>foo</li> </ol><span id="item1">This should not be a list.</span> <br><ol style="border: 1px solid red;"><br> bar <li>baz</li> </ol>This should not be in a list.<br><ol style="border: 1px solid red;"><li>foo</li> </ol>This should not be in a list.<br><ol style="border: 1px solid red;"> <li>bar</li> </ol> This should not be in a list.<br> This should not be in a list.
+<ol style="border: 1px solid red;"> <li>foo</li> </ol><span id="item1">This should not be a list.</span> <br><ol style="border: 1px solid red;"><br> bar <li>baz</li> </ol>This should not be in a list.<br><ol style="border: 1px solid red;"><li>foo</li> </ol>This should not be in a list.<br><ol style="border: 1px solid red;"> <li>bar</li> </ol> This should not be in a list.<br>This should not be in a list.
diff --git a/LayoutTests/editing/execCommand/switch-list-type-with-inner-list-expected.txt b/LayoutTests/editing/execCommand/switch-list-type-with-inner-list-expected.txt
new file mode 100644
index 0000000..1242fca
--- /dev/null
+++ b/LayoutTests/editing/execCommand/switch-list-type-with-inner-list-expected.txt
@@ -0,0 +1,21 @@
+This tests switching an unordered list with a nested list to an ordered list.
+| "
+"
+| <ol>
+| <li>
+| "<#selection-caret>hello"
+| "
+ "
+| <ol>
+| "
+ "
+| <li>
+| "world"
+| "
+ "
+| "
+ "
+| <li>
+| "webkit"
+| "
+"
diff --git a/LayoutTests/editing/execCommand/switch-list-type-with-inner-list.html b/LayoutTests/editing/execCommand/switch-list-type-with-inner-list.html
new file mode 100644
index 0000000..126deef
--- /dev/null
+++ b/LayoutTests/editing/execCommand/switch-list-type-with-inner-list.html
@@ -0,0 +1,20 @@
+<script src="../../resources/dump-as-markup.js"></script>
+<body>
+<div id="test" contenteditable>
+<ul>
+ <li>hello</li>
+ <ol>
+ <li>world</li>
+ </ol>
+ <li>webkit</li>
+</ul>
+</div>
+<script>
+Markup.description('This tests switching an unordered list with a nested list to an ordered list.');
+Markup.setNodeToDump('test');
+
+var test = document.getElementById('test');
+window.getSelection().selectAllChildren(test);
+document.execCommand('InsertOrderedList', false, null);
+</script>
+</body>
diff --git a/LayoutTests/editing/execCommand/switch-list-type-with-orphaned-li-expected.txt b/LayoutTests/editing/execCommand/switch-list-type-with-orphaned-li-expected.txt
new file mode 100644
index 0000000..ae2d55d
--- /dev/null
+++ b/LayoutTests/editing/execCommand/switch-list-type-with-orphaned-li-expected.txt
@@ -0,0 +1,26 @@
+This tests switching multiple lists with an orphaned list child to an ordered list.
+| "
+"
+| <ol>
+| <li>
+| "<#selection-anchor>webkit"
+| <li>
+| "rocks"
+| "
+ "
+| <li>
+| "because of"
+| "
+ "
+| <li>
+| "you<#selection-focus>"
+| "
+"
+| "
+"
+| "
+"
+| "
+"
+| "
+"
diff --git a/LayoutTests/editing/execCommand/switch-list-type-with-orphaned-li.html b/LayoutTests/editing/execCommand/switch-list-type-with-orphaned-li.html
new file mode 100644
index 0000000..908d1f5
--- /dev/null
+++ b/LayoutTests/editing/execCommand/switch-list-type-with-orphaned-li.html
@@ -0,0 +1,23 @@
+<script src="../../resources/dump-as-markup.js"></script>
+<body>
+<div id="test" contenteditable>
+<ul>
+ <li>webkit</li>
+</ul>
+<li>rocks</li>
+<ul>
+ <li>because of</li>
+</ul>
+<ol>
+ <li>you</li>
+</ol>
+</div>
+<script>
+Markup.description('This tests switching multiple lists with an orphaned list child to an ordered list.');
+Markup.setNodeToDump('test');
+
+var test = document.getElementById('test');
+window.getSelection().selectAllChildren(test);
+document.execCommand('InsertOrderedList', false, null);
+</script>
+</body>
diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
index 0b26493..bc0d8df 100644
--- a/WebCore/ChangeLog
+++ b/WebCore/ChangeLog
@@ -1,3 +1,31 @@
+2010-07-29 Ryosuke Niwa <rniwa at webkit.org>
+
+ Reviewed by Darin Adler.
+
+ InsertOrderedList does not switch the list type properly when it has an inner list.
+ https://bugs.webkit.org/show_bug.cgi?id=43166
+
+ The bug was caused by forcedCreateList was not set to true when the start and the end
+ of the selection lies in the same list. Added selectionHasListOfType to fix this problem.
+
+ WebKit used not to convert the outer lists even when the list is fully selected.
+ Corrected this behavior by converting the entire list at once when the list is fully selected.
+ To decide whether or not a list is fully selected, added currentSelection argument to doApplyForSingleParagraph.
+
+ Tests: editing/execCommand/switch-list-type-with-inner-list.html
+ editing/execCommand/switch-list-type-with-orphaned-li.html
+
+ * editing/InsertListCommand.cpp:
+ (WebCore::InsertListCommand::mergeWithNeighboringLists): Extracted the code to merge lists.
+ (WebCore::InsertListCommand::selectionHasListOfType): attachment.cgi
+ (WebCore::InsertListCommand::doApply): Calls selectionHasListOfType.
+ (WebCore::InsertListCommand::doApplyForSingleParagraph): See above.
+ (WebCore::InsertListCommand::listifyParagraph): Calls mergeWithNeighboringLists.
+ * editing/InsertListCommand.h:
+ * editing/htmlediting.cpp:
+ (WebCore::canMergeLists): Ensures lists being merged are instances of HTMLElement.
+ (WebCore::isNodeVisiblyContainedWithin): Works properly when one end is inside the range.
+
2010-07-29 Martin Robinson <mrobinson at igalia.com>
Reviewed by Dirk Schulze.
diff --git a/WebCore/editing/InsertListCommand.cpp b/WebCore/editing/InsertListCommand.cpp
index 8f9c0de..59a8bce 100644
--- a/WebCore/editing/InsertListCommand.cpp
+++ b/WebCore/editing/InsertListCommand.cpp
@@ -54,6 +54,42 @@ HTMLElement* InsertListCommand::fixOrphanedListChild(Node* node)
return listElement.get();
}
+PassRefPtr<HTMLElement> InsertListCommand::mergeWithNeighboringLists(PassRefPtr<HTMLElement> passedList)
+{
+ RefPtr<HTMLElement> list = passedList;
+ Element* previousList = list->previousElementSibling();
+ if (canMergeLists(previousList, list.get()))
+ mergeIdenticalElements(previousList, list);
+
+ if (!list || !list->nextElementSibling() || !list->nextElementSibling()->isHTMLElement())
+ return list.release();
+
+ RefPtr<HTMLElement> nextList = static_cast<HTMLElement*>(list->nextElementSibling());
+ if (canMergeLists(list.get(), nextList.get())) {
+ mergeIdenticalElements(list, nextList);
+ return nextList.release();
+ }
+ return list.release();
+}
+
+bool InsertListCommand::selectionHasListOfType(const VisibleSelection& selection, const QualifiedName& listTag)
+{
+ VisiblePosition start = selection.visibleStart();
+
+ if (!enclosingList(start.deepEquivalent().node()))
+ return false;
+
+ VisiblePosition end = selection.visibleEnd();
+ while (start.isNotNull() && start != end) {
+ Element* listNode = enclosingList(start.deepEquivalent().node());
+ if (!listNode || !listNode->hasTagName(listTag))
+ return false;
+ start = startOfNextParagraph(start);
+ }
+
+ return true;
+}
+
InsertListCommand::InsertListCommand(Document* document, Type type)
: CompositeEditCommand(document), m_type(type)
{
@@ -80,6 +116,7 @@ void InsertListCommand::doApply()
if (visibleEnd != visibleStart && isStartOfParagraph(visibleEnd))
setEndingSelection(VisibleSelection(visibleStart, visibleEnd.previous(true)));
+ const QualifiedName& listTag = (m_type == OrderedList) ? olTag : ulTag;
if (endingSelection().isRange()) {
VisibleSelection selection = selectionForParagraphIteration(endingSelection());
ASSERT(selection.isRange());
@@ -88,10 +125,9 @@ void InsertListCommand::doApply()
VisiblePosition startOfLastParagraph = startOfParagraph(endOfSelection);
if (startOfParagraph(startOfSelection) != startOfLastParagraph) {
- Node* startList = enclosingList(startOfSelection.deepEquivalent().node());
- Node* endList = enclosingList(endOfSelection.deepEquivalent().node());
- bool forceCreateList = !startList || startList != endList;
+ bool forceCreateList = !selectionHasListOfType(selection, listTag);
+ RefPtr<Range> currentSelection = endingSelection().firstRange();
VisiblePosition startOfCurrentParagraph = startOfSelection;
while (startOfCurrentParagraph != startOfLastParagraph) {
// doApply() may operate on and remove the last paragraph of the selection from the document
@@ -102,7 +138,7 @@ void InsertListCommand::doApply()
if (!startOfLastParagraph.deepEquivalent().node()->inDocument())
return;
setEndingSelection(startOfCurrentParagraph);
- doApplyForSingleParagraph(forceCreateList);
+ doApplyForSingleParagraph(forceCreateList, listTag, currentSelection.get());
// Fetch the start of the selection after moving the first paragraph,
// because moving the paragraph will invalidate the original start.
@@ -114,7 +150,7 @@ void InsertListCommand::doApply()
startOfCurrentParagraph = startOfNextParagraph(endingSelection().visibleStart());
}
setEndingSelection(endOfSelection);
- doApplyForSingleParagraph(forceCreateList);
+ doApplyForSingleParagraph(forceCreateList, listTag, currentSelection.get());
// Fetch the end of the selection, for the reason mentioned above.
endOfSelection = endingSelection().visibleEnd();
setEndingSelection(VisibleSelection(startOfSelection, endOfSelection));
@@ -122,28 +158,44 @@ void InsertListCommand::doApply()
}
}
- doApplyForSingleParagraph(false);
+ doApplyForSingleParagraph(false, listTag, endingSelection().firstRange().get());
}
-void InsertListCommand::doApplyForSingleParagraph(bool forceCreateList)
+void InsertListCommand::doApplyForSingleParagraph(bool forceCreateList, const QualifiedName& listTag, Range* currentSelection)
{
// FIXME: This will produce unexpected results for a selection that starts just before a
// table and ends inside the first cell, selectionForParagraphIteration should probably
// be renamed and deployed inside setEndingSelection().
Node* selectionNode = endingSelection().start().node();
- const QualifiedName listTag = (m_type == OrderedList) ? olTag : ulTag;
Node* listChildNode = enclosingListChild(selectionNode);
bool switchListType = false;
if (listChildNode) {
// Remove the list chlild.
- HTMLElement* listNode = enclosingList(listChildNode);
- if (!listNode)
+ RefPtr<HTMLElement> listNode = enclosingList(listChildNode);
+ if (!listNode) {
listNode = fixOrphanedListChild(listChildNode);
+ listNode = mergeWithNeighboringLists(listNode);
+ }
if (!listNode->hasTagName(listTag))
// listChildNode will be removed from the list and a list of type m_type will be created.
switchListType = true;
- unlistifyParagraph(endingSelection().visibleStart(), listNode, listChildNode);
+ // If the list is of the desired type, and we are not removing the list, then exit early.
+ if (!switchListType && forceCreateList)
+ return;
+
+ // If the entire list is selected, then convert the whole list.
+ if (switchListType && isNodeVisiblyContainedWithin(listNode.get(), currentSelection)) {
+ RefPtr<HTMLElement> newList = createHTMLElement(document(), listTag);
+ insertNodeBefore(newList, listNode);
+ Node* outerBlock = listChildNode->isBlockFlow() ? listChildNode : listNode.get();
+ moveParagraphWithClones(firstPositionInNode(listNode.get()), lastPositionInNode(listNode.get()), newList.get(), outerBlock);
+ newList = mergeWithNeighboringLists(newList);
+ setEndingSelection(VisiblePosition(firstPositionInNode(newList.get())));
+ return;
+ }
+
+ unlistifyParagraph(endingSelection().visibleStart(), listNode.get(), listChildNode);
}
if (!listChildNode || switchListType || forceCreateList)
@@ -285,20 +337,10 @@ PassRefPtr<HTMLElement> InsertListCommand::listifyParagraph(const VisiblePositio
moveParagraph(start, end, VisiblePosition(Position(placeholder.get(), 0)), true);
- // FIXME: listifyParagraph should not depend on a member variable.
- // Since fixOrphanedListChild is the only other method that updates m_listElement,
- // we should fix unlistifyParagraph to support orphaned list child to get rid of this assignment.
- if (!listElement && m_listElement)
- 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))
- mergeIdenticalElements(listElement.get(), nextList);
- } else if (canMergeLists(nextList, previousList))
+ if (listElement)
+ return mergeWithNeighboringLists(listElement);
+
+ if (canMergeLists(previousList, nextList))
mergeIdenticalElements(previousList, nextList);
return listElement;
diff --git a/WebCore/editing/InsertListCommand.h b/WebCore/editing/InsertListCommand.h
index 7fbf936..b81ae74 100644
--- a/WebCore/editing/InsertListCommand.h
+++ b/WebCore/editing/InsertListCommand.h
@@ -52,8 +52,9 @@ private:
virtual EditAction editingAction() const { return EditActionInsertList; }
HTMLElement* fixOrphanedListChild(Node*);
- bool modifyRange();
- void doApplyForSingleParagraph(bool forceCreateList);
+ bool selectionHasListOfType(const VisibleSelection& selection, const QualifiedName&);
+ PassRefPtr<HTMLElement> mergeWithNeighboringLists(PassRefPtr<HTMLElement>);
+ void doApplyForSingleParagraph(bool forceCreateList, const QualifiedName&, Range* currentSelection);
void unlistifyParagraph(const VisiblePosition& originalStart, HTMLElement* listNode, Node* listChildNode);
PassRefPtr<HTMLElement> listifyParagraph(const VisiblePosition& originalStart, const QualifiedName& listTag);
RefPtr<HTMLElement> m_listElement;
diff --git a/WebCore/editing/htmlediting.cpp b/WebCore/editing/htmlediting.cpp
index 9f73167..53b5d7e 100644
--- a/WebCore/editing/htmlediting.cpp
+++ b/WebCore/editing/htmlediting.cpp
@@ -829,7 +829,7 @@ HTMLElement* outermostEnclosingList(Node* node, Node* rootList)
bool canMergeLists(Element* firstList, Element* secondList)
{
- if (!firstList || !secondList)
+ if (!firstList || !secondList || !firstList->isHTMLElement() || !secondList->isHTMLElement())
return false;
return firstList->hasTagName(secondList->tagQName())// make sure the list types match (ol vs. ul)
@@ -1133,9 +1133,15 @@ bool isNodeVisiblyContainedWithin(Node* node, const Range* selectedRange)
if (selectedRange->compareNode(node, ec) == Range::NODE_INSIDE)
return true;
- // If the node starts and ends at where selectedRange starts and ends, the node is contained within
- return visiblePositionBeforeNode(node) == selectedRange->startPosition()
- && visiblePositionAfterNode(node) == selectedRange->endPosition();
+ bool startIsVisuallySame = visiblePositionBeforeNode(node) == selectedRange->startPosition();
+ if (startIsVisuallySame && comparePositions(Position(node->parentNode(), node->nodeIndex()+1), selectedRange->endPosition()) < 0)
+ return true;
+
+ bool endIsVisuallySame = visiblePositionAfterNode(node) == selectedRange->endPosition();
+ if (endIsVisuallySame && comparePositions(selectedRange->startPosition(), Position(node->parentNode(), node->nodeIndex())) < 0)
+ return true;
+
+ return startIsVisuallySame && endIsVisuallySame;
}
bool isRenderedAsNonInlineTableImageOrHR(const Node* node)
--
WebKit Debian packaging
More information about the Pkg-webkit-commits
mailing list