Bug 44560 – cannot remove text-decoration when style is added by u or s

• Component: HTML Editing
• Status: RESOLVED FIXED

[Enrica Casucci]

With the following markup:

<u><b>one two three</b></u>, select the word 'two' and execCommand('underline').

EXPECTED: the word 'two' is bold and NOT underlined
ACTUAL: the word 'two' is bold AND underlined

[Enrica Casucci]

Created attachment 65338 [details]
Test case

When running this test, the word 'and' should be bold but not underlined.

[Ryosuke Niwa]

(In reply to comment #1)
> Created an attachment (id=65338) [details]
> Test case
> 
> When running this test, the word 'and' should be bold but not underlined.

I just found this bug in pushDownInlineStyleAroundNode while working on the bug 30836.  Will work on it ASAP.  Thanks for filing a bug.

[Ryosuke Niwa]

Wait, this is a different bug.  We have never been able to push text-decoration down when text-decoration is added by u or s.  Notice that WebKit removes underline if your markup was:
<span style="text-decoration: underline;"><b>one two three</b></span>

[Enrica Casucci]

This regression was introduced in http://trac.webkit.org/changeset/46373 where 
if (alteredProperty.cssText() != "none")
was correctly changed to
if (!equalIgnoringCase(alteredProperty.value()->cssText(), "none")) .

The original if statement was comparing the full css text (text-decoration: none) with none, whereas the second one compares only the value of the property.

The change that introduced this was http://trac.webkit.org/changeset/46286 with the intent of removing redundant text-decoration: none.
Unfortunately the attached example shows a case where it is necessary to add text-decoration:none to the <u> element, to allow all the children to have text-decoration:underline inline when needed.

The current version of AppleStyleCommand.cpp is very different from this one, so I need to figure out how to change it.
The code before ttp://trac.webkit.org/changeset/46286 was pushing the text decoration down successfully (see the attached test case for a meaningful example).

[Ryosuke Niwa]

(In reply to comment #4)
> Unfortunately the attached example shows a case where it is necessary to add text-decoration:none to the <u> element, to allow all the children to have text-decoration:underline inline when needed.

The correct fix is to remove u element entirely.  I'm working on a patch I speak.  Give me 1-2 days and will post a patch.

[Ryosuke Niwa]

(In reply to comment #5)
> (In reply to comment #4)
> > Unfortunately the attached example shows a case where it is necessary to add text-decoration:none to the <u> element, to allow all the children to have text-decoration:underline inline when needed.
> 
> The correct fix is to remove u element entirely.  I'm working on a patch I speak.  Give me 1-2 days and will post a patch.

Oh, I didn't you assigned yourself. I can post work-in-progress patch if you'd like to take over.

[Ryosuke Niwa]

Created attachment 65383 [details]
work in progress

[Ryosuke Niwa]

Most of it is cleanup.  I'm combining implicitlyStyledElementShouldBeRemovedWhenApplyingStyle, removeHTMLFontStyle, and removeHTMLBidiEmbeddingStyle into removeImplicitlyStyledElement so that I can add a feature to extract styles later.  This cleanup is ~60% of patch and should be isolated into a separate patch.

The actual fix is in extractInlineStyleToPushDown where I call removeImplicitlyStyledElement in two places.  We have to extract inline declaration first because we won't be able to remove the node otherwise when implicitly styled element has style attribute.  Assuming I separate the cleanup, this fix should be very simple and small.

The only test that fails with this patch is editing/style/typing-style-003.html and I suspect that we're getting a simpler markup because the result is visually identical.  I'll file a bug to convert it to a dump-as-markup / dump-as-text test first.

[Enrica Casucci]

(In reply to comment #8)
> Most of it is cleanup.  I'm combining implicitlyStyledElementShouldBeRemovedWhenApplyingStyle, removeHTMLFontStyle, and removeHTMLBidiEmbeddingStyle into removeImplicitlyStyledElement so that I can add a feature to extract styles later.  This cleanup is ~60% of patch and should be isolated into a separate patch.
> 
> The actual fix is in extractInlineStyleToPushDown where I call removeImplicitlyStyledElement in two places.  We have to extract inline declaration first because we won't be able to remove the node otherwise when implicitly styled element has style attribute.  Assuming I separate the cleanup, this fix should be very simple and small.
> 
> The only test that fails with this patch is editing/style/typing-style-003.html and I suspect that we're getting a simpler markup because the result is visually identical.  I'll file a bug to convert it to a dump-as-markup / dump-as-text test first.

The patch looks good to me (other than the commented code left :-)). The markup generated is even better in my opinion. Do you think you can finish the cleanup and land it soon?
You could use that repro case I attached as starting point for the test to be added with the patch.
Also, I don't think you need to file a separate bug for the test that fails, it simply needs to be re-baselined since the generated markup is different.
Thanks for taking this on!!

[Ryosuke Niwa]

(In reply to comment #9)
> The patch looks good to me (other than the commented code left :-)). The markup generated is even better in my opinion. Do you think you can finish the cleanup and land it soon?

Sure.  I'll file cleanup & conversion bugs today so that someone can review those.  If those two bugs were to be resolved by Thursday, I should be able to fix the bug this week.

> You could use that repro case I attached as starting point for the test to be added with the patch.

Yeah, I need a whole bunch of tests here.

> Also, I don't think you need to file a separate bug for the test that fails, it simply needs to be re-baselined since the generated markup is different.

Right, but I'd like to convert editing tests to dump-as-text / dump-as-markup slowly because they're much more readable and have single expected results for all platforms.

> Thanks for taking this on!!

No problem.  After all, it seems like my regression so happy to fix it.

[Ryosuke Niwa]

I have filed bugs for test conversion and cleanup each with a corresponding patch. As soon as those two patches are landed, I'll post a patch for this bug.

[Ryosuke Niwa]

Created attachment 65745 [details]
work in progress 2

[Ryosuke Niwa]

Created attachment 65758 [details]
fixes the bug by pushing down all implicitly styled elements

[WebKit Review Bot]

Attachment 65758 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/ChangeLog:11:  Line contains tab character.  [whitespace/tab] [5]
WebCore/ChangeLog:12:  Line contains tab character.  [whitespace/tab] [5]
WebCore/ChangeLog:14:  Line contains tab character.  [whitespace/tab] [5]
WebCore/ChangeLog:15:  Line contains tab character.  [whitespace/tab] [5]
WebCore/ChangeLog:16:  Line contains tab character.  [whitespace/tab] [5]
WebCore/ChangeLog:17:  Line contains tab character.  [whitespace/tab] [5]
WebCore/ChangeLog:19:  Line contains tab character.  [whitespace/tab] [5]
WebCore/ChangeLog:20:  Line contains tab character.  [whitespace/tab] [5]
WebCore/ChangeLog:21:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 9 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ryosuke Niwa]

Created attachment 65759 [details]
fixed change logs and the expected result for push-down-implicit-styles

[Enrica Casucci]

Comment on attachment 65759 [details]
fixed change logs and the expected result for push-down-implicit-styles

> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog	(revision 66198)
> +++ WebCore/ChangeLog	(working copy)
> @@ -1,3 +1,37 @@
> [phone]  Ryosuke Niwa  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        cannot remove text-decoration when style is added by u or s
> +        https://bugs.webkit.org/show_bug.cgi?id=44560
> +
> +        The bug was caused by WebKit not pushing down implicitly added styles to descendent nodes.
> +        Fixed the bug by pushing down all elements that implicitly add style.
> +
> +        extractInlineStyleToPushDown now calls removeImplicitlyStyledElement, which was modified to extract
> +        the style implicitly added by the element to extractStyle.
> +
> +        This revealed a bug in applyInlineStyleToPushDown where applyInlineStyleIfNeeded could add
> +        an implicitly styled element inside an element with a conflicting style, thereby overriding the style of the element.
> +        Fixed this by extending the logic to honor the existing inline style declaration used in
> +        the case of rewriting inline style declaration to all cases including the one calling addInlineStyleIfNeeded.
> +
> +        Also fixed a bug in removeInlineStyle where pushDownInlineStyleAroundNode was called on a text node
> +        outside of selection if start was at the end of the text node.
> +
> +        Test: editing/style/push-down-implicit-styles.html
> +
> +        * editing/ApplyStyleCommand.cpp:
> +        (WebCore::ApplyStyleCommand::removeImplicitlyStyledElement): Added extractedStyle, which receives
> +        the style implicitly added by the element being removed.
> +        (WebCore::ApplyStyleCommand::extractInlineStyleToPushDown): Calls removeImplicitlyStyledElement
> +        to remove the implicitly styled element and extract the inline style added by the element.
> +        (WebCore::ApplyStyleCommand::applyInlineStyleToPushDown): Fixed a bug that addInlineStyleIfNeeded
> +        could override the inline style declaration style of the node.
> +        (WebCore::ApplyStyleCommand::pushDownInlineStyleAroundNode): Uses extractInlineStyleToPushDown
> +        (WebCore::ApplyStyleCommand::removeInlineStyle): See above.
> +        * editing/ApplyStyleCommand.h: Prototype changes.
> +
>  2010-08-27  Yury Semikhatsky  <[e-mail]>
>  
>          Reviewed by Pavel Feldman.
> Index: WebCore/editing/ApplyStyleCommand.cpp
> ===================================================================
> --- WebCore/editing/ApplyStyleCommand.cpp	(revision 66189)
> +++ WebCore/editing/ApplyStyleCommand.cpp	(working copy)
> @@ -1197,15 +1197,17 @@ static const HTMLEquivalent HTMLEquivale
>      { CSSPropertyDirection, false, CSSValueInvalid, 0, &dirAttr, ShouldNotBePushedDown },
>  };
>  
> -bool ApplyStyleCommand::removeImplicitlyStyledElement(CSSMutableStyleDeclaration* style, HTMLElement* element, InlineStyleRemovalMode mode)
> +bool ApplyStyleCommand::removeImplicitlyStyledElement(CSSMutableStyleDeclaration* style, HTMLElement* element, InlineStyleRemovalMode mode, CSSMutableStyleDeclaration* extractedStyle)
>  {
> +    // Current implementation does not support stylePushedDown when mode == RemoveNone because of early exit.
> +    ASSERT(!extractedStyle || mode != RemoveNone);
>      bool removed = false;
>      for (size_t i = 0; i < sizeof(HTMLEquivalents) / sizeof(HTMLEquivalent); i++) {
>          const HTMLEquivalent& equivalent = HTMLEquivalents[i];
>          ASSERT(equivalent.element || equivalent.attribute);
> -        if (equivalent.element && !element->hasTagName(*equivalent.element))
> -            continue;
> -        if (equivalent.attribute && !element->hasAttribute(*equivalent.attribute))
> +        if (extractedStyle && equivalent.pushDownType == ShouldNotBePushedDown
> +            || equivalent.element && !element->hasTagName(*equivalent.element)
> +            || equivalent.attribute && !element->hasAttribute(*equivalent.attribute))
>              continue;
>  
>          RefPtr<CSSValue> styleValue = style->getPropertyCSSValue(equivalent.propertyID);
> @@ -1218,6 +1220,14 @@ bool ApplyStyleCommand::removeImplicitly
>          else if (styleValue->cssText() == mapValue->cssText())
>              continue; // If CSS value is primitive, then skip if they are equal.
>  
> +        if (extractedStyle) {
> +            ASSERT(equivalent.pushDownType != ShouldNotBePushedDown);

I think the assert above is redundant.
> +            if (equivalent.primitiveId == CSSValueInvalid)
> +                extractedStyle->setProperty(equivalent.propertyID, element->getAttribute(*equivalent.attribute));
> +            else
> +                extractedStyle->setProperty(equivalent.propertyID, mapValue->cssText());
> +        }
> +
>          if (mode == RemoveNone)
>              return true;
>  
> @@ -1314,7 +1324,7 @@ HTMLElement* ApplyStyleCommand::highestA
>      return result;
>  }
>  
> -PassRefPtr<CSSMutableStyleDeclaration> ApplyStyleCommand::extractInlineStyleToPushDown(Node* node, bool isStyledElement, const Vector<int>& properties)
> +PassRefPtr<CSSMutableStyleDeclaration> ApplyStyleCommand::extractInlineStyleToPushDown(CSSMutableStyleDeclaration* styleToApply, Node* node, bool isStyledElement)
>  {
>      ASSERT(node);
>      ASSERT(node->isElementNode());
> @@ -1330,8 +1340,16 @@ PassRefPtr<CSSMutableStyleDeclaration> A
>          return style.release();
>      }
>  
> -    if (!style)
> -        return 0;
> +    if (!style) {
> +        style = CSSMutableStyleDeclaration::create();
> +        removeImplicitlyStyledElement(styleToApply, element, RemoveAttributesAndElements, style.get());
> +        return style.release();
> +    }
> +
> +    Vector<int> properties;
> +    CSSMutableStyleDeclaration::const_iterator end = styleToApply->end();
> +    for (CSSMutableStyleDeclaration::const_iterator it = styleToApply->begin(); it != end; ++it)
> +        properties.append(it->id());
>  
>      style = style->copyPropertiesInSet(properties.data(), properties.size());
>      for (size_t i = 0; i < properties.size(); i++) {
> @@ -1346,6 +1364,8 @@ PassRefPtr<CSSMutableStyleDeclaration> A
>      if (isSpanWithoutAttributesOrUnstyleStyleSpan(element))
>          removeNodePreservingChildren(element);
>  
> +    removeImplicitlyStyledElement(styleToApply, element, RemoveAttributesAndElements, style.get());
> +
>      return style.release();
>  }
>  
> @@ -1356,15 +1376,14 @@ void ApplyStyleCommand::applyInlineStyle
>      if (!style || !style->length() || !node->renderer())
>          return;
>  
> -    // Since addInlineStyleIfNeeded can't add styles to block-flow render objects, add style attribute instead.
> -    // FIXME: applyInlineStyleToRange should be used here instead.
> -    if ((node->renderer()->isBlockFlow() || node->childNodeCount()) && node->isHTMLElement()) {
> +    RefPtr<CSSMutableStyleDeclaration> newInlineStyle = style;
> +    if (node->isHTMLElement()) {
>          HTMLElement* element = static_cast<HTMLElement*>(node);
>          CSSMutableStyleDeclaration* existingInlineStyle = element->inlineStyleDecl();
>  
>          // Avoid overriding existing styles of node
>          if (existingInlineStyle) {
> -            RefPtr<CSSMutableStyleDeclaration> newInlineStyle = existingInlineStyle->copy();
> +            newInlineStyle = existingInlineStyle->copy();
>              CSSMutableStyleDeclaration::const_iterator end = style->end();
>              for (CSSMutableStyleDeclaration::const_iterator it = style->begin(); it != end; ++it) {
>                  ExceptionCode ec;
> @@ -1391,22 +1410,23 @@ void ApplyStyleCommand::applyInlineStyle
>                      }
>                  }
>              }
> +        }
> +    }
>  
> -            setNodeAttribute(element, styleAttr, newInlineStyle->cssText());
> -        } else
> -            setNodeAttribute(element, styleAttr, style->cssText());
> -
> +    // Since addInlineStyleIfNeeded can't add styles to block-flow render objects, add style attribute instead.
> +    // FIXME: applyInlineStyleToRange should be used here instead.
> +    if ((node->renderer()->isBlockFlow() || node->childNodeCount()) && node->isHTMLElement()) {
> +        setNodeAttribute(static_cast<HTMLElement*>(node), styleAttr, newInlineStyle->cssText());
>          return;
>      }
>  
>      if (node->renderer()->isText() && static_cast<RenderText*>(node->renderer())->isAllCollapsibleWhitespace())
>          return;
>  
> -    // FIXME: addInlineStyleIfNeeded may override the style of node
>      // We can't wrap node with the styled element here because new styled element will never be removed if we did.
>      // If we modified the child pointer in pushDownInlineStyleAroundNode to point to new style element
>      // then we fall into an infinite loop where we keep removing and adding styled element wrapping node.
> -    addInlineStyleIfNeeded(style, node, node, DoNotAddStyledElement);
> +    addInlineStyleIfNeeded(newInlineStyle.get(), node, node, DoNotAddStyledElement);
>  }
>  
>  void ApplyStyleCommand::pushDownInlineStyleAroundNode(CSSMutableStyleDeclaration* style, Node* targetNode)
> @@ -1415,11 +1435,6 @@ void ApplyStyleCommand::pushDownInlineSt
>      if (!highestAncestor)
>          return;
>  
> -    Vector<int> properties;
> -    CSSMutableStyleDeclaration::const_iterator end = style->end();
> -    for (CSSMutableStyleDeclaration::const_iterator it = style->begin(); it != end; ++it)
> -        properties.append(it->id());
> -
>      // The outer loop is traversing the tree vertically from highestAncestor to targetNode
>      Node* current = highestAncestor;
>      while (current != targetNode) {
> @@ -1431,7 +1446,7 @@ void ApplyStyleCommand::pushDownInlineSt
>          RefPtr<StyledElement> styledElement;
>          if (current->isStyledElement() && m_styledInlineElement && current->hasTagName(m_styledInlineElement->tagQName()))
>              styledElement = static_cast<StyledElement*>(current);
> -        RefPtr<CSSMutableStyleDeclaration> styleToPushDown = extractInlineStyleToPushDown(current, styledElement, properties);
> +        RefPtr<CSSMutableStyleDeclaration> styleToPushDown = extractInlineStyleToPushDown(style, current, styledElement);
>  
>          // The inner loop will go through children on each level
>          // FIXME: we should aggregate inline child elements together so that we don't wrap each child separately.
> @@ -1483,6 +1498,12 @@ void ApplyStyleCommand::removeInlineStyl
>      }
>  
>      Position pushDownStart = start.downstream();
> +    // If the pushDownStart is at the end of a text node, then this node is not fully selected.
> +    // Move it to the next deep quivalent position to avoid removing the style from this node.
> +    // e.g. if pushDownStart was at Position("hello", 5) in <b>hello<div>world</div></b>, we want Position("world", 0) instead.
> +    Node* pushDownStartContainer = pushDownStart.containerNode();
> +    if (pushDownStartContainer && pushDownStartContainer->isTextNode() && pushDownStart.computeOffsetInContainerNode() == pushDownStartContainer->maxCharacterOffset())
I would split this over two lines to ease of reading.
> +        pushDownStart = nextVisuallyDistinctCandidate(pushDownStart);
>      Position pushDownEnd = end.upstream();
>      pushDownInlineStyleAroundNode(style.get(), pushDownStart.node());
>      pushDownInlineStyleAroundNode(style.get(), pushDownEnd.node());
> Index: WebCore/editing/ApplyStyleCommand.h
> ===================================================================
> --- WebCore/editing/ApplyStyleCommand.h	(revision 66189)
> +++ WebCore/editing/ApplyStyleCommand.h	(working copy)
> @@ -73,11 +73,11 @@ private:
>      // style-removal helpers
>      bool removeInlineStyleFromElement(CSSMutableStyleDeclaration*, HTMLElement*, InlineStyleRemovalMode = RemoveAttributesAndElements);
>      inline bool shouldRemoveInlineStyleFromElement(CSSMutableStyleDeclaration* style, HTMLElement* element) {return removeInlineStyleFromElement(style, element, RemoveNone);}
> -    bool removeImplicitlyStyledElement(CSSMutableStyleDeclaration*, HTMLElement*, InlineStyleRemovalMode);
> +    bool removeImplicitlyStyledElement(CSSMutableStyleDeclaration*, HTMLElement*, InlineStyleRemovalMode, CSSMutableStyleDeclaration* extractedStyle = 0);
>      void replaceWithSpanOrRemoveIfWithoutAttributes(HTMLElement*&);
>      bool removeCSSStyle(CSSMutableStyleDeclaration*, HTMLElement*, InlineStyleRemovalMode = RemoveAttributesAndElements);
>      HTMLElement* highestAncestorWithConflictingInlineStyle(CSSMutableStyleDeclaration*, Node*);
> -    PassRefPtr<CSSMutableStyleDeclaration> extractInlineStyleToPushDown(Node*, bool isStyledElement, const Vector<int>&);
> +    PassRefPtr<CSSMutableStyleDeclaration> extractInlineStyleToPushDown(CSSMutableStyleDeclaration*, Node*, bool isStyledElement);
>      void applyInlineStyleToPushDown(Node*, CSSMutableStyleDeclaration *style);
>      void pushDownInlineStyleAroundNode(CSSMutableStyleDeclaration*, Node*);
>      void removeInlineStyle(PassRefPtr<CSSMutableStyleDeclaration>, const Position& start, const Position& end);
> Index: LayoutTests/ChangeLog
> ===================================================================
> --- LayoutTests/ChangeLog	(revision 66198)
> +++ LayoutTests/ChangeLog	(working copy)
> @@ -1,3 +1,25 @@
> [phone]  Ryosuke Niwa  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        cannot remove text-decoration when style is added by u or s
> +        https://bugs.webkit.org/show_bug.cgi?id=44560
> +
> +        Added a test to ensure WebKit removes text-decorations added by u or s
> +        and pushes down implicit styles properly into descendent nodes.
> +
> +        * editing/style/push-down-implicit-styles-expected.txt: Added.
> +        * editing/style/push-down-implicit-styles.html: Added.
> +        * editing/style/script-tests/push-down-implicit-styles.js: Added.
> +        (testSingleToggle):
> +        (selectAll):
> +        (selectTest):
> +        (selectSecondWord):
> +        (selectLastTwoWords):
> +        * editing/style/typing-style-003-expected.txt: Negation of inline
> +        styles are now done by pushing down implicitly styled elements rather than
> +        canceling them by inline style declarations.
> +
>  2010-08-27  Sheriff Bot  <[e-mail]>
>  
>          Unreviewed, rolling out r66188.
> Index: LayoutTests/editing/style/push-down-implicit-styles-expected.txt
> ===================================================================
> --- LayoutTests/editing/style/push-down-implicit-styles-expected.txt	(revision 0)
> +++ LayoutTests/editing/style/push-down-implicit-styles-expected.txt	(revision 0)
> @@ -0,0 +1,30 @@
> +Test to make sure we push down inline styles properly.
> +
> +On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
> +
> +
> +PASS bold on first word of <b><div>hello</div> world</b> yields <div>hello</div><b> world</b>
> +PASS bold on first word of <b><div>hello</div>world</b> yields <div>hello</div><b>world</b>
> +PASS bold on first word of <b><div>hello</div><em>world</em></b> yields <div>hello</div><em style="font-weight: bold; ">world</em>
> +PASS bold on second word of <b>hello <div>world</div></b> yields <b>hello </b><div>world</div>
> +PASS bold on second word of <b><em>hello</em> <div>world</div></b> yields <em style="font-weight: bold; ">hello</em> <div>world</div>
> +PASS bold on all of <b> <div>text</div> </b> yields  <div>text</div> 
> +PASS bold on all of <b><s><div>text</div></s></b> yields <s><div>text</div></s>
> +PASS bold on first word of <b><div>hello</div><div>world</div></b> yields <div>hello</div><div style="font-weight: bold; ">world</div>
> +PASS bold on first word of <b><div>hello</div><div style="font-weight: normal;">world</div>webkit</b> yields <div>hello</div><div style="font-weight: normal; ">world</div><b>webkit</b>
> +PASS bold on second word of <b style="font-style: italic;">hello world</b> yields <b style="font-style: italic;">hello</b><span style="font-style: italic;"> world</span>
> +PASS underline on second word of <u>hello <b>world</b> webkit</u> yields <u>hello</u> <b>world</b><u> webkit</u>
> +PASS underline on last two words of <u>hello <b>world</b> webkit</u> yields <u>hello </u><b>world</b> webkit
> +PASS underline on last two words of <u>hello <b>world webkit</b></u> yields <u>hello </u><b>world webkit</b>
> +PASS underline on second word of <u>hello <b>world webkit</b></u> yields <u>hello</u> <b>world<u> webkit</u></b>
> +PASS underline on second word of <u><b>hello world</b> webkit</u> yields <b><u>hello</u> world</b><u> webkit</u>
> +PASS underline on second word of <u><s>hello world</s></u> yields <s><u>hello</u> world</s>
> +PASS underline on second word of <u><s>hello world webkit</s></u> yields <s><u>hello</u> world<u> webkit</u></s>
> +PASS underline on second word of <u><s>hello world</s> webkit</u> yields <s><u>hello</u> world</s><u> webkit</u>
> +PASS underline on second word of <u>hello <em><code>world webkit</code></em> rocks</u> yields <u>hello</u> <em><code>world<u> webkit</u></code></em><u> rocks</u>
> +PASS strikeThrough on all of <s style="color: blue;">hello world</s> yields <span style="color: blue;">hello world</span>
> +PASS strikeThrough on first word of <s style="color: blue;"><div>hello</div> <b>world</b> webkit</s> yields <span style="color: blue;"><div>hello</div> <b style="text-decoration: line-through; ">world</b><s> webkit</s></span>
> +PASS successfullyParsed is true
> +
> +TEST COMPLETE
> +
> Index: LayoutTests/editing/style/push-down-implicit-styles.html
> ===================================================================
> --- LayoutTests/editing/style/push-down-implicit-styles.html	(revision 0)
> +++ LayoutTests/editing/style/push-down-implicit-styles.html	(revision 0)
> @@ -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/push-down-implicit-styles.js"></script>
> +<script src="../../fast/js/resources/js-test-post.js"></script>
> +</body>
> +</html>
> Index: LayoutTests/editing/style/typing-style-003-expected.txt
> ===================================================================
> --- LayoutTests/editing/style/typing-style-003-expected.txt	(revision 66189)
> +++ LayoutTests/editing/style/typing-style-003-expected.txt	(working copy)
> @@ -31,23 +31,23 @@ EDITING DELEGATE: shouldChangeSelectedDO
>  EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
>  EDITING DELEGATE: webViewDidChange:WebViewDidChangeNotification
>  EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
> -EDITING DELEGATE: shouldChangeSelectedDOMRange:(null) toDOMRange:range from 1 of #text > SPAN > I > B > DIV > BODY > HTML > #document to 1 of #text > SPAN > I > B > DIV > BODY > HTML > #document affinity:NSSelectionAffinityDownstream stillSelecting:FALSE
> +EDITING DELEGATE: shouldChangeSelectedDOMRange:(null) toDOMRange:range from 1 of #text > I > DIV > BODY > HTML > #document to 1 of #text > I > DIV > BODY > HTML > #document affinity:NSSelectionAffinityDownstream stillSelecting:FALSE
>  EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
>  EDITING DELEGATE: webViewDidChange:WebViewDidChangeNotification
> -EDITING DELEGATE: shouldChangeSelectedDOMRange:range from 1 of #text > SPAN > I > B > DIV > BODY > HTML > #document to 1 of #text > SPAN > I > B > DIV > BODY > HTML > #document toDOMRange:range from 2 of #text > SPAN > I > B > DIV > BODY > HTML > #document to 2 of #text > SPAN > I > B > DIV > BODY > HTML > #document affinity:NSSelectionAffinityDownstream stillSelecting:FALSE
> +EDITING DELEGATE: shouldChangeSelectedDOMRange:range from 1 of #text > I > DIV > BODY > HTML > #document to 1 of #text > I > DIV > BODY > HTML > #document toDOMRange:range from 2 of #text > I > DIV > BODY > HTML > #document to 2 of #text > I > DIV > BODY > HTML > #document affinity:NSSelectionAffinityDownstream stillSelecting:FALSE
>  EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
>  EDITING DELEGATE: webViewDidChange:WebViewDidChangeNotification
> -EDITING DELEGATE: shouldChangeSelectedDOMRange:range from 2 of #text > SPAN > I > B > DIV > BODY > HTML > #document to 2 of #text > SPAN > I > B > DIV > BODY > HTML > #document toDOMRange:range from 3 of #text > SPAN > I > B > DIV > BODY > HTML > #document to 3 of #text > SPAN > I > B > DIV > BODY > HTML > #document affinity:NSSelectionAffinityDownstream stillSelecting:FALSE
> +EDITING DELEGATE: shouldChangeSelectedDOMRange:range from 2 of #text > I > DIV > BODY > HTML > #document to 2 of #text > I > DIV > BODY > HTML > #document toDOMRange:range from 3 of #text > I > DIV > BODY > HTML > #document to 3 of #text > I > DIV > BODY > HTML > #document affinity:NSSelectionAffinityDownstream stillSelecting:FALSE
>  EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
>  EDITING DELEGATE: webViewDidChange:WebViewDidChangeNotification
>  EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
> -EDITING DELEGATE: shouldChangeSelectedDOMRange:(null) toDOMRange:range from 1 of #text > SPAN > SPAN > I > B > DIV > BODY > HTML > #document to 1 of #text > SPAN > SPAN > I > B > DIV > BODY > HTML > #document affinity:NSSelectionAffinityDownstream stillSelecting:FALSE
> +EDITING DELEGATE: shouldChangeSelectedDOMRange:(null) 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
> -EDITING DELEGATE: shouldChangeSelectedDOMRange:range from 1 of #text > SPAN > SPAN > I > B > DIV > BODY > HTML > #document to 1 of #text > SPAN > SPAN > I > B > DIV > BODY > HTML > #document toDOMRange:range from 2 of #text > SPAN > SPAN > I > B > DIV > BODY > HTML > #document to 2 of #text > SPAN > SPAN > I > B > DIV > BODY > HTML > #document affinity:NSSelectionAffinityDownstream stillSelecting:FALSE
> +EDITING DELEGATE: shouldChangeSelectedDOMRange:range from 1 of #text > DIV > BODY > HTML > #document to 1 of #text > DIV > BODY > HTML > #document toDOMRange:range from 2 of #text > DIV > BODY > HTML > #document to 2 of #text > DIV > BODY > HTML > #document affinity:NSSelectionAffinityDownstream stillSelecting:FALSE
>  EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
>  EDITING DELEGATE: webViewDidChange:WebViewDidChangeNotification
> -EDITING DELEGATE: shouldChangeSelectedDOMRange:range from 2 of #text > SPAN > SPAN > I > B > DIV > BODY > HTML > #document to 2 of #text > SPAN > SPAN > I > B > DIV > BODY > HTML > #document toDOMRange:range from 3 of #text > SPAN > SPAN > I > B > DIV > BODY > HTML > #document to 3 of #text > SPAN > SPAN > I > B > DIV > BODY > HTML > #document affinity:NSSelectionAffinityDownstream stillSelecting:FALSE
> +EDITING DELEGATE: shouldChangeSelectedDOMRange:range from 2 of #text > DIV > BODY > HTML > #document to 2 of #text > DIV > BODY > HTML > #document toDOMRange:range from 3 of #text > DIV > BODY > HTML > #document to 3 of #text > DIV > BODY > HTML > #document affinity:NSSelectionAffinityDownstream stillSelecting:FALSE
>  EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
>  EDITING DELEGATE: webViewDidChange:WebViewDidChangeNotification
>  xxxxxxxxxxxxxxx
> @@ -63,10 +63,10 @@ execTypeCharacterCommand: xxx<b>xxx<i>x<
>  execTypeCharacterCommand: xxx<b>xxx<i>xx</i></b><span id="test"></span>
>  execTypeCharacterCommand: xxx<b>xxx<i>xxx</i></b><span id="test"></span>
>  execBoldCommand: xxx<b>xxx<i>xxx</i></b><span id="test"></span>
> -execTypeCharacterCommand: xxx<b>xxx<i>xxx<span class="Apple-style-span" style="font-weight: normal;">x</span></i></b><span id="test"></span>
> -execTypeCharacterCommand: xxx<b>xxx<i>xxx<span class="Apple-style-span" style="font-weight: normal;">xx</span></i></b><span id="test"></span>
> -execTypeCharacterCommand: xxx<b>xxx<i>xxx<span class="Apple-style-span" style="font-weight: normal;">xxx</span></i></b><span id="test"></span>
> -execItalicCommand: xxx<b>xxx<i>xxx<span class="Apple-style-span" style="font-weight: normal;">xxx</span></i></b><span id="test"></span>
> -execTypeCharacterCommand: xxx<b>xxx<i>xxx<span class="Apple-style-span" style="font-weight: normal;">xxx<span class="Apple-style-span" style="font-style: normal;">x</span></span></i></b><span id="test"></span>
> -execTypeCharacterCommand: xxx<b>xxx<i>xxx<span class="Apple-style-span" style="font-weight: normal;">xxx<span class="Apple-style-span" style="font-style: normal;">xx</span></span></i></b><span id="test"></span>
> -execTypeCharacterCommand: xxx<b>xxx<i>xxx<span class="Apple-style-span" style="font-weight: normal;">xxx<span class="Apple-style-span" style="font-style: normal;">xxx</span></span></i></b><span id="test"></span>
> +execTypeCharacterCommand: xxx<b>xxx</b><i><b>xxx</b>x</i><span id="test"></span>
> +execTypeCharacterCommand: xxx<b>xxx</b><i><b>xxx</b>xx</i><span id="test"></span>
> +execTypeCharacterCommand: xxx<b>xxx</b><i><b>xxx</b>xxx</i><span id="test"></span>
> +execItalicCommand: xxx<b>xxx</b><i><b>xxx</b>xxx</i><span id="test"></span>
> +execTypeCharacterCommand: xxx<b>xxx</b><i><b>xxx</b>xxx</i>x<span id="test"></span>
> +execTypeCharacterCommand: xxx<b>xxx</b><i><b>xxx</b>xxx</i>xx<span id="test"></span>
> +execTypeCharacterCommand: xxx<b>xxx</b><i><b>xxx</b>xxx</i>xxx<span id="test"></span>
I assume the result is visually equivalent. Correct?
> Index: LayoutTests/editing/style/script-tests/push-down-implicit-styles.js
> ===================================================================
> --- LayoutTests/editing/style/script-tests/push-down-implicit-styles.js	(revision 0)
> +++ LayoutTests/editing/style/script-tests/push-down-implicit-styles.js	(revision 0)
> @@ -0,0 +1,76 @@
> +description('Test to make sure we push down inline styles properly.');
> +
> +var testContainer = document.createElement("div");
> +testContainer.contentEditable = true;
> +document.body.appendChild(testContainer);
> +
> +function testSingleToggle(toggleCommand, selector, initialContents, expectedContents)
> +{
> +    testContainer.innerHTML = initialContents;
> +    var selected = selector(testContainer);
> +    document.execCommand('styleWithCSS', false, 'false');
> +    document.execCommand(toggleCommand, false, null);
> +    var 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 selectTest(container) {
> +    window.getSelection().selectAllChildren(document.getElementById('test'));
> +    return 'test';
> +}
> +
> +function selectFirstWord(container) {
> +    window.getSelection().setPosition(container, 0);
> +    window.getSelection().modify('extend', 'forward', 'word');
> +    return 'first word';
> +}
> +
> +function selectSecondWord(container) {
> +    window.getSelection().setPosition(container, 0);
> +    window.getSelection().modify('move', 'forward', 'word');
> +    window.getSelection().modify('extend', 'forward', 'word');
> +    return 'second word';
> +}
> +
> +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';
> +}
> +
> +testSingleToggle("bold", selectFirstWord, '<b><div>hello</div> world</b>', '<div>hello</div><b> world</b>');
> +testSingleToggle("bold", selectFirstWord, '<b><div>hello</div>world</b>', '<div>hello</div><b>world</b>');
> +testSingleToggle("bold", selectFirstWord, '<b><div>hello</div><em>world</em></b>', '<div>hello</div><em style="font-weight: bold; ">world</em>');
> +testSingleToggle("bold", selectSecondWord, '<b>hello <div>world</div></b>', '<b>hello </b><div>world</div>');
> +testSingleToggle("bold", selectSecondWord, '<b><em>hello</em> <div>world</div></b>', '<em style="font-weight: bold; ">hello</em> <div>world</div>');
> +testSingleToggle("bold", selectAll, '<b> <div>text</div> </b>', ' <div>text</div> ');
> +testSingleToggle("bold", selectAll, '<b><s><div>text</div></s></b>', '<s><div>text</div></s>');
> +testSingleToggle("bold", selectFirstWord, '<b><div>hello</div><div>world</div></b>', '<div>hello</div><div style="font-weight: bold; ">world</div>');
> +testSingleToggle("bold", selectFirstWord, '<b><div>hello</div><div style="font-weight: normal;">world</div>webkit</b>', '<div>hello</div><div style="font-weight: normal; ">world</div><b>webkit</b>');
> +testSingleToggle("bold", selectSecondWord, '<b style="font-style: italic;">hello world</b>', '<b style="font-style: italic;">hello</b><span style="font-style: italic;"> world</span>');
> +
> +testSingleToggle("underline", selectSecondWord, '<u>hello <b>world</b> webkit</u>', '<u>hello</u> <b>world</b><u> webkit</u>');
> +testSingleToggle("underline", selectLastTwoWords, '<u>hello <b>world</b> webkit</u>', '<u>hello </u><b>world</b> webkit');
> +testSingleToggle("underline", selectLastTwoWords, '<u>hello <b>world webkit</b></u>', '<u>hello </u><b>world webkit</b>');
> +testSingleToggle("underline", selectSecondWord, '<u>hello <b>world webkit</b></u>', '<u>hello</u> <b>world<u> webkit</u></b>');
> +testSingleToggle("underline", selectSecondWord, '<u><b>hello world</b> webkit</u>', '<b><u>hello</u> world</b><u> webkit</u>');
> +testSingleToggle("underline", selectSecondWord, '<u><s>hello world</s></u>', '<s><u>hello</u> world</s>');
> +testSingleToggle("underline", selectSecondWord, '<u><s>hello world webkit</s></u>', '<s><u>hello</u> world<u> webkit</u></s>');
> +testSingleToggle("underline", selectSecondWord, '<u><s>hello world</s> webkit</u>', '<s><u>hello</u> world</s><u> webkit</u>');
> +testSingleToggle("underline", selectSecondWord, '<u>hello <em><code>world webkit</code></em> rocks</u>', '<u>hello</u> <em><code>world<u> webkit</u></code></em><u> rocks</u>');
> +
> +testSingleToggle("strikeThrough", selectAll, '<s style="color: blue;">hello world</s>', '<span style="color: blue;">hello world</span>');
> +testSingleToggle("strikeThrough", selectFirstWord, '<s style="color: blue;"><div>hello</div> <b>world</b> webkit</s>', '<span style="color: blue;"><div>hello</div> <b style="text-decoration: line-through; ">world</b><s> webkit</s></span>');
> +
> +document.body.removeChild(testContainer);
> +
> +var successfullyParsed = true;

The change looks good and there is a lot more testing now :-)!
My comments are just nit picks.
Thanks for working on this.

[Ryosuke Niwa]

(In reply to comment #16)
> > -bool ApplyStyleCommand::removeImplicitlyStyledElement(CSSMutableStyleDeclaration* style, HTMLElement* element, InlineStyleRemovalMode mode)
> > +bool ApplyStyleCommand::removeImplicitlyStyledElement(CSSMutableStyleDeclaration* style, HTMLElement* element, InlineStyleRemovalMode mode, CSSMutableStyleDeclaration* extractedStyle)
> >  {
> > +    // Current implementation does not support stylePushedDown when mode == RemoveNone because of early exit.
> > +    ASSERT(!extractedStyle || mode != RemoveNone);
> >      bool removed = false;
> >      for (size_t i = 0; i < sizeof(HTMLEquivalents) / sizeof(HTMLEquivalent); i++) {
> >          const HTMLEquivalent& equivalent = HTMLEquivalents[i];
> >          ASSERT(equivalent.element || equivalent.attribute);
> > -        if (equivalent.element && !element->hasTagName(*equivalent.element))
> > -            continue;
> > -        if (equivalent.attribute && !element->hasAttribute(*equivalent.attribute))
> > +        if (extractedStyle && equivalent.pushDownType == ShouldNotBePushedDown
> > +            || equivalent.element && !element->hasTagName(*equivalent.element)
> > +            || equivalent.attribute && !element->hasAttribute(*equivalent.attribute))
> >              continue;
> >  
> >          RefPtr<CSSValue> styleValue = style->getPropertyCSSValue(equivalent.propertyID);
> > @@ -1218,6 +1220,14 @@ bool ApplyStyleCommand::removeImplicitly
> >          else if (styleValue->cssText() == mapValue->cssText())
> >              continue; // If CSS value is primitive, then skip if they are equal.
> >  
> > +        if (extractedStyle) {
> > +            ASSERT(equivalent.pushDownType != ShouldNotBePushedDown);
> 
> I think the assert above is redundant.

Ok, I wanted to signify the fact above continue is required for this block because I was afraid that this condition wasn't clear from the content.  But I can remove if you think it's self-evident.

> >      Position pushDownStart = start.downstream();
> > +    // If the pushDownStart is at the end of a text node, then this node is not fully selected.
> > +    // Move it to the next deep quivalent position to avoid removing the style from this node.
> > +    // e.g. if pushDownStart was at Position("hello", 5) in <b>hello<div>world</div></b>, we want Position("world", 0) instead.
> > +    Node* pushDownStartContainer = pushDownStart.containerNode();
> > +    if (pushDownStartContainer && pushDownStartContainer->isTextNode() && pushDownStart.computeOffsetInContainerNode() == pushDownStartContainer->maxCharacterOffset())
> I would split this over two lines to ease of reading.

Will do.

> >  xxxxxxxxxxxxxxx
> > @@ -63,10 +63,10 @@ execTypeCharacterCommand: xxx<b>xxx<i>x<
> >  execTypeCharacterCommand: xxx<b>xxx<i>xx</i></b><span id="test"></span>
> >  execTypeCharacterCommand: xxx<b>xxx<i>xxx</i></b><span id="test"></span>
> >  execBoldCommand: xxx<b>xxx<i>xxx</i></b><span id="test"></span>
> > -execTypeCharacterCommand: xxx<b>xxx<i>xxx<span class="Apple-style-span" style="font-weight: normal;">x</span></i></b><span id="test"></span>
> > -execTypeCharacterCommand: xxx<b>xxx<i>xxx<span class="Apple-style-span" style="font-weight: normal;">xx</span></i></b><span id="test"></span>
> > -execTypeCharacterCommand: xxx<b>xxx<i>xxx<span class="Apple-style-span" style="font-weight: normal;">xxx</span></i></b><span id="test"></span>
> > -execItalicCommand: xxx<b>xxx<i>xxx<span class="Apple-style-span" style="font-weight: normal;">xxx</span></i></b><span id="test"></span>
> > -execTypeCharacterCommand: xxx<b>xxx<i>xxx<span class="Apple-style-span" style="font-weight: normal;">xxx<span class="Apple-style-span" style="font-style: normal;">x</span></span></i></b><span id="test"></span>
> > -execTypeCharacterCommand: xxx<b>xxx<i>xxx<span class="Apple-style-span" style="font-weight: normal;">xxx<span class="Apple-style-span" style="font-style: normal;">xx</span></span></i></b><span id="test"></span>
> > -execTypeCharacterCommand: xxx<b>xxx<i>xxx<span class="Apple-style-span" style="font-weight: normal;">xxx<span class="Apple-style-span" style="font-style: normal;">xxx</span></span></i></b><span id="test"></span>
> > +execTypeCharacterCommand: xxx<b>xxx</b><i><b>xxx</b>x</i><span id="test"></span>
> > +execTypeCharacterCommand: xxx<b>xxx</b><i><b>xxx</b>xx</i><span id="test"></span>
> > +execTypeCharacterCommand: xxx<b>xxx</b><i><b>xxx</b>xxx</i><span id="test"></span>
> > +execItalicCommand: xxx<b>xxx</b><i><b>xxx</b>xxx</i><span id="test"></span>
> > +execTypeCharacterCommand: xxx<b>xxx</b><i><b>xxx</b>xxx</i>x<span id="test"></span>
> > +execTypeCharacterCommand: xxx<b>xxx</b><i><b>xxx</b>xxx</i>xx<span id="test"></span>
> > +execTypeCharacterCommand: xxx<b>xxx</b><i><b>xxx</b>xxx</i>xxx<span id="test"></span>
> I assume the result is visually equivalent. Correct?

Yes.  Notice that the test result isn't unchanged until the line:
execBoldCommand: xxx<b>xxx<i>xxx</i></b><span id="test"></span>

Here, we're removing the bold from the typing style.  We then type xxx, we should put that xxx outside of b so we have:
execTypeCharacterCommand: xxx<b>xxx</b><i><b>xxx</b>xxx</i><span id="test"></span>
instead of
execTypeCharacterCommand: xxx<b>xxx<i>xxx<span class="Apple-style-span" style="font-weight: normal;">xxx</span></i></b><span id="test"></span>

Now, after typing xxx, we remove the italic from the typing style as well to get:
execItalicCommand: xxx<b>xxx</b><i><b>xxx</b>xxx</i><span id="test"></span>
Typing xxx here gives:
execTypeCharacterCommand: xxx<b>xxx</b><i><b>xxx</b>xxx</i>xxx<span id="test"></span>

which is visually identical to
execTypeCharacterCommand: xxx<b>xxx<i>xxx<span class="Apple-style-span" style="font-weight: normal;">xxx<span class="Apple-style-span" style="font-style: normal;">xxx</span></span></i></b><span id="test"></span>

IMHO, the new expected result looks nicer.

> > +testSingleToggle("bold", selectFirstWord, '<b><div>hello</div> world</b>', '<div>hello</div><b> world</b>');
> > +testSingleToggle("bold", selectFirstWord, '<b><div>hello</div>world</b>', '<div>hello</div><b>world</b>');
> > +testSingleToggle("bold", selectFirstWord, '<b><div>hello</div><em>world</em></b>', '<div>hello</div><em style="font-weight: bold; ">world</em>');
> > +testSingleToggle("bold", selectSecondWord, '<b>hello <div>world</div></b>', '<b>hello </b><div>world</div>');
> > +testSingleToggle("bold", selectSecondWord, '<b><em>hello</em> <div>world</div></b>', '<em style="font-weight: bold; ">hello</em> <div>world</div>');
> > +testSingleToggle("bold", selectAll, '<b> <div>text</div> </b>', ' <div>text</div> ');
> > +testSingleToggle("bold", selectAll, '<b><s><div>text</div></s></b>', '<s><div>text</div></s>');
> > +testSingleToggle("bold", selectFirstWord, '<b><div>hello</div><div>world</div></b>', '<div>hello</div><div style="font-weight: bold; ">world</div>');
> > +testSingleToggle("bold", selectFirstWord, '<b><div>hello</div><div style="font-weight: normal;">world</div>webkit</b>', '<div>hello</div><div style="font-weight: normal; ">world</div><b>webkit</b>');
> > +testSingleToggle("bold", selectSecondWord, '<b style="font-style: italic;">hello world</b>', '<b style="font-style: italic;">hello</b><span style="font-style: italic;"> world</span>');
> > +
> > +testSingleToggle("underline", selectSecondWord, '<u>hello <b>world</b> webkit</u>', '<u>hello</u> <b>world</b><u> webkit</u>');
> > +testSingleToggle("underline", selectLastTwoWords, '<u>hello <b>world</b> webkit</u>', '<u>hello </u><b>world</b> webkit');
> > +testSingleToggle("underline", selectLastTwoWords, '<u>hello <b>world webkit</b></u>', '<u>hello </u><b>world webkit</b>');
> > +testSingleToggle("underline", selectSecondWord, '<u>hello <b>world webkit</b></u>', '<u>hello</u> <b>world<u> webkit</u></b>');
> > +testSingleToggle("underline", selectSecondWord, '<u><b>hello world</b> webkit</u>', '<b><u>hello</u> world</b><u> webkit</u>');
> > +testSingleToggle("underline", selectSecondWord, '<u><s>hello world</s></u>', '<s><u>hello</u> world</s>');
> > +testSingleToggle("underline", selectSecondWord, '<u><s>hello world webkit</s></u>', '<s><u>hello</u> world<u> webkit</u></s>');
> > +testSingleToggle("underline", selectSecondWord, '<u><s>hello world</s> webkit</u>', '<s><u>hello</u> world</s><u> webkit</u>');
> > +testSingleToggle("underline", selectSecondWord, '<u>hello <em><code>world webkit</code></em> rocks</u>', '<u>hello</u> <em><code>world<u> webkit</u></code></em><u> rocks</u>');
> > +
> > +testSingleToggle("strikeThrough", selectAll, '<s style="color: blue;">hello world</s>', '<span style="color: blue;">hello world</span>');
> > +testSingleToggle("strikeThrough", selectFirstWord, '<s style="color: blue;"><div>hello</div> <b>world</b> webkit</s>', '<span style="color: blue;"><div>hello</div> <b style="text-decoration: line-through; ">world</b><s> webkit</s></span>');
> > +
> > +document.body.removeChild(testContainer);
> > +
> > +var successfullyParsed = true;
> 
> The change looks good and there is a lot more testing now :-)!
> My comments are just nit picks.
> Thanks for working on this.

Thanks for the comments.

[Ryosuke Niwa]

Created attachment 65765 [details]
fixed per enrica's comment

[Ryosuke Niwa]

Committed as http://trac.webkit.org/changeset/66324.