[Shadow][Editing] applying document.execCommand('bold') twice to elements having...
authorshinyak@chromium.org <shinyak@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 19 Jun 2012 20:13:45 +0000 (20:13 +0000)
committershinyak@chromium.org <shinyak@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 19 Jun 2012 20:13:45 +0000 (20:13 +0000)
https://bugs.webkit.org/show_bug.cgi?id=88502

Reviewed by Ryosuke Niwa.

Source/WebCore:

When inserting or removing insertion points (<shadow> or <content>) into or from Shdaow DOM,
ElementShadow::invalidateDistribution() will be called immediately. It will remove all the renderers
of elements in the Shadow DOM. Since Node::rendererIsEditable() returns false when an renderer does not
exist, all the elements in the Shadow DOM will be considered as non-content-editable until recalculating
layout, though some of them may actually be content-editable.

Actually the current code does not recalculate layout inside editing command, so a disaster happens.
For example, performing an editing command quits before completing all the commands, because the command
thinks it is adding some elements to a non-content-editable element (but actually it's content-editable).

So we have to recalculate layout if necessary when checking an element is content-editable or not.
This can be achieved by using Node::isContentEditable() instead of Node::rendererIsEditable().

Test: editing/shadow/bold-twice-in-shadow.html

* editing/ApplyStyleCommand.cpp:
(WebCore::ApplyStyleCommand::removeInlineStyleFromElement):
* editing/InsertNodeBeforeCommand.cpp:
(WebCore::InsertNodeBeforeCommand::doApply):
(WebCore::InsertNodeBeforeCommand::doUnapply):
* editing/RemoveNodeCommand.cpp:
(WebCore::RemoveNodeCommand::doApply):

LayoutTests:

* editing/shadow/bold-twice-in-shadow-expected.txt: Added.
* editing/shadow/bold-twice-in-shadow.html: Added.

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

LayoutTests/ChangeLog
LayoutTests/editing/shadow/bold-twice-in-shadow-expected.txt [new file with mode: 0644]
LayoutTests/editing/shadow/bold-twice-in-shadow.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/editing/ApplyStyleCommand.cpp
Source/WebCore/editing/InsertNodeBeforeCommand.cpp
Source/WebCore/editing/RemoveNodeCommand.cpp

index cc333c1..9714a95 100644 (file)
@@ -1,3 +1,13 @@
+2012-06-19  Shinya Kawanaka  <shinyak@chromium.org>
+
+        [Shadow][Editing] applying document.execCommand('bold') twice to elements having shadow insertion points causes a crash.
+        https://bugs.webkit.org/show_bug.cgi?id=88502
+
+        Reviewed by Ryosuke Niwa.
+
+        * editing/shadow/bold-twice-in-shadow-expected.txt: Added.
+        * editing/shadow/bold-twice-in-shadow.html: Added.
+
 2012-06-19  Jon Honeycutt  <jhoneycutt@apple.com>
 
         Remove some failing layout tests results added in
diff --git a/LayoutTests/editing/shadow/bold-twice-in-shadow-expected.txt b/LayoutTests/editing/shadow/bold-twice-in-shadow-expected.txt
new file mode 100644 (file)
index 0000000..62d79b3
--- /dev/null
@@ -0,0 +1,6 @@
+Applying document.execCommand('Bold') to elements having insertion points (<shadow> or <content>) shoud not cause a crash.
+
+To test manually, make a selection from somewhere in "nested before" to somehwere in "nested after", then press Ctrl+B twice. It should not cause a crash.
+
+PASS
+
diff --git a/LayoutTests/editing/shadow/bold-twice-in-shadow.html b/LayoutTests/editing/shadow/bold-twice-in-shadow.html
new file mode 100644 (file)
index 0000000..95706d3
--- /dev/null
@@ -0,0 +1,43 @@
+<!DOCTYPE html>
+<html>
+<body>
+<script src="../../fast/dom/shadow/resources/polyfill.js"></script>
+<script src="../../fast/dom/resources/event-sender-util.js"></script>
+
+<p>Applying document.execCommand('Bold') to elements having insertion points (&lt;shadow&gt; or &lt;content&gt;) shoud not cause a crash.</p>
+<p>To test manually, make a selection from somewhere in "nested before" to somehwere in "nested after", then press Ctrl+B twice. It should not cause a crash.</p>
+
+<div id="container" contenteditable>
+    before host
+    <div id="host">    <span contenteditable="false">not editable<span></div>
+    after host
+</div>
+<pre id="console"></pre>
+
+<script>
+if (window.layoutTestController)
+    layoutTestController.dumpAsText();
+
+var shadowRoot = new WebKitShadowRoot(host);
+shadowRoot.innerHTML = '<div id="shadow-host"><span contenteditable>shadow before</span><shadow></shadow>shadow (after)</div>';
+
+var nestedShadowHost = shadowRoot.getElementById('shadow-host');
+var nestedShadowRoot = new WebKitShadowRoot(nestedShadowHost);
+nestedShadowRoot.innerHTML = '<div id="inner" contenteditable>nested before<shadow></shadow>nested after</div>';
+
+if (window.eventSender) {
+    var div = nestedShadowRoot.getElementById('inner');
+
+    eventSender.mouseMoveTo(div.offsetLeft + 5, div.offsetTop + div.offsetHeight / 2);
+    eventSender.mouseDown();
+    eventSender.mouseMoveTo(div.offsetLeft + div.offsetWidth - 5, div.offsetTop + div.offsetHeight / 2);
+    eventSender.mouseUp();
+
+    document.execCommand('Bold');
+    document.execCommand('Bold');
+
+    container.innerHTML = "PASS";
+}
+</script>
+</body>
+</html>
index 9786252..abd0fa0 100644 (file)
@@ -1,3 +1,33 @@
+2012-06-19  Shinya Kawanaka  <shinyak@chromium.org>
+
+        [Shadow][Editing] applying document.execCommand('bold') twice to elements having shadow insertion points causes a crash.
+        https://bugs.webkit.org/show_bug.cgi?id=88502
+
+        Reviewed by Ryosuke Niwa.
+
+        When inserting or removing insertion points (<shadow> or <content>) into or from Shdaow DOM,
+        ElementShadow::invalidateDistribution() will be called immediately. It will remove all the renderers
+        of elements in the Shadow DOM. Since Node::rendererIsEditable() returns false when an renderer does not
+        exist, all the elements in the Shadow DOM will be considered as non-content-editable until recalculating
+        layout, though some of them may actually be content-editable.
+
+        Actually the current code does not recalculate layout inside editing command, so a disaster happens.
+        For example, performing an editing command quits before completing all the commands, because the command
+        thinks it is adding some elements to a non-content-editable element (but actually it's content-editable).
+
+        So we have to recalculate layout if necessary when checking an element is content-editable or not.
+        This can be achieved by using Node::isContentEditable() instead of Node::rendererIsEditable().
+
+        Test: editing/shadow/bold-twice-in-shadow.html
+
+        * editing/ApplyStyleCommand.cpp:
+        (WebCore::ApplyStyleCommand::removeInlineStyleFromElement):
+        * editing/InsertNodeBeforeCommand.cpp:
+        (WebCore::InsertNodeBeforeCommand::doApply):
+        (WebCore::InsertNodeBeforeCommand::doUnapply):
+        * editing/RemoveNodeCommand.cpp:
+        (WebCore::RemoveNodeCommand::doApply):
+
 2012-06-19  Kenneth Russell  <kbr@google.com>
 
         Unreviewed, rolling out r120744.
index 6ff8c44..25a3c4a 100644 (file)
@@ -821,7 +821,7 @@ bool ApplyStyleCommand::removeInlineStyleFromElement(EditingStyle* style, PassRe
 {
     ASSERT(element);
 
-    if (!element->parentNode() || !element->parentNode()->rendererIsEditable())
+    if (!element->parentNode() || !element->parentNode()->isContentEditable())
         return false;
 
     if (isStyledInlineElementToRemove(element.get())) {
index 5b985b5..93213cc 100644 (file)
@@ -48,7 +48,7 @@ InsertNodeBeforeCommand::InsertNodeBeforeCommand(PassRefPtr<Node> insertChild, P
 void InsertNodeBeforeCommand::doApply()
 {
     ContainerNode* parent = m_refChild->parentNode();
-    if (!parent || !parent->rendererIsEditable())
+    if (!parent || !parent->isContentEditable())
         return;
 
     ExceptionCode ec;
@@ -60,7 +60,7 @@ void InsertNodeBeforeCommand::doApply()
 
 void InsertNodeBeforeCommand::doUnapply()
 {
-    if (!m_insertChild->rendererIsEditable())
+    if (!m_insertChild->isContentEditable())
         return;
         
     // Need to notify this before actually deleting the text
index 18c5cd2..19657ac 100644 (file)
@@ -42,7 +42,7 @@ RemoveNodeCommand::RemoveNodeCommand(PassRefPtr<Node> node)
 void RemoveNodeCommand::doApply()
 {
     ContainerNode* parent = m_node->parentNode();
-    if (!parent || !parent->rendererIsEditable())
+    if (!parent || !parent->isContentEditable())
         return;
 
     m_parent = parent;