Commit e447ab31 authored by leviw@chromium.org's avatar leviw@chromium.org

2011-03-10 Levi Weintraub <leviw@chromium.org>

        Reviewed by Ryosuke Niwa.

        InsertUnorderedList over a non-editable region and multiple lines enters an infinite loop
        https://bugs.webkit.org/show_bug.cgi?id=53409

        Avoiding crashes and infinite loops when listifying content with mixed-editability

        * editing/execCommand/insert-list-with-noneditable-content-expected.txt: Added.
        * editing/execCommand/insert-list-with-noneditable-content.html: Added.
2011-03-10  Levi Weintraub  <leviw@chromium.org>

        Reviewed by Ryosuke Niwa.

        InsertUnorderedList over a non-editable region and multiple lines enters an infinite loop
        https://bugs.webkit.org/show_bug.cgi?id=53409

        Fixing broken handling of mixed-editability content for InsertListCommand. Previously, if the selection
        spanned non-contenteditable regions, it would get stuck endlessly iterating the same region as the algorithm
        didn't skip the editable boundary.

        Test: editing/execCommand/insert-list-with-noneditable-content.html

        * editing/CompositeEditCommand.cpp:
        (WebCore::CompositeEditCommand::cleanupAfterDeletion): Changed signature to take the destination
        position for the active editing command. Without this, there are cases when the destination happens
        to be a placeholder, and we remove it.
        (WebCore::CompositeEditCommand::moveParagraphs):
        * editing/CompositeEditCommand.h:
        * editing/InsertListCommand.cpp:
        (WebCore::InsertListCommand::doApply): Added logic to the paragraph iteration loop to handle pockets of
        non-editable content in an editable context. Previously, this could cause an infinite loop.
        * editing/visible_units.cpp:
        (WebCore::startOfParagraph): Added a mode of operation where we'll jump across non-editable
        content in the same paragraph to reach the actual editable paragraph start.
        (WebCore::endOfParagraph): Ditto.
        (WebCore::startOfNextParagraph): Now uses the aforementioned non-editable content skipping mode of
        endOfParagraph.


git-svn-id: svn://svn.chromium.org/blink/trunk@80780 bbb929c8-8fbe-4397-9dbb-9b2b20218538
parent 679374e7
2011-03-10 Levi Weintraub <leviw@chromium.org>
Reviewed by Ryosuke Niwa.
InsertUnorderedList over a non-editable region and multiple lines enters an infinite loop
https://bugs.webkit.org/show_bug.cgi?id=53409
Avoiding crashes and infinite loops when listifying content with mixed-editability
* editing/execCommand/insert-list-with-noneditable-content-expected.txt: Added.
* editing/execCommand/insert-list-with-noneditable-content.html: Added.
2011-03-10 Berend-Jan Wever <skylined@chromium.org> 2011-03-10 Berend-Jan Wever <skylined@chromium.org>
Reviewed by Darin Adler. Reviewed by Darin Adler.
This tests list creation in an editable context but across non-editable content. Editable content should be pulled into the list and not crash.
| <ul>
| <li>
| "Editabl<#selection-anchor>e paragraph containing a "
| <span>
| contenteditable="false"
| style="background-color: LightGray;"
| "non-editable"
| " in the middle"
| <br>
| <li>
| "Anothe<#selection-focus>r editable paragraph."
| <br>
<!DOCTYPE html>
<div id="description">This tests list creation in an editable context but across non-editable content. Editable content should be pulled into the list and not crash.</div>
<div contenteditable="true" id="test" style="padding: 1em;">
Editable paragraph containing a <span contenteditable="false" style="background-color: LightGray;">non-editable</span> in the middle<br>
Another editable paragraph.
</div>
<button onclick="insertList();">Insert List</button>
<script src="../../resources/dump-as-markup.js"></script>
<script>
function insertList() {
document.execCommand('insertunorderedlist', false, '');
}
var s = window.getSelection();
var div = document.getElementById("test");
s.setPosition(div.childNodes[0], 10);
s.modify("extend", "forward", "line");
if (window.layoutTestController) {
insertList();
Markup.description(document.getElementById('description').innerText);
Markup.dump(div);
}
</script>
2011-03-10 Levi Weintraub <leviw@chromium.org>
Reviewed by Ryosuke Niwa.
InsertUnorderedList over a non-editable region and multiple lines enters an infinite loop
https://bugs.webkit.org/show_bug.cgi?id=53409
Fixing broken handling of mixed-editability content for InsertListCommand. Previously, if the selection
spanned non-contenteditable regions, it would get stuck endlessly iterating the same region as the algorithm
didn't skip the editable boundary.
Test: editing/execCommand/insert-list-with-noneditable-content.html
* editing/CompositeEditCommand.cpp:
(WebCore::CompositeEditCommand::cleanupAfterDeletion): Changed signature to take the destination
position for the active editing command. Without this, there are cases when the destination happens
to be a placeholder, and we remove it.
(WebCore::CompositeEditCommand::moveParagraphs):
* editing/CompositeEditCommand.h:
* editing/InsertListCommand.cpp:
(WebCore::InsertListCommand::doApply): Added logic to the paragraph iteration loop to handle pockets of
non-editable content in an editable context. Previously, this could cause an infinite loop.
* editing/visible_units.cpp:
(WebCore::startOfParagraph): Added a mode of operation where we'll jump across non-editable
content in the same paragraph to reach the actual editable paragraph start.
(WebCore::endOfParagraph): Ditto.
(WebCore::startOfNextParagraph): Now uses the aforementioned non-editable content skipping mode of
endOfParagraph.
2011-03-10 Berend-Jan Wever <skylined@chromium.org> 2011-03-10 Berend-Jan Wever <skylined@chromium.org>
Reviewed by Darin Adler. Reviewed by Darin Adler.
......
...@@ -22636,6 +22636,7 @@ ...@@ -22636,6 +22636,7 @@
isa = PBXProject; isa = PBXProject;
buildConfigurationList = 149C284308902B11008A9EFC /* Build configuration list for PBXProject "WebCore" */; buildConfigurationList = 149C284308902B11008A9EFC /* Build configuration list for PBXProject "WebCore" */;
compatibilityVersion = "Xcode 2.4"; compatibilityVersion = "Xcode 2.4";
developmentRegion = English;
hasScannedForEncodings = 1; hasScannedForEncodings = 1;
knownRegions = ( knownRegions = (
English, English,
...@@ -830,10 +830,10 @@ void CompositeEditCommand::cloneParagraphUnderNewElement(Position& start, Positi ...@@ -830,10 +830,10 @@ void CompositeEditCommand::cloneParagraphUnderNewElement(Position& start, Positi
// Deleting a paragraph will leave a placeholder. Remove it (and prune // Deleting a paragraph will leave a placeholder. Remove it (and prune
// empty or unrendered parents). // empty or unrendered parents).
void CompositeEditCommand::cleanupAfterDeletion() void CompositeEditCommand::cleanupAfterDeletion(VisiblePosition destination)
{ {
VisiblePosition caretAfterDelete = endingSelection().visibleStart(); VisiblePosition caretAfterDelete = endingSelection().visibleStart();
if (isStartOfParagraph(caretAfterDelete) && isEndOfParagraph(caretAfterDelete)) { if (caretAfterDelete != destination && isStartOfParagraph(caretAfterDelete) && isEndOfParagraph(caretAfterDelete)) {
// Note: We want the rightmost candidate. // Note: We want the rightmost candidate.
Position position = caretAfterDelete.deepEquivalent().downstream(); Position position = caretAfterDelete.deepEquivalent().downstream();
Node* node = position.deprecatedNode(); Node* node = position.deprecatedNode();
...@@ -947,8 +947,8 @@ void CompositeEditCommand::moveParagraphs(const VisiblePosition& startOfParagrap ...@@ -947,8 +947,8 @@ void CompositeEditCommand::moveParagraphs(const VisiblePosition& startOfParagrap
} }
} }
VisiblePosition beforeParagraph = startOfParagraphToMove.previous(); VisiblePosition beforeParagraph = startOfParagraphToMove.previous(CannotCrossEditingBoundary);
VisiblePosition afterParagraph(endOfParagraphToMove.next()); VisiblePosition afterParagraph(endOfParagraphToMove.next(CannotCrossEditingBoundary));
// We upstream() the end and downstream() the start so that we don't include collapsed whitespace in the move. // We upstream() the end and downstream() the start so that we don't include collapsed whitespace in the move.
// When we paste a fragment, spaces after the end and before the start are treated as though they were rendered. // When we paste a fragment, spaces after the end and before the start are treated as though they were rendered.
...@@ -985,8 +985,7 @@ void CompositeEditCommand::moveParagraphs(const VisiblePosition& startOfParagrap ...@@ -985,8 +985,7 @@ void CompositeEditCommand::moveParagraphs(const VisiblePosition& startOfParagrap
deleteSelection(false, false, false, false); deleteSelection(false, false, false, false);
ASSERT(destination.deepEquivalent().anchorNode()->inDocument()); ASSERT(destination.deepEquivalent().anchorNode()->inDocument());
cleanupAfterDeletion(destination);
cleanupAfterDeletion();
ASSERT(destination.deepEquivalent().anchorNode()->inDocument()); ASSERT(destination.deepEquivalent().anchorNode()->inDocument());
// Add a br if pruning an empty block level element caused a collapse. For example: // Add a br if pruning an empty block level element caused a collapse. For example:
......
...@@ -109,7 +109,7 @@ protected: ...@@ -109,7 +109,7 @@ protected:
void moveParagraphs(const VisiblePosition&, const VisiblePosition&, const VisiblePosition&, bool preserveSelection = false, bool preserveStyle = true); void moveParagraphs(const VisiblePosition&, const VisiblePosition&, const VisiblePosition&, bool preserveSelection = false, bool preserveStyle = true);
void moveParagraphWithClones(const VisiblePosition& startOfParagraphToMove, const VisiblePosition& endOfParagraphToMove, Element* blockElement, Node* outerNode); void moveParagraphWithClones(const VisiblePosition& startOfParagraphToMove, const VisiblePosition& endOfParagraphToMove, Element* blockElement, Node* outerNode);
void cloneParagraphUnderNewElement(Position& start, Position& end, Node* outerNode, Element* blockElement); void cloneParagraphUnderNewElement(Position& start, Position& end, Node* outerNode, Element* blockElement);
void cleanupAfterDeletion(); void cleanupAfterDeletion(VisiblePosition destination = VisiblePosition());
bool breakOutOfEmptyListItem(); bool breakOutOfEmptyListItem();
bool breakOutOfEmptyMailBlockquotedParagraph(); bool breakOutOfEmptyMailBlockquotedParagraph();
......
...@@ -121,7 +121,7 @@ void InsertListCommand::doApply() ...@@ -121,7 +121,7 @@ void InsertListCommand::doApply()
// FIXME: We paint the gap before some paragraphs that are indented with left // FIXME: We paint the gap before some paragraphs that are indented with left
// margin/padding, but not others. We should make the gap painting more consistent and // margin/padding, but not others. We should make the gap painting more consistent and
// then use a left margin/padding rule here. // then use a left margin/padding rule here.
if (visibleEnd != visibleStart && isStartOfParagraph(visibleEnd)) if (visibleEnd != visibleStart && isStartOfParagraph(visibleEnd, CanSkipOverEditingBoundary))
setEndingSelection(VisibleSelection(visibleStart, visibleEnd.previous(CannotCrossEditingBoundary))); setEndingSelection(VisibleSelection(visibleStart, visibleEnd.previous(CannotCrossEditingBoundary)));
const QualifiedName& listTag = (m_type == OrderedList) ? olTag : ulTag; const QualifiedName& listTag = (m_type == OrderedList) ? olTag : ulTag;
...@@ -130,14 +130,14 @@ void InsertListCommand::doApply() ...@@ -130,14 +130,14 @@ void InsertListCommand::doApply()
ASSERT(selection.isRange()); ASSERT(selection.isRange());
VisiblePosition startOfSelection = selection.visibleStart(); VisiblePosition startOfSelection = selection.visibleStart();
VisiblePosition endOfSelection = selection.visibleEnd(); VisiblePosition endOfSelection = selection.visibleEnd();
VisiblePosition startOfLastParagraph = startOfParagraph(endOfSelection); VisiblePosition startOfLastParagraph = startOfParagraph(endOfSelection, CanSkipOverEditingBoundary);
if (startOfParagraph(startOfSelection) != startOfLastParagraph) { if (startOfParagraph(startOfSelection, CanSkipOverEditingBoundary) != startOfLastParagraph) {
bool forceCreateList = !selectionHasListOfType(selection, listTag); bool forceCreateList = !selectionHasListOfType(selection, listTag);
RefPtr<Range> currentSelection = endingSelection().firstRange(); RefPtr<Range> currentSelection = endingSelection().firstRange();
VisiblePosition startOfCurrentParagraph = startOfSelection; VisiblePosition startOfCurrentParagraph = startOfSelection;
while (startOfCurrentParagraph != startOfLastParagraph) { while (!inSameParagraph(startOfCurrentParagraph, startOfLastParagraph, CanCrossEditingBoundary)) {
// doApply() may operate on and remove the last paragraph of the selection from the document // doApply() may operate on and remove the last paragraph of the selection from the document
// if it's in the same list item as startOfCurrentParagraph. Return early to avoid an // if it's in the same list item as startOfCurrentParagraph. Return early to avoid an
// infinite loop and because there is no more work to be done. // infinite loop and because there is no more work to be done.
...@@ -162,7 +162,7 @@ void InsertListCommand::doApply() ...@@ -162,7 +162,7 @@ void InsertListCommand::doApply()
if (!lastSelectionRange) if (!lastSelectionRange)
return; return;
endOfSelection = lastSelectionRange->startPosition(); endOfSelection = lastSelectionRange->startPosition();
startOfLastParagraph = startOfParagraph(endOfSelection); startOfLastParagraph = startOfParagraph(endOfSelection, CanSkipOverEditingBoundary);
} }
// Fetch the start of the selection after moving the first paragraph, // Fetch the start of the selection after moving the first paragraph,
...@@ -263,8 +263,8 @@ void InsertListCommand::unlistifyParagraph(const VisiblePosition& originalStart, ...@@ -263,8 +263,8 @@ void InsertListCommand::unlistifyParagraph(const VisiblePosition& originalStart,
previousListChild = listChildNode->previousSibling(); previousListChild = listChildNode->previousSibling();
} else { } else {
// A paragraph is visually a list item minus a list marker. The paragraph will be moved. // A paragraph is visually a list item minus a list marker. The paragraph will be moved.
start = startOfParagraph(originalStart); start = startOfParagraph(originalStart, CanSkipOverEditingBoundary);
end = endOfParagraph(start); end = endOfParagraph(start, CanSkipOverEditingBoundary);
nextListChild = enclosingListChild(end.next().deepEquivalent().deprecatedNode(), listNode); nextListChild = enclosingListChild(end.next().deepEquivalent().deprecatedNode(), listNode);
ASSERT(nextListChild != listChildNode); ASSERT(nextListChild != listChildNode);
previousListChild = enclosingListChild(start.previous().deepEquivalent().deprecatedNode(), listNode); previousListChild = enclosingListChild(start.previous().deepEquivalent().deprecatedNode(), listNode);
...@@ -327,8 +327,8 @@ static Element* adjacentEnclosingList(const VisiblePosition& pos, const VisibleP ...@@ -327,8 +327,8 @@ static Element* adjacentEnclosingList(const VisiblePosition& pos, const VisibleP
PassRefPtr<HTMLElement> InsertListCommand::listifyParagraph(const VisiblePosition& originalStart, const QualifiedName& listTag) PassRefPtr<HTMLElement> InsertListCommand::listifyParagraph(const VisiblePosition& originalStart, const QualifiedName& listTag)
{ {
VisiblePosition start = startOfParagraph(originalStart); VisiblePosition start = startOfParagraph(originalStart, CanSkipOverEditingBoundary);
VisiblePosition end = endOfParagraph(start); VisiblePosition end = endOfParagraph(start, CanSkipOverEditingBoundary);
if (start.isNull() || end.isNull()) if (start.isNull() || end.isNull())
return 0; return 0;
...@@ -375,8 +375,11 @@ PassRefPtr<HTMLElement> InsertListCommand::listifyParagraph(const VisiblePositio ...@@ -375,8 +375,11 @@ PassRefPtr<HTMLElement> InsertListCommand::listifyParagraph(const VisiblePositio
// We inserted the list at the start of the content we're about to move // We inserted the list at the start of the content we're about to move
// Update the start of content, so we don't try to move the list into itself. bug 19066 // Update the start of content, so we don't try to move the list into itself. bug 19066
if (insertionPos == start.deepEquivalent()) // Layout is necessary since start's node's inline renderers may have been destroyed by the insertion
start = startOfParagraph(originalStart); if (insertionPos == start.deepEquivalent()) {
listElement->document()->updateLayoutIgnorePendingStylesheets();
start = startOfParagraph(originalStart, CanSkipOverEditingBoundary);
}
} }
moveParagraph(start, end, positionBeforeNode(placeholder.get()), true); moveParagraph(start, end, positionBeforeNode(placeholder.get()), true);
......
...@@ -751,14 +751,21 @@ VisiblePosition startOfParagraph(const VisiblePosition& c, EditingBoundaryCrossi ...@@ -751,14 +751,21 @@ VisiblePosition startOfParagraph(const VisiblePosition& c, EditingBoundaryCrossi
Node* startBlock = enclosingBlock(startNode); Node* startBlock = enclosingBlock(startNode);
Node *node = startNode; Node* node = startNode;
Node* highestRoot = highestEditableRoot(p);
int offset = p.deprecatedEditingOffset(); int offset = p.deprecatedEditingOffset();
Position::AnchorType type = p.anchorType(); Position::AnchorType type = p.anchorType();
Node *n = startNode; Node* n = startNode;
while (n) { while (n) {
if (boundaryCrossingRule == CannotCrossEditingBoundary && n->isContentEditable() != startNode->isContentEditable()) if (boundaryCrossingRule == CannotCrossEditingBoundary && n->isContentEditable() != startNode->isContentEditable())
break; break;
if (boundaryCrossingRule == CanSkipOverEditingBoundary) {
while (n && n->isContentEditable() != startNode->isContentEditable())
n = n->traversePreviousNodePostOrder(startBlock);
if (!n || !n->isDescendantOf(highestRoot))
break;
}
RenderObject *r = n->renderer(); RenderObject *r = n->renderer();
if (!r) { if (!r) {
n = n->traversePreviousNodePostOrder(startBlock); n = n->traversePreviousNodePostOrder(startBlock);
...@@ -814,16 +821,24 @@ VisiblePosition endOfParagraph(const VisiblePosition &c, EditingBoundaryCrossing ...@@ -814,16 +821,24 @@ VisiblePosition endOfParagraph(const VisiblePosition &c, EditingBoundaryCrossing
return lastDeepEditingPositionForNode(startNode); return lastDeepEditingPositionForNode(startNode);
Node* startBlock = enclosingBlock(startNode); Node* startBlock = enclosingBlock(startNode);
Node *stayInsideBlock = startBlock; Node* stayInsideBlock = startBlock;
Node *node = startNode; Node* node = startNode;
Node* highestRoot = highestEditableRoot(p);
int offset = p.deprecatedEditingOffset(); int offset = p.deprecatedEditingOffset();
Position::AnchorType type = p.anchorType(); Position::AnchorType type = p.anchorType();
Node *n = startNode; Node* n = startNode;
while (n) { while (n) {
if (boundaryCrossingRule == CannotCrossEditingBoundary && n->isContentEditable() != startNode->isContentEditable()) if (boundaryCrossingRule == CannotCrossEditingBoundary && n->isContentEditable() != startNode->isContentEditable())
break; break;
if (boundaryCrossingRule == CanSkipOverEditingBoundary) {
while (n && n->isContentEditable() != startNode->isContentEditable())
n = n->traverseNextNode(stayInsideBlock);
if (!n || !n->isDescendantOf(highestRoot))
break;
}
RenderObject *r = n->renderer(); RenderObject *r = n->renderer();
if (!r) { if (!r) {
n = n->traverseNextNode(stayInsideBlock); n = n->traverseNextNode(stayInsideBlock);
...@@ -866,9 +881,10 @@ VisiblePosition endOfParagraph(const VisiblePosition &c, EditingBoundaryCrossing ...@@ -866,9 +881,10 @@ VisiblePosition endOfParagraph(const VisiblePosition &c, EditingBoundaryCrossing
return VisiblePosition(Position(node, type), DOWNSTREAM); return VisiblePosition(Position(node, type), DOWNSTREAM);
} }
// FIXME: isStartOfParagraph(startOfNextParagraph(pos)) is not always true
VisiblePosition startOfNextParagraph(const VisiblePosition& visiblePosition) VisiblePosition startOfNextParagraph(const VisiblePosition& visiblePosition)
{ {
VisiblePosition paragraphEnd(endOfParagraph(visiblePosition)); VisiblePosition paragraphEnd(endOfParagraph(visiblePosition, CanSkipOverEditingBoundary));
VisiblePosition afterParagraphEnd(paragraphEnd.next(CannotCrossEditingBoundary)); VisiblePosition afterParagraphEnd(paragraphEnd.next(CannotCrossEditingBoundary));
// The position after the last position in the last cell of a table // The position after the last position in the last cell of a table
// is not the start of the next paragraph. // is not the start of the next paragraph.
...@@ -877,9 +893,9 @@ VisiblePosition startOfNextParagraph(const VisiblePosition& visiblePosition) ...@@ -877,9 +893,9 @@ VisiblePosition startOfNextParagraph(const VisiblePosition& visiblePosition)
return afterParagraphEnd; return afterParagraphEnd;
} }
bool inSameParagraph(const VisiblePosition &a, const VisiblePosition &b) bool inSameParagraph(const VisiblePosition &a, const VisiblePosition &b, EditingBoundaryCrossingRule boundaryCrossingRule)
{ {
return a.isNotNull() && startOfParagraph(a) == startOfParagraph(b); return a.isNotNull() && startOfParagraph(a, boundaryCrossingRule) == startOfParagraph(b, boundaryCrossingRule);
} }
bool isStartOfParagraph(const VisiblePosition &pos, EditingBoundaryCrossingRule boundaryCrossingRule) bool isStartOfParagraph(const VisiblePosition &pos, EditingBoundaryCrossingRule boundaryCrossingRule)
......
...@@ -70,7 +70,7 @@ VisiblePosition previousParagraphPosition(const VisiblePosition &, int x); ...@@ -70,7 +70,7 @@ VisiblePosition previousParagraphPosition(const VisiblePosition &, int x);
VisiblePosition nextParagraphPosition(const VisiblePosition &, int x); VisiblePosition nextParagraphPosition(const VisiblePosition &, int x);
bool isStartOfParagraph(const VisiblePosition &, EditingBoundaryCrossingRule = CannotCrossEditingBoundary); bool isStartOfParagraph(const VisiblePosition &, EditingBoundaryCrossingRule = CannotCrossEditingBoundary);
bool isEndOfParagraph(const VisiblePosition &, EditingBoundaryCrossingRule = CannotCrossEditingBoundary); bool isEndOfParagraph(const VisiblePosition &, EditingBoundaryCrossingRule = CannotCrossEditingBoundary);
bool inSameParagraph(const VisiblePosition &, const VisiblePosition &); bool inSameParagraph(const VisiblePosition &, const VisiblePosition &, EditingBoundaryCrossingRule = CannotCrossEditingBoundary);
// blocks (true paragraphs; line break elements don't break blocks) // blocks (true paragraphs; line break elements don't break blocks)
VisiblePosition startOfBlock(const VisiblePosition &); VisiblePosition startOfBlock(const VisiblePosition &);
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment