2010-10-15 Ryosuke Niwa <rniwa@webkit.org>
authorrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 16 Oct 2010 04:10:26 +0000 (04:10 +0000)
committerrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 16 Oct 2010 04:10:26 +0000 (04:10 +0000)
        Reviewed by Darin Adler.

        unlink removes inline style of anchor elements
        https://bugs.webkit.org/show_bug.cgi?id=47424

        The bug was caused by our not extracting styles when the anchor element is removed by removeInlineStyle.
        Because we removed the element without pushing its inline style down, we lost style information.

        Fixed the bug by pushing down styles in removeInlineStyle as done in pushDownInlineStyleAroundNode.
        Also fixed a bug in addInlineStyleIfNeeded where StyleChange incorrectly removed styles of the container
        node as supposed to that of startNode from the style to apply when startNode is not an element.

        Test: editing/execCommand/toggle-unlink.html

        * editing/ApplyStyleCommand.cpp:
        (WebCore::ApplyStyleCommand::removeInlineStyleFromElement): To prevent a similar bug in the future,
        added an assert that extractedStyle is specified whenever we're removing a styled element.
        (WebCore::ApplyStyleCommand::removeInlineStyle):
        (WebCore::ApplyStyleCommand::addInlineStyleIfNeeded):
2010-10-15  Ryosuke Niwa  <rniwa@webkit.org>

        Reviewed by Darin Adler.

        unlink removes inline style of anchor elements
        https://bugs.webkit.org/show_bug.cgi?id=47424

        Added a test to ensure execCommand('unlink', false, null) preserves the inline style declaration.

        * editing/execCommand/script-tests/toggle-unlink.js: Added.
        (testSingleToggle):
        (selectAll):
        (selectFirstTwoWords):
        (selectLastTwoWords):
        (selectLastWord):
        * editing/execCommand/toggle-unlink-expected.txt: Added.
        * editing/execCommand/toggle-unlink.html: Added.
        * editing/style/script-tests/make-text-writing-direction-inline.js: Specifies both direction
        and unicode-bidi properties instead of just direction property.

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

LayoutTests/ChangeLog
LayoutTests/editing/execCommand/script-tests/toggle-unlink.js [new file with mode: 0644]
LayoutTests/editing/execCommand/toggle-unlink-expected.txt [new file with mode: 0644]
LayoutTests/editing/execCommand/toggle-unlink.html [new file with mode: 0644]
LayoutTests/editing/style/script-tests/make-text-writing-direction-inline.js
WebCore/ChangeLog
WebCore/editing/ApplyStyleCommand.cpp

index bcb097a..11f4674 100644 (file)
@@ -1,3 +1,23 @@
+2010-10-15  Ryosuke Niwa  <rniwa@webkit.org>
+
+        Reviewed by Darin Adler.
+
+        unlink removes inline style of anchor elements
+        https://bugs.webkit.org/show_bug.cgi?id=47424
+
+        Added a test to ensure execCommand('unlink', false, null) preserves the inline style declaration.
+
+        * editing/execCommand/script-tests/toggle-unlink.js: Added.
+        (testSingleToggle):
+        (selectAll):
+        (selectFirstTwoWords):
+        (selectLastTwoWords):
+        (selectLastWord):
+        * editing/execCommand/toggle-unlink-expected.txt: Added.
+        * editing/execCommand/toggle-unlink.html: Added.
+        * editing/style/script-tests/make-text-writing-direction-inline.js: Specifies both direction
+        and unicode-bidi properties instead of just direction property.
+
 2010-10-15  Kinuko Yasuda  <kinuko@chromium.org>
 
         Reviewed by Jian Li.
diff --git a/LayoutTests/editing/execCommand/script-tests/toggle-unlink.js b/LayoutTests/editing/execCommand/script-tests/toggle-unlink.js
new file mode 100644 (file)
index 0000000..bcf133d
--- /dev/null
@@ -0,0 +1,68 @@
+description("Test to make sure we preserve styles when removing links")
+
+var testContainer = document.createElement("div");
+testContainer.contentEditable = true;
+document.body.appendChild(testContainer);
+
+function testSingleToggle(toggleCommand, initialContents, selector, expectedContents)
+{
+    testContainer.innerHTML = initialContents;
+    var selected = selector(testContainer);
+    document.execCommand(toggleCommand, false, 'http://webkit.org/');
+    action = toggleCommand + ' on ' + selected + ' of "' + initialContents + '" yields "' + testContainer.innerHTML + '"';
+    if (testContainer.innerHTML === expectedContents)
+        testPassed(action);
+    else
+        testFailed(action + ', expected "' + expectedContents + '"');
+}
+
+function selectAll(container) {
+    window.getSelection().selectAllChildren(container);
+    return 'all';
+}
+
+function selectFirstTwoWords(container) {
+    window.getSelection().setPosition(container, 0);
+    window.getSelection().modify('extend', 'forward', 'word');
+    window.getSelection().modify('extend', 'forward', 'word');
+    return 'first two words';
+}
+
+function selectLastTwoWords(container) {
+    window.getSelection().setPosition(container, container.childNodes.length);
+    window.getSelection().modify('extend', 'backward', 'word');
+    window.getSelection().modify('extend', 'backward', 'word');
+    return 'last two words';
+}
+
+function selectLastWord(container) {
+    window.getSelection().setPosition(container, container.childNodes.length);
+    window.getSelection().modify('extend', 'backward', 'word');
+    return 'last word';
+}
+
+testSingleToggle("unlink", 'hello <b>world</b>', selectAll, 'hello <b>world</b>');
+testSingleToggle("unlink", '<a href="http://webkit.org/"><u>hello world</u></a>', selectAll, '<u>hello world</u>');
+testSingleToggle("unlink", 'hello <i><a href="http://webkit.org/">world</a></i>', selectAll, 'hello <i>world</i>');
+testSingleToggle("unlink", 'hello <a href="http://webkit.org/" style="font-weight: bold;">world</a>', selectAll, 'hello <b>world</b>');
+testSingleToggle("unlink", 'hello <a href="http://webkit.org/" style="color: blue;">world</a> WebKit', selectAll, 'hello <font class="Apple-style-span" color="#0000FF">world</font> WebKit');
+testSingleToggle("unlink", 'hello <a href="http://webkit.org/" style="color: blue; display: block;">world</a> WebKit', selectAll, 'hello <font class="Apple-style-span" color="#0000FF"><span class="Apple-style-span" style="display: block;">world</span></font> WebKit');
+testSingleToggle("unlink", '<a href="http://webkit.org/" style="font-size: large;">hello world</a> WebKit',
+    selectLastTwoWords, '<a href="http://webkit.org/" style="font-size: large;">hello </a><font class="Apple-style-span" size="4">world</font> WebKit');
+testSingleToggle("unlink", 'hello <a href="http://webkit.org/" style="font-size: large;">world <span style="font-size: small; ">WebKit</span> rocks</a>',
+    selectLastTwoWords, 'hello <a href="http://webkit.org/"><font class="Apple-style-span" size="4">world </font></a><span style="font-size: small; ">WebKit</span><font class="Apple-style-span" size="4"> rocks</font>');
+testSingleToggle("unlink", 'hello <a href="http://webkit.org/" style="font-style: italic;"><b>world</b> WebKit</a>',
+    selectFirstTwoWords, 'hello <b style="font-style: italic; ">world</b><a href="http://webkit.org/"><i> WebKit</i></a>');
+
+testSingleToggle("unlink", '<a href="http://webkit.org/" style="background-color: yellow;"><div>hello</div><div>world</div></a>',
+    selectAll, '<div style="background-color: yellow; ">hello</div><div style="background-color: yellow; ">world</div>');
+testSingleToggle("unlink", 'hello<a href="http://webkit.org/" style="background-color: yellow;"><div>world</div></a>WebKit',
+    selectAll, 'hello<div style="background-color: yellow; ">world</div><span class="Apple-style-span" style="background-color: yellow;">WebKit</span>');
+testSingleToggle("unlink", '<a href="http://webkit.org/" style="font-weight: bold;"><div>hello</div><div>world WebKit</div></a>',
+    selectLastTwoWords, '<div style="font-weight: bold; "><a href="http://webkit.org/">hello</a></div><div style="font-weight: bold; ">world WebKit</div>');
+testSingleToggle("unlink", '<a href="http://webkit.org/" style="font-weight: bold;"><div style="font-weight: normal;">hello</div><div>world</div></a>',
+    selectLastWord, '<div style="font-weight: normal; "><a href="http://webkit.org/">hello</a></div><div style="font-weight: bold; ">world</div>');
+
+document.body.removeChild(testContainer);
+
+var successfullyParsed = true;
diff --git a/LayoutTests/editing/execCommand/toggle-unlink-expected.txt b/LayoutTests/editing/execCommand/toggle-unlink-expected.txt
new file mode 100644 (file)
index 0000000..688ee70
--- /dev/null
@@ -0,0 +1,22 @@
+Test to make sure we preserve styles when removing links
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS unlink on all of "hello <b>world</b>" yields "hello <b>world</b>"
+PASS unlink on all of "<a href="http://webkit.org/"><u>hello world</u></a>" yields "<u>hello world</u>"
+PASS unlink on all of "hello <i><a href="http://webkit.org/">world</a></i>" yields "hello <i>world</i>"
+PASS unlink on all of "hello <a href="http://webkit.org/" style="font-weight: bold;">world</a>" yields "hello <b>world</b>"
+PASS unlink on all of "hello <a href="http://webkit.org/" style="color: blue;">world</a> WebKit" yields "hello <font class="Apple-style-span" color="#0000FF">world</font> WebKit"
+PASS unlink on all of "hello <a href="http://webkit.org/" style="color: blue; display: block;">world</a> WebKit" yields "hello <font class="Apple-style-span" color="#0000FF"><span class="Apple-style-span" style="display: block;">world</span></font> WebKit"
+PASS unlink on last two words of "<a href="http://webkit.org/" style="font-size: large;">hello world</a> WebKit" yields "<a href="http://webkit.org/" style="font-size: large;">hello </a><font class="Apple-style-span" size="4">world</font> WebKit"
+PASS unlink on last two words of "hello <a href="http://webkit.org/" style="font-size: large;">world <span style="font-size: small; ">WebKit</span> rocks</a>" yields "hello <a href="http://webkit.org/"><font class="Apple-style-span" size="4">world </font></a><span style="font-size: small; ">WebKit</span><font class="Apple-style-span" size="4"> rocks</font>"
+PASS unlink on first two words of "hello <a href="http://webkit.org/" style="font-style: italic;"><b>world</b> WebKit</a>" yields "hello <b style="font-style: italic; ">world</b><a href="http://webkit.org/"><i> WebKit</i></a>"
+PASS unlink on all of "<a href="http://webkit.org/" style="background-color: yellow;"><div>hello</div><div>world</div></a>" yields "<div style="background-color: yellow; ">hello</div><div style="background-color: yellow; ">world</div>"
+PASS unlink on all of "hello<a href="http://webkit.org/" style="background-color: yellow;"><div>world</div></a>WebKit" yields "hello<div style="background-color: yellow; ">world</div><span class="Apple-style-span" style="background-color: yellow;">WebKit</span>"
+PASS unlink on last two words of "<a href="http://webkit.org/" style="font-weight: bold;"><div>hello</div><div>world WebKit</div></a>" yields "<div style="font-weight: bold; "><a href="http://webkit.org/">hello</a></div><div style="font-weight: bold; ">world WebKit</div>"
+PASS unlink on last word of "<a href="http://webkit.org/" style="font-weight: bold;"><div style="font-weight: normal;">hello</div><div>world</div></a>" yields "<div style="font-weight: normal; "><a href="http://webkit.org/">hello</a></div><div style="font-weight: bold; ">world</div>"
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/editing/execCommand/toggle-unlink.html b/LayoutTests/editing/execCommand/toggle-unlink.html
new file mode 100644 (file)
index 0000000..01d39ae
--- /dev/null
@@ -0,0 +1,13 @@
+<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">
+<html>
+<head>
+<link rel="stylesheet" href="../../fast/js/resources/js-test-style.css">
+<script src="../../fast/js/resources/js-test-pre.js"></script>
+</head>
+<body>
+<p id="description"></p>
+<div id="console"></div>
+<script src="script-tests/toggle-unlink.js"></script>
+<script src="../../fast/js/resources/js-test-post.js"></script>
+</body>
+</html>
index e3c5c56..3e77509 100644 (file)
@@ -97,7 +97,7 @@ modifyWritingDirection('<div dir="rtl"><b>هنا يكتب</b> النص العر
 modifyWritingDirection('<div dir="rtl">هنا <span dir="ltr">يكتب النص العربي</span></div>', selectThirdWord, 'Natural',
                        '<div dir="rtl">هنا <span dir="ltr">يكتب</span> النص<span dir="ltr"> العربي</span></div>');
 modifyWritingDirection('<div dir="rtl">هنا <span dir="ltr">يكتب النص العربي</span></div>', selectThirdWord, 'LeftToRight',
-                       '<div dir="rtl"><span style="direction: ltr;">هنا يكتب النص العربي</span></div>');
+                       '<div dir="rtl"><span style="unicode-bidi: embed; direction: ltr;">هنا يكتب النص العربي</span></div>');
 modifyWritingDirection('<div dir="rtl">هنا <span dir="ltr">يكتب النص العربي</span></div>', selectThirdWord, 'RightToLeft',
                        '<div dir="rtl">هنا <span dir="ltr">يكتب</span><span style="unicode-bidi: embed;"> النص</span><span dir="ltr"> العربي</span></div>');
 
index cd6452f..b35c856 100644 (file)
@@ -1,5 +1,27 @@
 2010-10-15  Ryosuke Niwa  <rniwa@webkit.org>
 
+        Reviewed by Darin Adler.
+
+        unlink removes inline style of anchor elements
+        https://bugs.webkit.org/show_bug.cgi?id=47424
+
+        The bug was caused by our not extracting styles when the anchor element is removed by removeInlineStyle.
+        Because we removed the element without pushing its inline style down, we lost style information.
+
+        Fixed the bug by pushing down styles in removeInlineStyle as done in pushDownInlineStyleAroundNode.
+        Also fixed a bug in addInlineStyleIfNeeded where StyleChange incorrectly removed styles of the container
+        node as supposed to that of startNode from the style to apply when startNode is not an element.
+
+        Test: editing/execCommand/toggle-unlink.html
+
+        * editing/ApplyStyleCommand.cpp:
+        (WebCore::ApplyStyleCommand::removeInlineStyleFromElement): To prevent a similar bug in the future,
+        added an assert that extractedStyle is specified whenever we're removing a styled element.
+        (WebCore::ApplyStyleCommand::removeInlineStyle):
+        (WebCore::ApplyStyleCommand::addInlineStyleIfNeeded):
+
+2010-10-15  Ryosuke Niwa  <rniwa@webkit.org>
+
         Reviewed by Tony Chang.
 
         serializeNodesWithNamespaces should be in MarkupAccumulator
index d041f74..3f60a8b 100644 (file)
@@ -1223,11 +1223,12 @@ bool ApplyStyleCommand::removeInlineStyleFromElement(CSSMutableStyleDeclaration*
         return false;
 
     if (m_styledInlineElement && element->hasTagName(m_styledInlineElement->tagQName())) {
-        if (mode != RemoveNone) {
-            if (extractedStyle && element->inlineStyleDecl())
-                extractedStyle->merge(element->inlineStyleDecl());
-            removeNodePreservingChildren(element);
-        }
+        if (mode == RemoveNone)
+            return true;
+        ASSERT(extractedStyle);
+        if (element->inlineStyleDecl())
+            extractedStyle->merge(element->inlineStyleDecl());
+        removeNodePreservingChildren(element);
         return true;
     }
 
@@ -1586,12 +1587,19 @@ void ApplyStyleCommand::removeInlineStyle(PassRefPtr<CSSMutableStyleDeclaration>
 
     Node* node = start.node();
     while (node) {
-        Node* next = node->traverseNextNode();
+        RefPtr<Node> next = node->traverseNextNode();
         if (node->isHTMLElement() && nodeFullySelected(node, start, end)) {
-            HTMLElement* elem = static_cast<HTMLElement*>(node);
-            Node* prev = elem->traversePreviousNodePostOrder();
-            Node* next = elem->traverseNextNode();
-            removeInlineStyleFromElement(style.get(), elem);
+            RefPtr<HTMLElement> elem = static_cast<HTMLElement*>(node);
+            RefPtr<Node> prev = elem->traversePreviousNodePostOrder();
+            RefPtr<Node> next = elem->traverseNextNode();
+            RefPtr<CSSMutableStyleDeclaration> styleToPushDown;
+            PassRefPtr<Node> childNode = 0;
+            if (m_styledInlineElement && elem->hasTagName(m_styledInlineElement->tagQName())) {
+                styleToPushDown = CSSMutableStyleDeclaration::create();
+                childNode = elem->firstChild();
+            }
+
+            removeInlineStyleFromElement(style.get(), elem.get(), RemoveIfNeeded, styleToPushDown.get());
             if (!elem->inDocument()) {
                 if (s.node() == elem) {
                     // Since elem must have been fully selected, and it is at the start
@@ -1604,13 +1612,18 @@ void ApplyStyleCommand::removeInlineStyle(PassRefPtr<CSSMutableStyleDeclaration>
                     // of the selection, it is clear we can set the new e offset to
                     // the max range offset of prev.
                     ASSERT(e.deprecatedEditingOffset() >= lastOffsetForEditing(e.node()));
-                    e = Position(prev, lastOffsetForEditing(prev));
+                    e = Position(prev, lastOffsetForEditing(prev.get()));
                 }
             }
+
+            if (styleToPushDown) {
+                for (; childNode; childNode = childNode->nextSibling())
+                    applyInlineStyleToPushDown(childNode.get(), styleToPushDown.get());
+            }
         }
         if (node == end.node())
             break;
-        node = next;
+        node = next.get();
     }
     
     ASSERT(s.node()->inDocument());
@@ -1867,7 +1880,19 @@ void ApplyStyleCommand::addBlockStyle(const StyleChange& styleChange, HTMLElemen
 void ApplyStyleCommand::addInlineStyleIfNeeded(CSSMutableStyleDeclaration *style, Node *startNode, Node *endNode, EAddStyledElement addStyledElement)
 {
     // It's okay to obtain the style at the startNode because we've removed all relevant styles from the current run.
-    StyleChange styleChange(style, Position(startNode, 0));
+    RefPtr<HTMLElement> dummyElement;
+    Position positionForStyleComparison;
+    if (!startNode->isElementNode()) {
+        dummyElement = createStyleSpanElement(document());
+        insertNodeAt(dummyElement, positionBeforeNode(startNode));
+        positionForStyleComparison = positionBeforeNode(dummyElement.get());
+    } else
+        positionForStyleComparison = firstPositionInNode(startNode);
+
+    StyleChange styleChange(style, positionForStyleComparison);
+
+    if (dummyElement)
+        removeNode(dummyElement);
 
     // Find appropriate font and span elements top-down.
     HTMLElement* fontContainer = 0;