2010-09-30 Ryosuke Niwa <rniwa@webkit.org>
authorrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 30 Sep 2010 21:37:17 +0000 (21:37 +0000)
committerrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 30 Sep 2010 21:37:17 +0000 (21:37 +0000)
        Reviewed by Tony Chang.

        WebKit nests font element when applying different font styles
        https://bugs.webkit.org/show_bug.cgi?id=45568

        The bug was caused by fixRangeAndApplyInlineStyle's not including fully selected ancestors,
        and addInlineStyleIfNeeded's always surrounding the contents by new elements as supposed to
        adding font attributes or style attribute.

        Fixed the bug by extending the node range in fixRangeAndApplyInlineStyle and finding
        the appropriate container node to add attributes in addInlineStyleIfNeeded.
        addInlineStyleIfNeeded now tires to add font and style attributes to the inner most font and
        span elements respectively.

        Also added an early exit check to removeStyleFromRunBeforeApplyingStyle so that WebKit does not
        modify the contents when the entire contents already have the desired style.

        Test: editing/style/inline-style-container.html

        * editing/ApplyStyleCommand.cpp:
        (WebCore::ApplyStyleCommand::fixRangeAndApplyInlineStyle):
        (WebCore::ApplyStyleCommand::removeStyleFromRunBeforeApplyingStyle):
        (WebCore::ApplyStyleCommand::removeInlineStyleFromElement):
        (WebCore::ApplyStyleCommand::addInlineStyleIfNeeded):
2010-09-30  Ryosuke Niwa  <rniwa@webkit.org>

        Reviewed by Tony Chang.

        WebKit nests font element when applying different font styles
        https://bugs.webkit.org/show_bug.cgi?id=45568

        Added a test to ensure WebKit does not add extra font elements when the content already has an enclosing font element.
        The test also ensures WebKit tries to add style attribute to the inner most span.

        * editing/execCommand/createLink-expected.txt: The ordering of anchor and span changed.
        * editing/execCommand/unlink-expected.txt: Ditto.
        * editing/execCommand/query-font-size-expected.txt: Merged font elements.
        * editing/execCommand/script-tests/toggle-style-2.js: The ordering of s and u elements changed.
        * editing/execCommand/toggle-style-2-expected.txt:  Ditto.
        * editing/style/inline-style-container-expected.txt: Added.
        * editing/style/inline-style-container.html: Added.
        * editing/style/make-text-writing-direction-inline-expected.txt: Removed some extraneous spans added by the bug 44359.
        * editing/style/script-tests/inline-style-container.js: Added.
        (testSingleToggle):
        * editing/style/style-3690704-fix-expected.txt: Merged two b elements.
        * fast/events/event-input-contentEditable-expected.txt: Merged anchor elements.
        * fast/events/script-tests/event-input-contentEditable-expected.txt: Ditto.

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

15 files changed:
LayoutTests/ChangeLog
LayoutTests/editing/execCommand/createLink-expected.txt
LayoutTests/editing/execCommand/query-font-size-expected.txt
LayoutTests/editing/execCommand/script-tests/toggle-style-2.js
LayoutTests/editing/execCommand/toggle-style-2-expected.txt
LayoutTests/editing/execCommand/unlink-expected.txt
LayoutTests/editing/style/inline-style-container-expected.txt [new file with mode: 0644]
LayoutTests/editing/style/inline-style-container.html [new file with mode: 0644]
LayoutTests/editing/style/make-text-writing-direction-inline-expected.txt
LayoutTests/editing/style/script-tests/inline-style-container.js [new file with mode: 0644]
LayoutTests/editing/style/style-3690704-fix-expected.txt
LayoutTests/fast/events/event-input-contentEditable-expected.txt
LayoutTests/fast/events/script-tests/event-input-contentEditable.js
WebCore/ChangeLog
WebCore/editing/ApplyStyleCommand.cpp

index e9cc36e..1303c8c 100644 (file)
@@ -1,3 +1,27 @@
+2010-09-30  Ryosuke Niwa  <rniwa@webkit.org>
+
+        Reviewed by Tony Chang.
+
+        WebKit nests font element when applying different font styles
+        https://bugs.webkit.org/show_bug.cgi?id=45568
+
+        Added a test to ensure WebKit does not add extra font elements when the content already has an enclosing font element.
+        The test also ensures WebKit tries to add style attribute to the inner most span.
+
+        * editing/execCommand/createLink-expected.txt: The ordering of anchor and span changed.
+        * editing/execCommand/unlink-expected.txt: Ditto.
+        * editing/execCommand/query-font-size-expected.txt: Merged font elements.
+        * editing/execCommand/script-tests/toggle-style-2.js: The ordering of s and u elements changed.
+        * editing/execCommand/toggle-style-2-expected.txt:  Ditto.
+        * editing/style/inline-style-container-expected.txt: Added.
+        * editing/style/inline-style-container.html: Added.
+        * editing/style/make-text-writing-direction-inline-expected.txt: Removed some extraneous spans added by the bug 44359.
+        * editing/style/script-tests/inline-style-container.js: Added.
+        (testSingleToggle):
+        * editing/style/style-3690704-fix-expected.txt: Merged two b elements.
+        * fast/events/event-input-contentEditable-expected.txt: Merged anchor elements.
+        * fast/events/script-tests/event-input-contentEditable-expected.txt: Ditto.
+
 2010-09-30  Alpha Lam  <hclam@chromium.org>
 
         Build fix. Not reviewed.
index d0fe963..0301e72 100644 (file)
@@ -23,10 +23,10 @@ EDITING DELEGATE: webViewDidBeginEditing:WebViewDidBeginEditingNotification
 EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
 EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
 EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
-EDITING DELEGATE: shouldChangeSelectedDOMRange:(null) toDOMRange:range from 0 of #text > A > SPAN > DIV > LI > OL > BODY > HTML > #document to 121 of #text > A > DIV > LI > OL > BODY > HTML > #document affinity:NSSelectionAffinityDownstream stillSelecting:FALSE
+EDITING DELEGATE: shouldChangeSelectedDOMRange:(null) toDOMRange:range from 0 of #text > SPAN > A > DIV > LI > OL > BODY > HTML > #document to 121 of #text > A > DIV > LI > OL > BODY > HTML > #document affinity:NSSelectionAffinityDownstream stillSelecting:FALSE
 EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
 EDITING DELEGATE: webViewDidChange:WebViewDidChangeNotification
-EDITING DELEGATE: shouldEndEditingInDOMRange:range from 0 of DIV > LI > OL > BODY > HTML > #document to 4 of DIV > LI > OL > BODY > HTML > #document
+EDITING DELEGATE: shouldEndEditingInDOMRange:range from 0 of DIV > LI > OL > BODY > HTML > #document to 3 of DIV > LI > OL > BODY > HTML > #document
 EDITING DELEGATE: webViewDidEndEditing:WebViewDidEndEditingNotification
 EDITING DELEGATE: shouldBeginEditingInDOMRange:range from 0 of DIV > LI > OL > BODY > HTML > #document to 13 of DIV > LI > OL > BODY > HTML > #document
 EDITING DELEGATE: webViewDidBeginEditing:WebViewDidBeginEditingNotification
index b26894e..d90eff3 100644 (file)
@@ -19,13 +19,13 @@ manual CSS font-size: 50px  <span style="font-size: 50px">test</span>       7
 manual CSS font-size: 5em      <span style="font-size: 5em">test</span>        7
 manual CSS font-size: 10px     <span style="font-size: 10px">test</span>       1
 monospace tests to ensure the bug 19161 does not affect queryCommandValue's values
-execCommand('FontSize', -2)    <font class="Apple-style-span" size="1"><font class="Apple-style-span" face="monospace">test</font></font>      1
-execCommand('FontSize', -1)    <font class="Apple-style-span" size="2"><font class="Apple-style-span" face="monospace">test</font></font>      2
-execCommand('FontSize', 0)     <font class="Apple-style-span" size="1"><font class="Apple-style-span" face="monospace">test</font></font>      1
-execCommand('FontSize', 1)     <font class="Apple-style-span" size="1"><font class="Apple-style-span" face="monospace">test</font></font>      1
-execCommand('FontSize', 2)     <font class="Apple-style-span" size="2"><font class="Apple-style-span" face="monospace">test</font></font>      2
+execCommand('FontSize', -2)    <font class="Apple-style-span" size="1" face="monospace">test</font>    1
+execCommand('FontSize', -1)    <font class="Apple-style-span" size="2" face="monospace">test</font>    2
+execCommand('FontSize', 0)     <font class="Apple-style-span" size="1" face="monospace">test</font>    1
+execCommand('FontSize', 1)     <font class="Apple-style-span" size="1" face="monospace">test</font>    1
+execCommand('FontSize', 2)     <font class="Apple-style-span" size="2" face="monospace">test</font>    2
 execCommand('FontSize', 3)     <font class="Apple-style-span" face="monospace">test</font>     3
-execCommand('FontSize', 4)     <font class="Apple-style-span" size="4"><font class="Apple-style-span" face="monospace">test</font></font>      4
-execCommand('FontSize', 5)     <font class="Apple-style-span" size="5"><font class="Apple-style-span" face="monospace">test</font></font>      5
-execCommand('FontSize', 6)     <font class="Apple-style-span" size="6"><font class="Apple-style-span" face="monospace">test</font></font>      6
-execCommand('FontSize', 7)     <font class="Apple-style-span" size="7"><font class="Apple-style-span" face="monospace">test</font></font>      7
+execCommand('FontSize', 4)     <font class="Apple-style-span" size="4" face="monospace">test</font>    4
+execCommand('FontSize', 5)     <font class="Apple-style-span" size="5" face="monospace">test</font>    5
+execCommand('FontSize', 6)     <font class="Apple-style-span" size="6" face="monospace">test</font>    6
+execCommand('FontSize', 7)     <font class="Apple-style-span" size="7" face="monospace">test</font>    7
index 1f610a0..b3bfced 100644 (file)
@@ -37,7 +37,7 @@ testSingleToggle("strikethrough", "<u><b><s>test</s></b></u>", "<u><b>test</b></
 testDoubleToggle("strikethrough", "test", "test");
 
 testSingleToggle("strikethrough", "<u>test</u>", "<u><s>test</s></u>");
-testSingleToggle("underline", "<s>test</s>", "<s><u>test</u></s>");
+testSingleToggle("underline", "<s>test</s>", "<u><s>test</s></u>");
 
 testSingleToggle("strikethrough", '<span style="text-decoration: overline;">test</span>', '<span style="text-decoration: overline;"><s>test</s></span>');
 testSingleToggle("underline", '<span style="text-decoration: overline;">test</span>', '<span style="text-decoration: overline;"><u>test</u></span>');
index f901c72..edc4449 100644 (file)
@@ -10,7 +10,7 @@ PASS one strikethrough command converted test to <s>test</s>
 PASS one strikethrough command converted <u><b><s>test</s></b></u> to <u><b>test</b></u>
 PASS two strikethrough commands converted test to test
 PASS one strikethrough command converted <u>test</u> to <u><s>test</s></u>
-PASS one underline command converted <s>test</s> to <s><u>test</u></s>
+PASS one underline command converted <s>test</s> to <u><s>test</s></u>
 FAIL one strikethrough command converted <span style="text-decoration: overline;">test</span> to <span class="Apple-style-span" style="text-decoration: overline;"><s>test</s></span>, expected <span style="text-decoration: overline;"><s>test</s></span>
 FAIL one underline command converted <span style="text-decoration: overline;">test</span> to <span class="Apple-style-span" style="text-decoration: overline;"><u>test</u></span>, expected <span style="text-decoration: overline;"><u>test</u></span>
 PASS successfullyParsed is true
index b9b114e..5e04322 100644 (file)
@@ -65,4 +65,4 @@ The innerHTML of editable regions after the test:
 This paragraph should should end up unlinked.
 <a href="http://www.apple.com/">The</a> second<a href="http://www.apple.com/"> word in this paragraph should end up being unlinked, everything else should be a link.</a>
 This paragraph starts with <i><a href="http://www.google.com">a</a></i><span id="test3start"> link</span> in the middle. Only the 'a' in the previous sentence should be linked after the test.
-<p>This <i>editable region</i> contains lists, tables, styled text, and images. Everything in this region that is not selected should be a link, nothing that is selected should be a link.</p> <ul> <li>Item 1</li> <li>Item 2</li> </ul> <table border="1"><tbody><tr><td>1</td><td>2</td><td><a href="http://www.google.com/"><span id="test4end">3</span></a></td></tr></tbody></table> <a href="http://www.google.com/"><br> This <b>line</b> contains <img src="../resources/abe.png"> an image. </a>
+<p>This <i>editable region</i> contains lists, tables, styled text, and images. Everything in this region that is not selected should be a link, nothing that is selected should be a link.</p> <ul> <li>Item 1</li> <li>Item 2</li> </ul> <table border="1"><tbody><tr><td>1</td><td>2</td><td><span id="test4end"><a href="http://www.google.com/">3</a></span></td></tr></tbody></table> <a href="http://www.google.com/"><br> This <b>line</b> contains <img src="../resources/abe.png"> an image. </a>
diff --git a/LayoutTests/editing/style/inline-style-container-expected.txt b/LayoutTests/editing/style/inline-style-container-expected.txt
new file mode 100644 (file)
index 0000000..baca967
--- /dev/null
@@ -0,0 +1,36 @@
+Test to make sure WebKit adds style to the appropriate container.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+styleWithCSS = false
+PASS fontSize(3) on all of "<font size="3">hello <font size="4">world</font></font>" yields "<font size="3">hello world</font>"
+PASS fontSize(4) on all of "<font face="Arial">hello</font>" yields "<font face="Arial" size="4">hello</font>"
+PASS fontSize(4) on all of "<font color="blue"><font face="Arial">hello</font></font>" yields "<font color="blue"><font face="Arial" size="4">hello</font></font>"
+PASS fontSize(4) on all of "<b><font face="Arial">hello</font></b>" yields "<b><font face="Arial" size="4">hello</font></b>"
+PASS fontSize(4) on all of "<font face="Arial"><i>hello</i></font>" yields "<font face="Arial" size="4"><i>hello</i></font>"
+PASS fontName(Arial) on all of "<b><font face="Arial">hello</font></b> world" yields "<font class="Apple-style-span" face="Arial"><b>hello</b> world</font>"
+PASS fontName(Arial) on all of "<font color="blue">hello</font> world" yields "<font class="Apple-style-span" face="Arial"><font color="blue">hello</font> world</font>"
+PASS fontName(Arial) on all of "<b><u>hello</u> world</b>" yields "<b><font class="Apple-style-span" face="Arial"><u>hello</u> world</font></b>"
+PASS foreColor(blue) on all of "<font><u style="color:red;">hello</u></font>" yields "<font color="#0000FF"><u>hello</u></font>"
+PASS bold(null) on all of "<u><s>hello</s> <s>world</s></u>" yields "<u><b><s>hello</s> <s>world</s></b></u>"
+PASS bold(null) on all of "<i>hello</i> <b>world</b>" yields "<b><i>hello</i> world</b>"
+PASS bold(null) on all of "<s><i><u>hello <b>world</b></u></i> webkit</s>" yields "<s><b><i><u>hello world</u></i> webkit</b></s>"
+PASS bold(null) on all of "<b contenteditable="false"><span style="font-weight: normal;">hello</span> world</b> world" yields "<b contenteditable="false"><span style="font-weight: normal;">hello</span> world</b><b> world</b>"
+PASS bold(null) on all of "<i>hello</i> <b contenteditable="false">world</b>" yields "<b><i>hello</i> </b><b contenteditable="false">world</b>"
+PASS strikeThrough(null) on all of "<i>hello</i> <b><s>world</s></b> WebKit" yields "<s><i>hello</i> <b>world</b> WebKit</s>"
+PASS strikeThrough(null) on all of "<b><i>hello <s>world</s></i> WebKit</b>" yields "<b><s><i>hello world</i> WebKit</s></b>"
+
+styleWithCSS = true
+PASS fontSize(4) on all of "<font face="Arial">hello</font>" yields "<font face="Arial" style="font-size: large;">hello</font>"
+PASS fontSize(4) on all of "<font face="Arial"><b>hello</b></font>" yields "<font face="Arial"><b style="font-size: large;">hello</b></font>"
+PASS fontSize(4) on all of "<i><b>hello</b></i>" yields "<i><b style="font-size: large;">hello</b></i>"
+PASS fontSize(4) on all of "<i><b>hello</b> world</i>" yields "<i style="font-size: large;"><b>hello</b> world</i>"
+PASS fontSize(4) on all of "<font color="blue"><b>hello</b></font>" yields "<font color="blue"><b style="font-size: large;">hello</b></font>"
+PASS bold(null) on all of "<span style="font-style: italic;">hello</span>" yields "<span style="font-style: italic; font-weight: bold;">hello</span>"
+PASS underline(null) on all of "<span style="font-style: italic;"><b>hello</b></span>" yields "<span style="font-style: italic; text-decoration: underline;"><b>hello</b></span>"
+PASS underline(null) on all of "<span style="color: blue;"><i><span style="font-size: large;"><b>hello</b> world</span></i></span>" yields "<span style="color: blue;"><i><span style="font-size: large; text-decoration: underline;"><b>hello</b> world</span></i></span>"
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/editing/style/inline-style-container.html b/LayoutTests/editing/style/inline-style-container.html
new file mode 100644 (file)
index 0000000..756a906
--- /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/inline-style-container.js"></script>
+<script src="../../fast/js/resources/js-test-post.js"></script>
+</body>
+</html>
index d5d9128..5c20a9c 100644 (file)
@@ -10,7 +10,7 @@ PASS Natural on second and third words of "<b>hello world</b> webkit"
 PASS LeftToRight on second and third words of "<b>hello world</b> webkit"
 PASS RightToLeft on second and third words of "<b>hello world</b> webkit"
 FAIL (due to the bug 44359) Natural on third word of "<span dir="rtl">hello <span dir="ltr">world webkit rocks</span></span>" yielded <span dir="rtl">hello <span dir="ltr">world</span></span><span><span> webkit</span></span><span dir="rtl"><span dir="ltr"> rocks</span></span> but expected <span dir="rtl">hello <span dir="ltr">world</span></span> webkit<span dir="rtl"><span dir="ltr"> rocks</span></span>
-FAIL (due to the bug 44359) LeftToRight on third word of "<span dir="rtl">hello <span dir="ltr">world webkit rocks</span></span>" yielded <span dir="rtl">hello <span dir="ltr">world</span></span><span><span><span style="unicode-bidi: embed;"> webkit</span></span></span><span dir="rtl"><span dir="ltr"> rocks</span></span> but expected <span dir="rtl">hello <span dir="ltr">world</span></span><span style="unicode-bidi: embed;"> webkit</span><span dir="rtl"><span dir="ltr"> rocks</span></span>
+FAIL (due to the bug 44359) LeftToRight on third word of "<span dir="rtl">hello <span dir="ltr">world webkit rocks</span></span>" yielded <span dir="rtl">hello <span dir="ltr">world</span></span><span><span style="unicode-bidi: embed;"> webkit</span></span><span dir="rtl"><span dir="ltr"> rocks</span></span> but expected <span dir="rtl">hello <span dir="ltr">world</span></span><span style="unicode-bidi: embed;"> webkit</span><span dir="rtl"><span dir="ltr"> rocks</span></span>
 FAIL (due to the bug 44359) RightToLeft on third word of "<span dir="rtl">hello <span dir="ltr">world webkit rocks</span></span>" yielded <span dir="rtl">hello <span dir="ltr">world</span><span> webkit</span><span dir="ltr"> rocks</span></span> but expected <span dir="rtl">hello <span dir="ltr">world</span> webkit<span dir="ltr"> rocks</span></span>
 PASS Natural on first word of "هنا يكتب النص العربي"
 PASS LeftToRight on first word of "هنا يكتب النص العربي"
@@ -26,7 +26,7 @@ PASS LeftToRight on second and third words of "<div dir="rtl"><b>هنا يكتب
 PASS RightToLeft on second and third words of "<div dir="rtl"><b>هنا يكتب</b> النص العربي</div>"
 FAIL (due to the bug 44359) Natural on third word of "<div dir="rtl">هنا <span dir="ltr">يكتب النص العربي</span></div>" yielded <div dir="rtl">هنا <span dir="ltr">يكتب</span><span> النص</span><span dir="ltr"> العربي</span></div> but expected <div dir="rtl">هنا <span dir="ltr">يكتب</span> النص<span dir="ltr"> العربي</span></div>
 PASS LeftToRight on third word of "<div dir="rtl">هنا <span dir="ltr">يكتب النص العربي</span></div>"
-FAIL (due to the bug 44359) RightToLeft on third word of "<div dir="rtl">هنا <span dir="ltr">يكتب النص العربي</span></div>" yielded <div dir="rtl">هنا <span dir="ltr">يكتب</span><span><span style="unicode-bidi: embed;"> النص</span></span><span dir="ltr"> العربي</span></div> but expected <div dir="rtl">هنا <span dir="ltr">يكتب</span><span style="unicode-bidi: embed;"> النص</span><span dir="ltr"> العربي</span></div>
+PASS RightToLeft on third word of "<div dir="rtl">هنا <span dir="ltr">يكتب النص العربي</span></div>"
 PASS Natural on first word of "写中文"
 PASS LeftToRight on first word of "写中文"
 PASS RightToLeft on first word of "写中文"
diff --git a/LayoutTests/editing/style/script-tests/inline-style-container.js b/LayoutTests/editing/style/script-tests/inline-style-container.js
new file mode 100644 (file)
index 0000000..0497f79
--- /dev/null
@@ -0,0 +1,58 @@
+description('Test to make sure WebKit adds style to the appropriate container.');
+
+var testContainer = document.createElement("div");
+testContainer.contentEditable = true;
+document.body.appendChild(testContainer);
+
+var styleWithCSS = false;
+function testSingleToggle(toggleCommand, value, initialContents, expectedContents)
+{
+    testContainer.innerHTML = initialContents;
+    window.getSelection().selectAllChildren(testContainer);
+    document.execCommand('styleWithCSS', false, styleWithCSS);
+    document.execCommand(toggleCommand, false, value);
+    var action = toggleCommand + '(' + value + ') on all of "' + initialContents + '" yields "' + testContainer.innerHTML + '"';
+    if (testContainer.innerHTML == expectedContents)
+        testPassed(action);
+    else
+        testFailed(action + ', expected "' + expectedContents + '"');
+}
+
+debug('styleWithCSS = false');
+testSingleToggle("fontSize", 3, '<font size="3">hello <font size="4">world</font></font>', '<font size="3">hello world</font>');
+testSingleToggle("fontSize", 4, '<font face="Arial">hello</font>', '<font face="Arial" size="4">hello</font>');
+testSingleToggle("fontSize", 4, '<font color="blue"><font face="Arial">hello</font></font>', '<font color="blue"><font face="Arial" size="4">hello</font></font>');
+testSingleToggle("fontSize", 4, '<b><font face="Arial">hello</font></b>', '<b><font face="Arial" size="4">hello</font></b>');
+testSingleToggle("fontSize", 4, '<font face="Arial"><i>hello</i></font>', '<font face="Arial" size="4"><i>hello</i></font>');
+testSingleToggle("fontName", "Arial", '<b><font face="Arial">hello</font></b> world', '<font class="Apple-style-span" face="Arial"><b>hello</b> world</font>');
+testSingleToggle("fontName", "Arial", '<font color="blue">hello</font> world', '<font class="Apple-style-span" face="Arial"><font color="blue">hello</font> world</font>');
+testSingleToggle("fontName", "Arial", '<b><u>hello</u> world</b>', '<b><font class="Apple-style-span" face="Arial"><u>hello</u> world</font></b>');
+testSingleToggle("foreColor", 'blue', '<font><u style="color:red;">hello</u></font>', '<font color="#0000FF"><u>hello</u></font>');
+testSingleToggle("bold", null, '<u><s>hello</s> <s>world</s></u>', '<u><b><s>hello</s> <s>world</s></b></u>');
+testSingleToggle("bold", null, '<i>hello</i> <b>world</b>', '<b><i>hello</i> world</b>');
+testSingleToggle("bold", null, '<s><i><u>hello <b>world</b></u></i> webkit</s>', '<s><b><i><u>hello world</u></i> webkit</b></s>');
+testSingleToggle("bold", null,
+    '<b contenteditable="false"><span style="font-weight: normal;">hello</span> world</b> world',
+    '<b contenteditable="false"><span style="font-weight: normal;">hello</span> world</b><b> world</b>');
+testSingleToggle("bold", null,
+    '<i>hello</i> <b contenteditable="false">world</b>',
+    '<b><i>hello</i> </b><b contenteditable="false">world</b>');
+testSingleToggle("strikeThrough", null, '<i>hello</i> <b><s>world</s></b> WebKit', '<s><i>hello</i> <b>world</b> WebKit</s>');
+testSingleToggle("strikeThrough", null, '<b><i>hello <s>world</s></i> WebKit</b>', '<b><s><i>hello world</i> WebKit</s></b>');
+
+debug('')
+debug('styleWithCSS = true')
+styleWithCSS = true;
+testSingleToggle("fontSize", 4, '<font face="Arial">hello</font>', '<font face="Arial" style="font-size: large;">hello</font>');
+testSingleToggle("fontSize", 4, '<font face="Arial"><b>hello</b></font>', '<font face="Arial"><b style="font-size: large;">hello</b></font>');
+testSingleToggle("fontSize", 4, '<i><b>hello</b></i>', '<i><b style="font-size: large;">hello</b></i>');
+testSingleToggle("fontSize", 4, '<i><b>hello</b> world</i>', '<i style="font-size: large;"><b>hello</b> world</i>');
+testSingleToggle("fontSize", 4, '<font color="blue"><b>hello</b></font>', '<font color="blue"><b style="font-size: large;">hello</b></font>');
+testSingleToggle("bold", null, '<span style="font-style: italic;">hello</span>', '<span style="font-style: italic; font-weight: bold;">hello</span>');
+testSingleToggle("underline", null, '<span style="font-style: italic;"><b>hello</b></span>', '<span style="font-style: italic; text-decoration: underline;"><b>hello</b></span>');
+testSingleToggle("underline", null,
+    '<span style="color: blue;"><i><span style="font-size: large;"><b>hello</b> world</span></i></span>',
+    '<span style="color: blue;"><i><span style="font-size: large; text-decoration: underline;"><b>hello</b> world</span></i></span>');
+
+document.body.removeChild(testContainer);
+var successfullyParsed = true;
index 55a7ad5..37d1485 100644 (file)
@@ -24,9 +24,9 @@ EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotificatio
 EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
 EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
 EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
-EDITING DELEGATE: shouldChangeSelectedDOMRange:(null) toDOMRange:range from 0 of #text > B > I > SPAN > DIV > BODY > HTML > #document to 4 of #text > B > SPAN > DIV > BODY > HTML > #document affinity:NSSelectionAffinityDownstream stillSelecting:FALSE
+EDITING DELEGATE: shouldChangeSelectedDOMRange:(null) toDOMRange:range from 0 of #text > I > B > SPAN > DIV > BODY > HTML > #document to 4 of #text > B > SPAN > DIV > BODY > HTML > #document affinity:NSSelectionAffinityDownstream stillSelecting:FALSE
 EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
 EDITING DELEGATE: webViewDidChange:WebViewDidChangeNotification
 here is some text
 here is some more text
-execBoldCommand: <span id="test">here is <i><b>some</b></i><b> text<br>here</b> is some more text</span>
+execBoldCommand: <span id="test">here is <b><i>some</i> text<br>here</b> is some more text</span>
index eb2f6d0..96cee7f 100644 (file)
@@ -12,7 +12,7 @@ PASS event.target.innerHTML is '<br>'
 PASS event.target.id is 'target4'
 PASS event.target.innerHTML is '<a href="http://www.example.com/">This text should be a link.</a>'
 PASS event.target.id is 'target6parent'
-PASS event.target.innerHTML is '<span id="target6start"><a href="http://www.example.com/">Start,</a></span><a href="http://www.example.com/"><span id="target6middle">Middle,</span><span id="target6end">End.</span></a>'
+PASS event.target.innerHTML is '<a href="http://www.example.com/"><span id="target6start">Start,</span><span id="target6middle">Middle,</span><span id="target6end">End.</span></a>'
 PASS event.target.id is 'target7'
 PASS event.target.innerHTML is 'X'
 PASS event.target.id is 'target8'
index 8580841..4b4b797 100644 (file)
@@ -74,7 +74,7 @@ var target6start = document.getElementById("target6start");
 var target6middle = document.getElementById("target6middle");
 var target6end = document.getElementById("target6end");
 
-var expectedText6 = "<span id=\"target6start\"><a href=\"http://www.example.com/\">Start,</a></span><a href=\"http://www.example.com/\"><span id=\"target6middle\">Middle,</span><span id=\"target6end\">End.</span></a>";
+var expectedText6 = "<a href=\"http://www.example.com/\"><span id=\"target6start\">Start,</span><span id=\"target6middle\">Middle,</span><span id=\"target6end\">End.</span></a>";
 target6parent.addEventListener("input", function(evt) { fired(evt, "target6parent", expectedText6); });
 expectedInputEventCount++;
 target6start.addEventListener("input", function(evt) { testFailed("should not be reached"); });
index 274888f..ccc8cb7 100644 (file)
@@ -1,3 +1,30 @@
+2010-09-30  Ryosuke Niwa  <rniwa@webkit.org>
+
+        Reviewed by Tony Chang.
+
+        WebKit nests font element when applying different font styles
+        https://bugs.webkit.org/show_bug.cgi?id=45568
+
+        The bug was caused by fixRangeAndApplyInlineStyle's not including fully selected ancestors,
+        and addInlineStyleIfNeeded's always surrounding the contents by new elements as supposed to
+        adding font attributes or style attribute.
+
+        Fixed the bug by extending the node range in fixRangeAndApplyInlineStyle and finding
+        the appropriate container node to add attributes in addInlineStyleIfNeeded.
+        addInlineStyleIfNeeded now tires to add font and style attributes to the inner most font and
+        span elements respectively.
+
+        Also added an early exit check to removeStyleFromRunBeforeApplyingStyle so that WebKit does not
+        modify the contents when the entire contents already have the desired style.
+
+        Test: editing/style/inline-style-container.html
+
+        * editing/ApplyStyleCommand.cpp:
+        (WebCore::ApplyStyleCommand::fixRangeAndApplyInlineStyle):
+        (WebCore::ApplyStyleCommand::removeStyleFromRunBeforeApplyingStyle):
+        (WebCore::ApplyStyleCommand::removeInlineStyleFromElement):
+        (WebCore::ApplyStyleCommand::addInlineStyleIfNeeded):
+
 2010-09-30  Jarred Nicholls  <jarred@sencha.com>
 
         Reviewed by Darin Adler.
index 3ff7169..18ae02d 100644 (file)
@@ -1093,6 +1093,14 @@ void ApplyStyleCommand::fixRangeAndApplyInlineStyle(CSSMutableStyleDeclaration*
     if (start == end && start.node()->hasTagName(brTag))
         pastEndNode = start.node()->traverseNextNode();
 
+    // Start from the highest fully selected ancestor so that we can modify the fully selected node.
+    // e.g. When applying font-size: large on <font color="blue">hello</font>, we need to include the font element in our run
+    // to generate <font color="blue" size="4">hello</font> instead of <font color="blue"><font size="4">hello</font></font>
+    RefPtr<Range> range = Range::create(startNode->document(), start, end);
+    Element* editableRoot = startNode->rootEditableElement();
+    while (editableRoot && startNode->parentNode() != editableRoot && isNodeVisiblyContainedWithin(startNode->parentNode(), range.get()))
+        startNode = startNode->parentNode();
+
     applyInlineStyleToNodeRange(style, startNode, pastEndNode);
 }
 
@@ -1163,7 +1171,21 @@ void ApplyStyleCommand::applyInlineStyleToNodeRange(CSSMutableStyleDeclaration*
 
 bool ApplyStyleCommand::removeStyleFromRunBeforeApplyingStyle(CSSMutableStyleDeclaration* style, Node*& runStart, Node*& runEnd)
 {
+    ASSERT(runStart && runEnd && runStart->parentNode() == runEnd->parentNode());
     Node* pastEndNode = runEnd->traverseNextSibling();
+    bool needToApplyStyle = false;
+    for (Node* node = runStart; node && node != pastEndNode; node = node->traverseNextNode()) {
+        if (node->childNodeCount())
+            continue;
+        if (getPropertiesNotIn(style, computedStyle(node).get())->length()
+            || (m_styledInlineElement && !enclosingNodeWithTag(positionBeforeNode(node), m_styledInlineElement->tagQName()))) {
+            needToApplyStyle = true;
+            break;
+        }
+    }
+    if (!needToApplyStyle)
+        return false;
+
     Node* next;
     for (Node* node = runStart; node && node != pastEndNode; node = next) {
         next = node->traverseNextNode();
@@ -1191,6 +1213,9 @@ bool ApplyStyleCommand::removeInlineStyleFromElement(CSSMutableStyleDeclaration*
     ASSERT(style);
     ASSERT(element);
 
+    if (!element->parentNode() || !element->parentNode()->isContentEditable())
+        return false;
+
     if (m_styledInlineElement && element->hasTagName(m_styledInlineElement->tagQName())) {
         if (mode != RemoveNone) {
             if (extractedStyle && element->inlineStyleDecl())
@@ -1650,7 +1675,7 @@ void ApplyStyleCommand::splitTextElementAtEnd(const Position& start, const Posit
 
 bool ApplyStyleCommand::shouldSplitTextElement(Element* element, CSSMutableStyleDeclaration* style)
 {
-    if (!element || !element->isHTMLElement() || !element->parentElement() || !element->parentElement()->isContentEditable())
+    if (!element || !element->isHTMLElement())
         return false;
 
     return shouldRemoveInlineStyleFromElement(style, static_cast<HTMLElement*>(element));
@@ -1835,26 +1860,57 @@ 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));
 
+    // Find appropriate font and span elements top-down.
+    HTMLElement* fontContainer = 0;
+    HTMLElement* styleContainer = 0;
+    for (Node* container = startNode; container && startNode == endNode; container = container->firstChild()) {
+        if (container->isHTMLElement() && container->hasTagName(fontTag))
+            fontContainer = static_cast<HTMLElement*>(container);
+        bool styleContainerIsNotSpan = !styleContainer || !styleContainer->hasTagName(spanTag);
+        if (container->isHTMLElement() && (container->hasTagName(spanTag) || (styleContainerIsNotSpan && container->childNodeCount())))
+            styleContainer = static_cast<HTMLElement*>(container);
+        if (!container->firstChild())
+            break;
+        startNode = container->firstChild();
+        endNode = container->lastChild();
+    }
+
     // Font tags need to go outside of CSS so that CSS font sizes override leagcy font sizes.
     if (styleChange.applyFontColor() || styleChange.applyFontFace() || styleChange.applyFontSize()) {
-        RefPtr<Element> fontElement = createFontElement(document());
-
-        if (styleChange.applyFontColor())
-            fontElement->setAttribute(colorAttr, styleChange.fontColor());
-        if (styleChange.applyFontFace())
-            fontElement->setAttribute(faceAttr, styleChange.fontFace());
-        if (styleChange.applyFontSize())
-            fontElement->setAttribute(sizeAttr, styleChange.fontSize());
-
-        surroundNodeRangeWithElement(startNode, endNode, fontElement.get());
+        if (fontContainer) {
+            if (styleChange.applyFontColor())
+                setNodeAttribute(fontContainer, colorAttr, styleChange.fontColor());
+            if (styleChange.applyFontFace())
+                setNodeAttribute(fontContainer, faceAttr, styleChange.fontFace());
+            if (styleChange.applyFontSize())
+                setNodeAttribute(fontContainer, sizeAttr, styleChange.fontSize());
+        } else {
+            RefPtr<Element> fontElement = createFontElement(document());
+            if (styleChange.applyFontColor())
+                fontElement->setAttribute(colorAttr, styleChange.fontColor());
+            if (styleChange.applyFontFace())
+                fontElement->setAttribute(faceAttr, styleChange.fontFace());
+            if (styleChange.applyFontSize())
+                fontElement->setAttribute(sizeAttr, styleChange.fontSize());
+            surroundNodeRangeWithElement(startNode, endNode, fontElement.get());
+        }
     }
 
     if (styleChange.cssStyle().length()) {
-        RefPtr<Element> styleElement = createStyleSpanElement(document());
-        styleElement->setAttribute(styleAttr, styleChange.cssStyle());
-        surroundNodeRangeWithElement(startNode, endNode, styleElement.release());
+        if (styleContainer) {
+            CSSMutableStyleDeclaration* existingStyle = static_cast<HTMLElement*>(styleContainer)->inlineStyleDecl();
+            if (existingStyle)
+                setNodeAttribute(styleContainer, styleAttr, existingStyle->cssText() + styleChange.cssStyle());
+            else
+                setNodeAttribute(styleContainer, styleAttr, styleChange.cssStyle());
+        } else {
+            RefPtr<Element> styleElement = createStyleSpanElement(document());
+            styleElement->setAttribute(styleAttr, styleChange.cssStyle());
+            surroundNodeRangeWithElement(startNode, endNode, styleElement.release());
+        }
     }
 
     if (styleChange.applyBold())