InsertUnorderedList can lead to lost content and assertions in moveParagraphs
authorleviw@chromium.org <leviw@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 7 Mar 2013 00:07:02 +0000 (00:07 +0000)
committerleviw@chromium.org <leviw@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 7 Mar 2013 00:07:02 +0000 (00:07 +0000)
https://bugs.webkit.org/show_bug.cgi?id=111228

Reviewed by Ryosuke Niwa.

Source/WebCore:

When a list is wrapped in a presentational inline like a b tag, we'd create markup that included
everything up to the b tag from moveParagraphs when intending to only move the contents of the
list item. This could make it impossible to remove content from a list and trigger loss of
editable text.

Test: editing/execCommand/insert-remove-block-list-inside-presentational-inline.html

* editing/EditingStyle.cpp:
(WebCore::elementMatchesAndPropertyIsNotInInlineStyleDecl): Ensure there's an inline style
before calling propertyExistsInStyle.
(WebCore::HTMLElementEquivalent::propertyExistsInStyle): Removing null check for style.
All callers ensure this value isn't null.
* editing/markup.cpp:
(WebCore::highestAncestorToWrapMarkup): Avoid going over RenderBlocks when finding the highest
presentational ancestor to avoid leaving the bounds of the original paragraph.

LayoutTests:

* editing/deleting/pruning-after-merge-1-expected.txt:
* editing/execCommand/insert-remove-block-list-inside-presentational-inline-expected.txt: Added.
* editing/execCommand/insert-remove-block-list-inside-presentational-inline.html: Added.
* editing/pasteboard/paste-and-sanitize-expected.txt:
* editing/pasteboard/paste-and-sanitize.html:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@144995 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/editing/deleting/pruning-after-merge-1-expected.txt
LayoutTests/editing/execCommand/insert-remove-block-list-inside-presentational-inline-expected.txt [new file with mode: 0644]
LayoutTests/editing/execCommand/insert-remove-block-list-inside-presentational-inline.html [new file with mode: 0644]
LayoutTests/editing/pasteboard/paste-and-sanitize-expected.txt
LayoutTests/editing/pasteboard/paste-and-sanitize.html
Source/WebCore/ChangeLog
Source/WebCore/editing/EditingStyle.cpp
Source/WebCore/editing/markup.cpp

index e045e21..9033988 100644 (file)
@@ -1,3 +1,16 @@
+2013-03-06  Levi Weintraub  <leviw@chromium.org>
+
+        InsertUnorderedList can lead to lost content and assertions in moveParagraphs
+        https://bugs.webkit.org/show_bug.cgi?id=111228
+
+        Reviewed by Ryosuke Niwa.
+
+        * editing/deleting/pruning-after-merge-1-expected.txt:
+        * editing/execCommand/insert-remove-block-list-inside-presentational-inline-expected.txt: Added.
+        * editing/execCommand/insert-remove-block-list-inside-presentational-inline.html: Added.
+        * editing/pasteboard/paste-and-sanitize-expected.txt:
+        * editing/pasteboard/paste-and-sanitize.html:
+
 2013-03-06  Rafael Weinstein  <rafaelw@chromium.org>
 
         Unreviewed gardening.
index ed2559e..c3063d8 100644 (file)
@@ -5,7 +5,7 @@ EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotificatio
 EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
 EDITING DELEGATE: shouldDeleteDOMRange:range from 1 of #text > DIV > BODY > HTML > #document to 1 of #text > DIV > B > DIV > BODY > HTML > #document
 EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
-EDITING DELEGATE: shouldChangeSelectedDOMRange:range from 0 of #text > DIV > B > DIV > BODY > HTML > #document to 2 of #text > DIV > B > DIV > BODY > HTML > #document toDOMRange:range from 1 of #text > DIV > BODY > HTML > #document to 1 of #text > DIV > BODY > HTML > #document affinity:NSSelectionAffinityDownstream stillSelecting:FALSE
+EDITING DELEGATE: shouldChangeSelectedDOMRange:range from 0 of #text > SPAN > DIV > BODY > HTML > #document to 2 of #text > SPAN > DIV > BODY > HTML > #document toDOMRange:range from 1 of #text > DIV > BODY > HTML > #document to 1 of #text > DIV > BODY > HTML > #document affinity:NSSelectionAffinityDownstream stillSelecting:FALSE
 EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
 EDITING DELEGATE: webViewDidChange:WebViewDidChangeNotification
 This tests the pruning that delete does when the it merges two paragraphs when the selection to delete spans multiple blocks.
diff --git a/LayoutTests/editing/execCommand/insert-remove-block-list-inside-presentational-inline-expected.txt b/LayoutTests/editing/execCommand/insert-remove-block-list-inside-presentational-inline-expected.txt
new file mode 100644 (file)
index 0000000..7af0a9d
--- /dev/null
@@ -0,0 +1,10 @@
+This test verifies insertUnorderedList can properly undo its own DOM manipulation.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS successfullyParsed is true
+
+TEST COMPLETE
+PASS document.getElementsByTagName('ul').length == 0 is true
+
diff --git a/LayoutTests/editing/execCommand/insert-remove-block-list-inside-presentational-inline.html b/LayoutTests/editing/execCommand/insert-remove-block-list-inside-presentational-inline.html
new file mode 100644 (file)
index 0000000..6334718
--- /dev/null
@@ -0,0 +1,38 @@
+<!DOCTYPE html>
+<html>
+<head>
+<script src="../../fast/js/resources/js-test-pre.js"></script>
+</head>
+<body onload="runTest();">
+<div id="console"></div>
+<div contenteditable="true" id="root"><b>one</b><div><b>two</b></div><div><b>three</b></div><div><br></div></div>
+<script>
+var root = document.getElementById("root");
+function toggleList() {
+    var temporaryBlock = document.createElement('div');
+    temporaryBlock.style.cssText =  "height: 0";
+    temporaryBlock.appendChild(document.createTextNode('x'));
+    root.insertBefore(temporaryBlock, root.firstChild);
+    document.execCommand('insertUnorderedList');
+    root.removeChild(temporaryBlock);
+}
+
+function runTest() {
+    if (window.testRunner)
+        testRunner.dumpAsText();
+
+    description("This test verifies insertUnorderedList can properly undo its own DOM manipulation.");
+    root.focus();
+    document.execCommand("SelectAll");
+    toggleList();
+    toggleList();
+    toggleList();
+    toggleList();
+    shouldBeTrue("document.getElementsByTagName('ul').length == 0");
+
+    if (window.testRunner)
+        root.parentNode.removeChild(root);
+}
+</script>
+<script src="../../fast/js/resources/js-test-post.js"></script>
+</body>
index 53df6e5..af89dcf 100644 (file)
@@ -4,7 +4,7 @@ PASS confirmedMarkup is 'Hello'
 PASS confirmedMarkup is '<b><i>Hello</i></b>'
 PASS confirmedMarkup is '<b><i>Hello</i></b>'
 PASS confirmedMarkup is 'Hello'
-PASS confirmedMarkup is '<b><i>Hello</i></b>'
+PASS confirmedMarkup is '<i style="font-weight: bold;">Hello</i>'
 PASS confirmedMarkup is '<div style="text-align: center;"><b>Hello</b></div>'
 PASS confirmedMarkup is '<div><b><i>hello</i></b></div><div><b><i>world</i></b></div>'
 PASS confirmedMarkup is '<b><i><span style="font-weight: normal;"><b><i>hello1</i></b><b><i>&nbsp;hello2</i></b></span></i></b>'
index 7a1dc44..6ae50e5 100644 (file)
@@ -38,7 +38,7 @@ testPaste("div", "Hello", "Hello");
 testPaste("div", "<b><i>Hello</i></b>", "<b><i>Hello</i></b>");
 testPaste("div", "<div><b><i><span style=\"font-weight: normal\"><b><i>Hello</i></b></span></i></b></div>", "<b><i>Hello</i></b>");
 testPaste("div", "<div><div><div>Hello</div></div></div>", "Hello");
-testPaste("div", "<div><b><div><i>Hello</i></div></b></div>", "<b><i>Hello</i></b>");
+testPaste("div", "<div><b><div><i>Hello</i></div></b></div>", "<i style=\"font-weight: bold;\">Hello</i>");
 testPaste("div", "<div><div style=\"text-align: center;\"><b>Hello</b></div></div>", "<div style=\"text-align: center;\"><b>Hello</b></div>");
 testPaste("div", "<div><b><i><span style=\"font-weight: normal\"><b><i>hello</i></b></span></i></b></div><div><b><i><span style=\"font-weight: normal\"><b><i>world</i></b></span></i></b></div>", 
           "<div><b><i>hello</i></b></div><div><b><i>world</i></b></div>");
index 045fccc..af4d214 100644 (file)
@@ -1,3 +1,26 @@
+2013-03-06  Levi Weintraub  <leviw@chromium.org>
+
+        InsertUnorderedList can lead to lost content and assertions in moveParagraphs
+        https://bugs.webkit.org/show_bug.cgi?id=111228
+
+        Reviewed by Ryosuke Niwa.
+
+        When a list is wrapped in a presentational inline like a b tag, we'd create markup that included
+        everything up to the b tag from moveParagraphs when intending to only move the contents of the
+        list item. This could make it impossible to remove content from a list and trigger loss of
+        editable text.
+
+        Test: editing/execCommand/insert-remove-block-list-inside-presentational-inline.html
+
+        * editing/EditingStyle.cpp:
+        (WebCore::elementMatchesAndPropertyIsNotInInlineStyleDecl): Ensure there's an inline style
+        before calling propertyExistsInStyle.
+        (WebCore::HTMLElementEquivalent::propertyExistsInStyle): Removing null check for style.
+        All callers ensure this value isn't null.
+        * editing/markup.cpp:
+        (WebCore::highestAncestorToWrapMarkup): Avoid going over RenderBlocks when finding the highest
+        presentational ancestor to avoid leaving the bounds of the original paragraph.
+
 2013-03-06  Adam Klein  <adamk@chromium.org>
 
         [V8] Use implicit references instead of object groups to keep registered MutationObservers alive
index 7cbd5fa..b348f60 100644 (file)
@@ -128,7 +128,7 @@ public:
     virtual ~HTMLElementEquivalent() { }
     virtual bool matches(const Element* element) const { return !m_tagName || element->hasTagName(*m_tagName); }
     virtual bool hasAttribute() const { return false; }
-    virtual bool propertyExistsInStyle(const StylePropertySet* style) const { return style && style->getPropertyCSSValue(m_propertyID); }
+    virtual bool propertyExistsInStyle(const StylePropertySet* style) const { return style->getPropertyCSSValue(m_propertyID); }
     virtual bool valueIsPresentInStyle(Element*, StylePropertySet*) const;
     virtual void addToStyle(Element*, EditingStyle*) const;
 
@@ -974,7 +974,7 @@ void EditingStyle::mergeInlineStyleOfElement(StyledElement* element, CSSProperty
 static inline bool elementMatchesAndPropertyIsNotInInlineStyleDecl(const HTMLElementEquivalent* equivalent, const StyledElement* element,
     EditingStyle::CSSPropertyOverrideMode mode, StylePropertySet* style)
 {
-    return equivalent->matches(element) && !equivalent->propertyExistsInStyle(element->inlineStyle())
+    return equivalent->matches(element) && (!element->inlineStyle() || !equivalent->propertyExistsInStyle(element->inlineStyle()))
         && (mode == EditingStyle::OverrideValues || !equivalent->propertyExistsInStyle(style));
 }
 
index 3c3a570..164ee0d 100644 (file)
@@ -56,6 +56,7 @@
 #include "MarkupAccumulator.h"
 #include "NodeTraversal.h"
 #include "Range.h"
+#include "RenderBlock.h"
 #include "RenderObject.h"
 #include "Settings.h"
 #include "StylePropertySet.h"
@@ -522,7 +523,7 @@ static Node* highestAncestorToWrapMarkup(const Range* range, EAnnotateForInterch
 
     Node* checkAncestor = specialCommonAncestor ? specialCommonAncestor : commonAncestor;
     if (checkAncestor->renderer()) {
-        Node* newSpecialCommonAncestor = highestEnclosingNodeOfType(firstPositionInNode(checkAncestor), &isElementPresentational);
+        Node* newSpecialCommonAncestor = highestEnclosingNodeOfType(firstPositionInNode(checkAncestor), &isElementPresentational, CanCrossEditingBoundary, checkAncestor->renderer()->containingBlock()->node());
         if (newSpecialCommonAncestor)
             specialCommonAncestor = newSpecialCommonAncestor;
     }