[Webkit-unassigned] [Bug 24872] Pasting from one bulleted list to another adds an extra level of outdent
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Feb 3 10:38:25 PST 2010
https://bugs.webkit.org/show_bug.cgi?id=24872
--- Comment #3 from Enrica Casucci <enrica at apple.com> 2010-02-03 10:38:24 PST ---
(From update of attachment 47992)
> diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
> index 6b5ab6f..d54a47e 100644
> --- a/LayoutTests/ChangeLog
> +++ b/LayoutTests/ChangeLog
> @@ -1,3 +1,14 @@
> +2010-02-02 Tony Chang <tony at chromium.org>
> +
> + Reviewed by NOBODY (OOPS!).
> +
> + https://bugs.webkit.org/show_bug.cgi?id=24872
> + Add a test to make sure copying from a list and pasting into a list
> + keeps the list at the same indention level rather than nesting.
> +
> + * editing/pasteboard/paste-list-002-expected.txt: Added.
> + * editing/pasteboard/paste-list-002.html: Added.
> +
> 2010-02-02 Eric Seidel <eric at webkit.org>
>
> Reviewed by Gustavo Noronha Silva.
> diff --git a/LayoutTests/editing/pasteboard/paste-list-002-expected.txt b/LayoutTests/editing/pasteboard/paste-list-002-expected.txt
> new file mode 100644
> index 0000000..5ff1cb6
> --- /dev/null
> +++ b/LayoutTests/editing/pasteboard/paste-list-002-expected.txt
> @@ -0,0 +1,13 @@
> +Copy/pasting list items in a list. This test should be run with DRT for pasteboard access.
> +
> +PASS: <li>alpha</li><ul><li>beta</li><li>gamma</li></ul><li>beta</li><li>gamma</li><li id="delta">delta</li><li>beta</li><li>gamma</li><li></li>
> +
> +alpha
> +beta
> +gamma
> +beta
> +gamma
> +delta
> +beta
> +gamma
> +
> diff --git a/LayoutTests/editing/pasteboard/paste-list-002.html b/LayoutTests/editing/pasteboard/paste-list-002.html
> new file mode 100644
> index 0000000..2fae410
> --- /dev/null
> +++ b/LayoutTests/editing/pasteboard/paste-list-002.html
> @@ -0,0 +1,63 @@
> +<html>
> +<script src=../editing.js language="JavaScript" type="text/JavaScript" ></script>
> +<script>
> +
> +function editingTest() {
> + // Select "beta" and "gamma".
> + moveSelectionForwardByLineCommand();
> + for (i = 0; i < 2; i++)
> + extendSelectionForwardByLineCommand();
> + copyCommand();
> +
> + // Paste with the cursor right before "delta".
> + moveSelectionForwardByLineCommand();
> + pasteCommand();
> +
> + // Verify that the cursor is in the right place (still before delta).
> + var selection = window.getSelection();
> + if (selection.baseNode.parentNode != document.getElementById("delta") ||
> + selection.baseOffset != 0 || !selection.isCollapsed)
> + throw "Wrong selection position on before paste.";
> +
> + // Paste with the cursor at the end of "delta".
> + moveSelectionForwardByWordCommand();
> + pasteCommand();
> +
> + // Verify that the cursor is in the right place (new list item after delta).
> + var selection = window.getSelection();
> + if (!selection.isCollapsed || selection.baseNode.value != "")
> + throw "Wrong selection position on after paste.";
> +}
> +
> +</script>
> +<body>
> +<div contentEditable="true">
> +<p>Copy/pasting list items in a list. This test should be run with DRT for pasteboard access.</p>
> +<p id="results">FAIL</p>
> +<ul id="test">
> + <li>alpha</li>
> + <li>beta</li>
> + <li>gamma</li>
> + <li id="delta">delta</li>
> +</ul>
> +</div>
> +
> +<script>
> +if (window.layoutTestController)
> + layoutTestController.dumpAsText();
> +
> +var elem = document.getElementById("test");
> +var selection = window.getSelection();
> +selection.setPosition(elem, 0);
> +editingTest();
> +
> +// Rerun the test but have the source list be indented.
> +document.getElementById("test").innerHTML = "<li>alpha</li><ul><li>beta</li><li>gamma</li></ul><li id='delta'>delta</li>";
> +selection.setPosition(elem, 0);
> +editingTest();
> +
> +document.getElementById("results").innerText = "PASS: " + document.getElementById("test").innerHTML;
> +</script>
> +
> +</body>
> +</html>
> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> index 67f5b56..6f365d0 100644
> --- a/WebCore/ChangeLog
> +++ b/WebCore/ChangeLog
> @@ -1,3 +1,30 @@
> +2010-02-02 Tony Chang <tony at chromium.org>
> +
> + Reviewed by NOBODY (OOPS!).
> +
> + https://bugs.webkit.org/show_bug.cgi?id=24872
> + When pasting a list into another list should not indent another level.
> + If the cursor is at the beginning of the line, it should insert the
> + list items before the current list item. If the cursor is at the end
> + of the line, it should insert the list items after the current list item.
> +
> + This matches Firefox and IE and makes the common activity of reordering a list
> + work as expected.
> +
> + This also adds a small helper method (isListItem) to htmlediting.h.
> +
> + Test: editing/pasteboard/paste-list-002.html
> +
> + * editing/ReplaceSelectionCommand.cpp:
> + (WebCore::ReplaceSelectionCommand::doApply):
> + (WebCore::ReplaceSelectionCommand::insertNodeAtAndUpdateNodesInserted):
> + (WebCore::ReplaceSelectionCommand::insertAsListItems):
> + * editing/ReplaceSelectionCommand.h:
> + * editing/htmlediting.cpp:
> + (WebCore::isListItem):
> + (WebCore::appendedSublist):
> + * editing/htmlediting.h:
> +
> 2010-02-02 Steve Falkenburg <sfalken at apple.com>
>
> Reviewed by Darin Adler.
> diff --git a/WebCore/editing/ReplaceSelectionCommand.cpp b/WebCore/editing/ReplaceSelectionCommand.cpp
> index 85a4471..77d69d1 100644
> --- a/WebCore/editing/ReplaceSelectionCommand.cpp
> +++ b/WebCore/editing/ReplaceSelectionCommand.cpp
> @@ -752,9 +752,7 @@ void ReplaceSelectionCommand::doApply()
> bool startIsInsideMailBlockquote = nearestMailBlockquote(insertionPos.node());
>
> if ((selectionStartWasStartOfParagraph && selectionEndWasEndOfParagraph && !startIsInsideMailBlockquote) ||
> - startBlock == currentRoot ||
> - (startBlock && startBlock->renderer() && startBlock->renderer()->isListItem()) ||
> - selectionIsPlainText)
> + startBlock == currentRoot || isListItem(startBlock) || selectionIsPlainText)
> m_preventNesting = false;
>
> if (selection.isRange()) {
> @@ -871,7 +869,7 @@ void ReplaceSelectionCommand::doApply()
> RefPtr<Node> node = refNode->nextSibling();
>
> fragment.removeNode(refNode);
> - insertNodeAtAndUpdateNodesInserted(refNode, insertionPos);
> + refNode = insertNodeAtAndUpdateNodesInserted(refNode, insertionPos);
>
> // Mutation events (bug 22634) may have already removed the inserted content
> if (!refNode->inDocument())
> @@ -960,9 +958,15 @@ void ReplaceSelectionCommand::doApply()
> if (selectionEndWasEndOfParagraph || !isEndOfParagraph(endOfInsertedContent) || next.isNull()) {
> if (!isStartOfParagraph(endOfInsertedContent)) {
> setEndingSelection(endOfInsertedContent);
> - // Use a default paragraph element (a plain div) for the empty paragraph, using the last paragraph
> - // block's style seems to annoy users.
> - insertParagraphSeparator(true);
> + Node* enclosingNode = enclosingBlock(endOfInsertedContent.deepEquivalent().node());
> + if (isListItem(enclosingNode)) {
> + RefPtr<Node> newListItem = createListItemElement(document());
> + insertNodeAfter(newListItem, enclosingNode);
> + setEndingSelection(VisiblePosition(Position(newListItem, 0)));
> + } else
> + // Use a default paragraph element (a plain div) for the empty paragraph, using the last paragraph
> + // block's style seems to annoy users.
> + insertParagraphSeparator(true);
>
> // Select up to the paragraph separator that was added.
> lastPositionToSelect = endingSelection().visibleStart().deepEquivalent();
> @@ -1097,11 +1101,16 @@ void ReplaceSelectionCommand::insertNodeAfterAndUpdateNodesInserted(PassRefPtr<N
> updateNodesInserted(nodeToUpdate);
> }
>
> -void ReplaceSelectionCommand::insertNodeAtAndUpdateNodesInserted(PassRefPtr<Node> insertChild, const Position& p)
> +Node* ReplaceSelectionCommand::insertNodeAtAndUpdateNodesInserted(PassRefPtr<Node> insertChild, const Position& p)
> {
> Node* nodeToUpdate = insertChild.get(); // insertChild will be cleared when passed
> + Node* blockStart = enclosingBlock(p.node());
> + if (isListElement(insertChild.get()) && blockStart->renderer()->isListItem())
> + return insertAsListItems(insertChild, blockStart, p);
> +
> insertNodeAt(insertChild, p);
> updateNodesInserted(nodeToUpdate);
> + return nodeToUpdate;
> }
>
> void ReplaceSelectionCommand::insertNodeBeforeAndUpdateNodesInserted(PassRefPtr<Node> insertChild, Node* refChild)
> @@ -1111,6 +1120,39 @@ void ReplaceSelectionCommand::insertNodeBeforeAndUpdateNodesInserted(PassRefPtr<
> updateNodesInserted(nodeToUpdate);
> }
>
> +// If the user is inserting a list into an existing list, instead of nesting the list,
> +// we put the list items into the existing list.
> +Node* ReplaceSelectionCommand::insertAsListItems(PassRefPtr<Node> listElement, Node* insertionNode, const Position& p)
> +{
> + while (listElement->hasChildNodes() && isListElement(listElement->firstChild()) && listElement->childNodeCount() == 1)
> + listElement = listElement->firstChild();
> +
> + bool isStart = isStartOfParagraph(p);
> + bool isEnd = isEndOfParagraph(p);
> +
> + Node* lastNode = insertionNode;
> + while (RefPtr<Node> listItem = listElement->firstChild()) {
> + ExceptionCode ec = 0;
> + listElement->removeChild(listItem.get(), ec);
> + ASSERT(!ec);
> + if (isStart)
> + insertNodeBefore(listItem, lastNode);
> + else if (isEnd) {
> + insertNodeAfter(listItem, lastNode);
> + lastNode = listItem.get();
> + } else {
> + // FIXME: If we're in the middle of a list item, we should split it into two separate
> + // list items and insert these nodes between them. For now, just append the nodes.
> + insertNodeAfter(listItem, lastNode);
> + lastNode = listItem.get();
> + }
> + }
> + if (isStart)
> + lastNode = lastNode->previousSibling();
> + updateNodesInserted(lastNode);
> + return lastNode;
> +}
> +
> void ReplaceSelectionCommand::updateNodesInserted(Node *node)
> {
> if (!node)
> diff --git a/WebCore/editing/ReplaceSelectionCommand.h b/WebCore/editing/ReplaceSelectionCommand.h
> index 1cb93c3..b7ff857 100644
> --- a/WebCore/editing/ReplaceSelectionCommand.h
> +++ b/WebCore/editing/ReplaceSelectionCommand.h
> @@ -52,8 +52,9 @@ private:
> void completeHTMLReplacement(const Position& lastPositionToSelect);
>
> void insertNodeAfterAndUpdateNodesInserted(PassRefPtr<Node> insertChild, Node* refChild);
> - void insertNodeAtAndUpdateNodesInserted(PassRefPtr<Node>, const Position&);
> + Node* insertNodeAtAndUpdateNodesInserted(PassRefPtr<Node>, const Position&);
> void insertNodeBeforeAndUpdateNodesInserted(PassRefPtr<Node> insertChild, Node* refChild);
> + Node* insertAsListItems(PassRefPtr<Node>, Node* insertionNode, const Position&);
>
> void updateNodesInserted(Node*);
> bool shouldRemoveEndBR(Node*, const VisiblePosition&);
> diff --git a/WebCore/editing/htmlediting.cpp b/WebCore/editing/htmlediting.cpp
> index b58dff3..4981f63 100644
> --- a/WebCore/editing/htmlediting.cpp
> +++ b/WebCore/editing/htmlediting.cpp
> @@ -658,6 +658,11 @@ bool isListElement(Node *n)
> return (n && (n->hasTagName(ulTag) || n->hasTagName(olTag) || n->hasTagName(dlTag)));
> }
>
> +bool isListItem(Node *n)
> +{
> + return n && n->renderer() && n->renderer()->isListItem();
> +}
> +
> Node* enclosingNodeWithTag(const Position& p, const QualifiedName& tagName)
> {
> if (p.isNull())
> @@ -779,7 +784,7 @@ static Node* appendedSublist(Node* listItem)
> for (Node* n = listItem->nextSibling(); n; n = n->nextSibling()) {
> if (isListElement(n))
> return static_cast<HTMLElement*>(n);
> - if (n->renderer() && n->renderer()->isListItem())
> + if (isListItem(listItem))
> return 0;
> }
>
> diff --git a/WebCore/editing/htmlediting.h b/WebCore/editing/htmlediting.h
> index c5a44ac..ef3db35 100644
> --- a/WebCore/editing/htmlediting.h
> +++ b/WebCore/editing/htmlediting.h
> @@ -90,6 +90,7 @@ bool isTableCell(const Node*);
> bool isEmptyTableCell(const Node*);
> bool isTableStructureNode(const Node*);
> bool isListElement(Node*);
> +bool isListItem(Node*);
> bool isNodeRendered(const Node*);
> bool isNodeVisiblyContainedWithin(Node*, const Range*);
> bool isRenderedAsNonInlineTableImageOrHR(const Node*);
The approach looks ok to me. I don't like very much the fact that you've
changed insertNodeAndUpdateNodesInserted to add a return value that is used
only at the beginning of the loop.
I'd rather see that method stay the same and add another for the specific
purpose. I believe it makes it more clear, it is already such a complex piece
of code :-).
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list