Bug 27156 – SplitElementCommand shouldn't be duplicating id attribute

• Component: HTML Editing
• Status: RESOLVED FIXED

[Ryosuke Niwa]

When SplitElementCommand is called for an element with id attribute, it generates two elements with the same id attributes.  Since the id attribute must be unique throughout the document, this behavior is wrong.

[Justin Garcia]

If you fix this verify that places that split and then merge soon after will continue to merge even though the second element isn't exactly identical.

[Justin Garcia]

I'm referring to mergeElementsIfIdentical (I think that's the function name).

[Ryosuke Niwa]

(In reply to comment #1)
> If you fix this verify that places that split and then merge soon after will continue to merge even though the second element isn't exactly identical.

I discussed this with Ojan but it seems like we shouldn't be merging elements unless we split the element.  Because imagine you have [hello<b id="test">world</b>] and bold the text.  If we had merged the elements, we'll get [<b id="test">helloworld</b>] but world may have style or some functionality provided by JavaScript.  And merging two elements together may result in loss of functionality.

(In reply to comment #2)
> I'm referring to mergeElementsIfIdentical (I think that's the function name).

areIdenticalElements in ApplyStyleCommand and MergeIdenticalElementsCommand need to be modified to do what you're proposing.

IMHO, the correct approach is not to split elements as much as possible in the first place.

[Ryosuke Niwa]

Created attachment 62141 [details]
work in progress

[Ryosuke Niwa]

Created attachment 62241 [details]
ready for feedback

Same approach but shouldSplitElement is much smarter in determining which element must be split.  I ended up hitting a bug in SplitTextNodeCommand (it wasn't implementing redo property) so the patch contains the fix for SplitTextNodeCommand as well.

[Tony Chang]

Comment on attachment 62241 [details]
ready for feedback

Should there be new layout tests?

WebCore/editing/ApplyStyleCommand.h:90
 +      bool shouldSplitElement(Element* elem, CSSMutableStyleDeclaration* style);
Nit: No variable names needed here.

WebCore/editing/ApplyStyleCommand.h:126
 +  RefPtr<CSSMutableStyleDeclaration> getPropertiesNotInComputedStyle(CSSStyleDeclaration* style, CSSStyleDeclaration* computedStyle);
Should this method now be renamed?  It's not operating on a computedStyle anymore.

WebCore/editing/ApplyStyleCommand.cpp:54
 +  static void reconcileTextDecorationProperties(CSSMutableStyleDeclaration* style)
This is just a refactoring right?  Might be good to do this in a separate change (although it's pretty clear here too).

WebCore/editing/ApplyStyleCommand.cpp:938
 +              isDiff = true;
Can we just return true here and stop checking the other styles?  If so, we could probably get rid of the isDiff variable.

[Ryosuke Niwa]

Thanks for the feedback!

(In reply to comment #6)
> (From update of attachment 62241 [details])
> Should there be new layout tests?

Yes, I'll be writing and adding a test for the actual patch.

> WebCore/editing/ApplyStyleCommand.h:90
>  +      bool shouldSplitElement(Element* elem, CSSMutableStyleDeclaration* style);
> Nit: No variable names needed here.

Will do.

> WebCore/editing/ApplyStyleCommand.h:126
>  +  RefPtr<CSSMutableStyleDeclaration> getPropertiesNotInComputedStyle(CSSStyleDeclaration* style, CSSStyleDeclaration* computedStyle);
> Should this method now be renamed?  It's not operating on a computedStyle anymore.

I was thinking about that too but couldn't think of a good name.  CSSStyleEditingDiff ?

> WebCore/editing/ApplyStyleCommand.cpp:54
>  +  static void reconcileTextDecorationProperties(CSSMutableStyleDeclaration* style)
> This is just a refactoring right?  Might be good to do this in a separate change (although it's pretty clear here too).

No, the reason I had to make it a static function was because I have to use it in shouldSplitElement.  ASSERT statement is also modified to be more flexible.

> WebCore/editing/ApplyStyleCommand.cpp:938
>  +              isDiff = true;
> Can we just return true here and stop checking the other styles?  If so, we could probably get rid of the isDiff variable.

Good idea.  I had a printf right beneath for debugging purposes but now that I think about it, isDiff is completely redundant.

[Ryosuke Niwa]

Created attachment 62324 [details]
fixed per Tony's comments

I replaced getPropertiesNotInComputedStyle by canonicalizedDiff in CSSStyleDeclaration since it makes more sense now for getPropertiesNotInComputedStyle to become a method now.  I recall someone (either mitz or dhaytt) didn't like this idea last time I proposed but I don't recall where I got this impression. I guess this change can be done in a separate patch. The rest is fixed as Tony suggested.

[Ryosuke Niwa]

Created attachment 62649 [details]
fixes the bug

Ready for review.  Per Justin's comment, I made sure to minimize unnecessary splits.  As a result, there is no layout tests that end up with extra tags.

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/editing/ApplyStyleCommand.cpp:1599:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
Total errors found: 1 in 13 files


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

[Ryosuke Niwa]

Created attachment 62650 [details]
Fixed the switch statement

[Ojan Vafai]

Comment on attachment 62650 [details]
Fixed the switch statement

It seems like you could separate the ApplyStyleCommand changes into a separate patch from the SplitElementCommand changes.

I don't follow the logic of how we decide if we should split. Is there code somewhere else that corresponds to this? I worry that this will fall out of sync with the code that depends on nodes being split or not.

> +bool ApplyStyleCommand::shouldSplitElement(Element* elem, CSSMutableStyleDeclaration* style)
> +{
> +    if (!elem || !elem->isHTMLElement() || !elem->parentElement() || !elem->parentElement()->isContentEditable())
> +        return false;
> +
> +    // If the element is of styleInlineElement, then split because commands such as createLink and unlink require this behavior
> +    if (m_styledInlineElement && elem->hasTagName(m_styledInlineElement->tagQName()))
> +        return true;
> +
> +    // If the element is one of presentational elements and interferes with the syle we're applying, then split
> +    if (implicitlyStyledElementShouldBeRemovedWhenApplyingStyle(static_cast<HTMLElement*>(elem), style))
> +        return true;
> +
> +    // If the element is a font tag and interferes with the style we're applying, then split
> +    if (elem->hasTagName(fontTag)) {
> +        CSSMutableStyleDeclaration::const_iterator end = style->end();
> +        for (CSSMutableStyleDeclaration::const_iterator it = style->begin(); it != end; ++it) {
> +            switch ((*it).id()) {
> +            case CSSPropertyColor:
> +                if (elem->hasAttribute(colorAttr))
> +                    return true;
> +                break;
> +            case CSSPropertyFontFamily:
> +                if (elem->hasAttribute(faceAttr))
> +                    return true;
> +                break;
> +            case CSSPropertyFontSize:
> +                if (elem->hasAttribute(sizeAttr))
> +                    return true;
> +                break;
> +            }
> +        }
> +    }
> +
> +    // Otherwise, compute the difference between the computed style and the syle we're applying.
> +    // If the difference doesn't match new style, then the element interferes, so split.
> +    RefPtr<CSSComputedStyleDeclaration> styleOfElem = computedStyle(elem);
> +    RefPtr<CSSComputedStyleDeclaration> styleOfParent = computedStyle(elem->parentNode());
> +    RefPtr<CSSMutableStyleDeclaration> diff = propertiesNotIn(styleOfElem.get(), styleOfParent.get());
> +    RefPtr<CSSMutableStyleDeclaration> styleToApply = style->copy();
> +
> +    bool isDiff = false;
> +    CSSMutableStyleDeclaration::const_iterator end = style->end();
> +    for (CSSMutableStyleDeclaration::const_iterator it = style->begin(); it != end; ++it) {
> +        const CSSProperty& property = *it;
> +        if (diff->getPropertyCSSValue(property.id()) && diff->getPropertyValue(property.id()) != property.cssText())
> +            isDiff = true;
> +    }
> +    return isDiff;
> +}


> +++ LayoutTests/ChangeLog	(working copy)
> +        Added a test to ensure splitting element doesn't produce multiple nodes with the same id.
> +        The test unbolds a text inside b element with id so as to split the element.
> +        It traverses through all nodes in under document.body to ensure that there is only one node with the id.

Don't need these last two sentences. It's clear from looking at the test that it does this. The ChangeLog should just give a high-level understanding of the changes and explain non-obvious bits.

> +++ LayoutTests/editing/style/split-element.html	(revision 0)
It might be easier to just:
document.execCommand('bold');
var test = document.getElementById('test');
test.parentNode.removeChild(test);
test = document.getElementById('test');
document.body.innerHTML = test ? 'FAIL ...' : 'PASS';

No need to keep a count of the nodes or anything.

> +<script type="text/javascript">

No need to include the type here.

> +function countNodesWithId(node, id)
> +{
> +    if (!(node instanceof Element))
> +        return 0;
> +

No need to make sure they are elements. There won't be non-element nodes on this page with an ID.

> +    var count = 0;
> +    if (node.id == id)
> +        count++;
> +
> +    for (var i = 0; i < node.childNodes.length; i++)
> +        count += countNodesWithId(node.childNodes[i], id);
> +
> +    return count;
> +}
> +
> +var range = document.createRange();
> +range.setStart(document.getElementById('test').firstChild, 1);
> +range.setEnd(document.getElementById('test').firstChild, 4);
> +window.getSelection().removeAllRanges();
> +window.getSelection().addRange(range);

var testNode = document.getElementById('test').firstChild;
window.getSelection().setBaseAndExtent(testNode, 1, testNode, 4);

[Ryosuke Niwa]

Thanks for the review as usual.

(In reply to comment #12)
> (From update of attachment 62650 [details])
> It seems like you could separate the ApplyStyleCommand changes into a separate patch from the SplitElementCommand changes.
> 
> I don't follow the logic of how we decide if we should split. Is there code somewhere else that corresponds to this? I worry that this will fall out of sync with the code that depends on nodes being split or not.

Take a look at removeInlineStyle.  Everything that is touched by removeInlineStyle needs to be split:

            if (m_styledInlineElement && elem->hasTagName(m_styledInlineElement->tagQName()))
                removeNodePreservingChildren(elem);

            if (implicitlyStyledElementShouldBeRemovedWhenApplyingStyle(elem, style.get()))
                replaceWithSpanOrRemoveIfWithoutAttributes(elem);

            // If the node was converted to a span, the span may still contain relevant
            // styles which must be removed (e.g. <b style='font-weight: bold'>)
            if (elem->inDocument()) {
                removeHTMLFontStyle(style.get(), elem);
                removeHTMLBidiEmbeddingStyle(style.get(), elem);
                removeCSSStyle(style.get(), elem);
            }

The reason we need to split an element is because we need to remove the style from either the start or the end of the selection.  So, m_styledInlineElement (only used for createLink, unlink) and implicitlyStyledElementShouldBeRemovedWhenApplyingStyle are straight from these two lines.  Now, the comment sounds as if removeHTMLFontStyle removes font style from a span but that's completely wrong.  removeHTMLFontStyle only removes attributes of the font tag.  So this results in the third check.  Finally, removeCSSStyle takes care of the CSS styles.

> > +    // Otherwise, compute the difference between the computed style and the syle we're applying.
> > +    // If the difference doesn't match new style, then the element interferes, so split.
> > +    RefPtr<CSSComputedStyleDeclaration> styleOfElem = computedStyle(elem);
> > +    RefPtr<CSSComputedStyleDeclaration> styleOfParent = computedStyle(elem->parentNode());
> > +    RefPtr<CSSMutableStyleDeclaration> diff = propertiesNotIn(styleOfElem.get(), styleOfParent.get());
> > +    RefPtr<CSSMutableStyleDeclaration> styleToApply = style->copy();

This part comes from removeCSSStyle.  Now that I think about, I should have used elem->inlineStyleDecl(); for diff.  Will fix this.

> > +++ LayoutTests/ChangeLog	(working copy)
> > +        Added a test to ensure splitting element doesn't produce multiple nodes with the same id.
> > +        The test unbolds a text inside b element with id so as to split the element.
> > +        It traverses through all nodes in under document.body to ensure that there is only one node with the id.
> 
> Don't need these last two sentences. It's clear from looking at the test that it does this. The ChangeLog should just give a high-level understanding of the changes and explain non-obvious bits.

Will fix.

> It might be easier to just:
> document.execCommand('bold');
> var test = document.getElementById('test');
> test.parentNode.removeChild(test);
> test = document.getElementById('test');
> document.body.innerHTML = test ? 'FAIL ...' : 'PASS';
> 
> No need to keep a count of the nodes or anything.

The reason I did that was for debugging purposes.  Should this test fail in the future, we can easily see what's happening.  If I had just said PASS or FAIL, it's not instantly clear whether the node is disappearing or we're producing nodes with the same id.  Also, Firefox currently produce 2 nodes with the same id but WebKit without my patch produces 3.  So I wanted to distinguish those things as well to make the future debugging etc.. easily.

> var testNode = document.getElementById('test').firstChild;
> window.getSelection().setBaseAndExtent(testNode, 1, testNode, 4);

I wanted to make this test run on Firefox so that we can compare behaviors easily (Firefox fails).  Ideally, I make it run on MSIE as well but I decided that's too much trouble.

[Ryosuke Niwa]

Created attachment 62790 [details]
fixed per ojan's comments

Maybe we can extract removeInlineStyleFromElement in a separate patch but then adding dontRemove in that cleanup patch would be weird.

[Darin Adler]

Comment on attachment 62790 [details]
fixed per ojan's comments

I didn't do a review, but I did spend a moment looking at style. Here is one style comment. I hope to be able to review this bug later, but that should not prevent someone else from doing so.

> +bool ApplyStyleCommand::removeInlineStyleFromElement(CSSMutableStyleDeclaration *style, HTMLElement *elem, bool dontRemove)

I don’t know why the style bot is not complaining about this. The "*" character should be next to the types.

Please use a word, "element", instead of a word fragment, "elem". In general we should keep abbreviations to a minimum in new code.

[Ryosuke Niwa]

Created attachment 62856 [details]
fixed style per darin's comment

(In reply to comment #15)
> (From update of attachment 62790 [details])
> > +bool ApplyStyleCommand::removeInlineStyleFromElement(CSSMutableStyleDeclaration *style, HTMLElement *elem, bool dontRemove)
> 
> I don’t know why the style bot is not complaining about this. The "*" character should be next to the types.

Ugh... that's why I shouldn't copy & paste to create a new method.  Sorry about that.

> Please use a word, "element", instead of a word fragment, "elem". In general we should keep abbreviations to a minimum in new code.

Fixed.

[Ojan Vafai]

Comment on attachment 62856 [details]
fixed style per darin's comment

WebCore/editing/ApplyStyleCommand.cpp:1185
 +  bool ApplyStyleCommand::removeInlineStyleFromElement(CSSMutableStyleDeclaration* style, HTMLElement* element, bool dontRemove)
Generally, don't name variable names with negatives in them. invert this and call it shouldRemove instead. Also, Darin will kill me if I let you add a boolean argument. Use an enum instead. That way, the meaning of the argument is clear from the calling context.

> +    // If the node was converted to a span, the span may still contain relevant
> +    // styles which must be removed (e.g. <b style='font-weight: bold'>)
> +    if (removeHTMLFontStyle(style, element, dontRemove))
> +        removed = true;
> +    if (removeHTMLBidiEmbeddingStyle(style, element, dontRemove))
> +        removed = true;
> +    if (removeCSSStyle(style, element, dontRemove))
> +        removed = true;

This can be a single if-statement:
if (removeHTMLFontStyle(style, element, dontRemove)
    || removeHTMLBidiEmbeddingStyle(style, element, dontRemove)
    || removeCSSStyle(style, element, dontRemove))
    removed = true;

> -    if (isEmptyFontTag(elem))
> +    if (isEmptyFontTag(elem) && !dontRemove)
>          removeNodePreservingChildren(elem);

Shouldn't we set removed=true here?

>      // FIXME: should this be isSpanWithoutAttributesOrUnstyleStyleSpan?  Need a test.
>      if (isUnstyledStyleSpan(elem))
>          removeNodePreservingChildren(elem);

Ditto.

> -        splitTextElementAtStart(start, end);
> +        if (shouldSplitElement(start.node()->parentElement(), style))
> +            splitTextElementAtStart(start, end);
> +        else
> +            splitTextAtStart(start, end);

> -        splitTextElementAtEnd(start, end);
> +        if (shouldSplitElement(end.node()->parentElement(), style))
> +            splitTextElementAtEnd(start, end);
> +        else
> +            splitTextAtEnd(start, end);

Maybe the method should be called shouldSplitTextElement to match splitTextElementAt*?

[Ryosuke Niwa]

(In reply to comment #17)
> (From update of attachment 62856 [details])
> WebCore/editing/ApplyStyleCommand.cpp:1185
>  +  bool ApplyStyleCommand::removeInlineStyleFromElement(CSSMutableStyleDeclaration* style, HTMLElement* element, bool dontRemove)
> Generally, don't name variable names with negatives in them. invert this and call it shouldRemove instead. Also, Darin will kill me if I let you add a boolean argument. Use an enum instead. That way, the meaning of the argument is clear from the calling context.

Will do.

> > +    // If the node was converted to a span, the span may still contain relevant
> > +    // styles which must be removed (e.g. <b style='font-weight: bold'>)
> > +    if (removeHTMLFontStyle(style, element, dontRemove))
> > +        removed = true;
> > +    if (removeHTMLBidiEmbeddingStyle(style, element, dontRemove))
> > +        removed = true;
> > +    if (removeCSSStyle(style, element, dontRemove))
> > +        removed = true;
> 
> This can be a single if-statement:
> if (removeHTMLFontStyle(style, element, dontRemove)
>     || removeHTMLBidiEmbeddingStyle(style, element, dontRemove)
>     || removeCSSStyle(style, element, dontRemove))
>     removed = true;

No. I did that first not realizing removeCSSStyle can be processing the nodes that have also been processed by removeHTMLFontStyle and removeHTMLBidiEmbeddingStyle.  Had I used ||, removeCSSStyle will not be called on any nodes that have been processed by removeHTMLFontStyle or removeHTMLBidiEmbeddingStyle.

> > -    if (isEmptyFontTag(elem))
> > +    if (isEmptyFontTag(elem) && !dontRemove)
> >          removeNodePreservingChildren(elem);
> 
> Shouldn't we set removed=true here?

No. We should split even if the node was fully removed.  Removing attribute will cause the style to change so not splitting will result in the not fully selected nodes' style to modify if we moved removed=true here.

> > -        splitTextElementAtStart(start, end);
> > +        if (shouldSplitElement(start.node()->parentElement(), style))
> > +            splitTextElementAtStart(start, end);
> > +        else
> > +            splitTextAtStart(start, end);
> 
> > -        splitTextElementAtEnd(start, end);
> > +        if (shouldSplitElement(end.node()->parentElement(), style))
> > +            splitTextElementAtEnd(start, end);
> > +        else
> > +            splitTextAtEnd(start, end);
> 
> Maybe the method should be called shouldSplitTextElement to match splitTextElementAt*?

Will fix.

[Ryosuke Niwa]

Created attachment 63012 [details]
fixed per ojan's comments

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/editing/ApplyStyleCommand.h:75:  Missing spaces around =  [whitespace/operators] [4]
WebCore/editing/ApplyStyleCommand.h:77:  Missing spaces around =  [whitespace/operators] [4]
WebCore/editing/ApplyStyleCommand.h:78:  Missing spaces around =  [whitespace/operators] [4]
WebCore/editing/ApplyStyleCommand.h:79:  Missing spaces around =  [whitespace/operators] [4]
Total errors found: 4 in 13 files


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

[Ryosuke Niwa]

Created attachment 63014 [details]
fixed the style

[Ojan Vafai]

Comment on attachment 63014 [details]
fixed the style

> +    enum InlineStyleRemovalMode { RemoveAttributesAndElement, RemoveNone };

s/RemoveAttributesAndElement/RemoveAttributesAndElements

> +    bool removeInlineStyleFromElement(CSSMutableStyleDeclaration*, HTMLElement*, InlineStyleRemovalMode = RemoveAttributesAndElement);
> +    bool removeHTMLFontStyle(CSSMutableStyleDeclaration*, HTMLElement*, InlineStyleRemovalMode = RemoveAttributesAndElement);
> +    bool removeHTMLBidiEmbeddingStyle(CSSMutableStyleDeclaration*, HTMLElement*, InlineStyleRemovalMode = RemoveAttributesAndElement);
> +    bool removeCSSStyle(CSSMutableStyleDeclaration*, HTMLElement*, InlineStyleRemovalMode = RemoveAttributesAndElement);

Typically, returning a boolean value from a method indicates whether it succeeded or not. In this case, it indicates whether we would have removed something. Which isn't exactly the same a success. Can we return an enum here as well?

[Ryosuke Niwa]

Created attachment 63113 [details]
fixed per ojan's comments


(In reply to comment #22)
> (From update of attachment 63014 [details])
> > +    enum InlineStyleRemovalMode { RemoveAttributesAndElement, RemoveNone };
> 
> s/RemoveAttributesAndElement/RemoveAttributesAndElements

Done.

> > +    bool removeInlineStyleFromElement(CSSMutableStyleDeclaration*, HTMLElement*, InlineStyleRemovalMode = RemoveAttributesAndElement);
> > +    bool removeHTMLFontStyle(CSSMutableStyleDeclaration*, HTMLElement*, InlineStyleRemovalMode = RemoveAttributesAndElement);
> > +    bool removeHTMLBidiEmbeddingStyle(CSSMutableStyleDeclaration*, HTMLElement*, InlineStyleRemovalMode = RemoveAttributesAndElement);
> > +    bool removeCSSStyle(CSSMutableStyleDeclaration*, HTMLElement*, InlineStyleRemovalMode = RemoveAttributesAndElement);
> 
> Typically, returning a boolean value from a method indicates whether it succeeded or not. In this case, it indicates whether we would have removed something. Which isn't exactly the same a success. Can we return an enum here as well?

I can't agree on this. Adding another enum will make the functions too verbose IMHO. And there are plenty of member functions such as mergeStartWithPreviousIfIdentical whose return value indicates whether or not the operation was done or not. It's pretty standard that remove function always succeed and therefore have a void type. So remove* being a boolean type pretty much tells us that it's indicating whether or not attributes / elements have been (or should have been removed) or not.

In general, I'm not a big fan of using enum for something that doesn't have more than two states other than function argument. If we had boolean variable, then I can tell immediately that it can be either true or false. But if I had enum, there could be more than two states so I have to think twice when reading the code. I agree that the code calling a method with enum is much easier to read than the one calling with a boolean variable because I have to check the function declaration to know what it is. However, when I read the code of these methods as is, I have to wonder if there are such thing as RemoveElements or RemoveAttributes and if conditions checking with == are right or not.  If we made the method to return enum type, then caller will check it against Removed or NotRemoved.  But since it's enum type, a person reading the code will wonder whether there are some other states.

[Ojan Vafai]

> I can't agree on this. Adding another enum will make the functions too verbose IMHO. And there are plenty of member functions such as mergeStartWithPreviousIfIdentical whose return value indicates whether or not the operation was done or not. It's pretty standard that remove function always succeed and therefore have a void type. So remove* being a boolean type pretty much tells us that it's indicating whether or not attributes / elements have been (or should have been removed) or not.

The problematic case is something like:
removeInlineStyleFromElement(foo, bar, RemoveNone)

Since it's not actually removing anything, what does success or failure mean here? The return value isn't actually a measure of whether the call succeeded. It's a measure fo whether the call would have succeeded if you passed in a different enum value. That's the part I find confusing.

Where something like the following is more clear:
WouldRemove == removeInlineStyleFromElement(foo, bar, RemoveNone)

[Ryosuke Niwa]

(In reply to comment #24)
> Since it's not actually removing anything, what does success or failure mean here? The return value isn't actually a measure of whether the call succeeded. It's a measure fo whether the call would have succeeded if you passed in a different enum value. That's the part I find confusing.
> 
> Where something like the following is more clear:
> WouldRemove == removeInlineStyleFromElement(foo, bar, RemoveNone)

WouldRemove will look nice for that case but then if I did
removeInlineStyleFromElement(foo, bar, RemoveAttributesAndElements)

it'll be quite strange for me to check against "WouldRemove" as the attributes and elements have already been removed.  But if we made removeInlineStyleFromElement to return both WouldRemove and Removed depending on the mode passed in, it'll add unnecessary complexity to the code that's already way too complicated in my taste.

[Ryosuke Niwa]

Created attachment 63121 [details]
added shouldRemoveInlineStyleFromElement

[Ryosuke Niwa]

Landed as http://trac.webkit.org/changeset/64432.