Bug 20215 – execCommand can't remove presentational tags (u, s, & strike)

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[David Bloom]

1. go to http://www.mozilla.org/editor/midasdemo/
2. check "View HTML source"
3. set the source to: "<u>test test test</u>"
4. uncheck "View HTML source"
5. select the middle "test"
6. underline the text using toolbar
7. text stays underlined
expected result: <u>test </u>test <u>test</u>
actual result: <u>test </u><u style=""><span class="Apple-style-span" style="text-decoration: none;">test</span></u><u> test</u>

[Eric Seidel (no email)]

Ahha!

bool ApplyStyleCommand::isHTMLStyleNode(CSSMutableStyleDeclaration *style, HTMLElement *elem)
{
    CSSMutableStyleDeclaration::const_iterator end = style->end();
    for (CSSMutableStyleDeclaration::const_iterator it = style->begin(); it != end; ++it) {
        switch ((*it).id()) {
            case CSSPropertyFontWeight:
                if (elem->hasLocalName(bTag))
                    return true;
                break;
            case CSSPropertyFontStyle:
                if (elem->hasLocalName(iTag))
                    return true;
        }
    }

    return false;
}

[Eric Seidel (no email)]

This is our current behavior:

<u style=""><span class="Apple-style-span" style="text-decoration: none;">foo</span></u>

Which seems at least partially caused by this code:

void ApplyStyleCommand::removeInlineStyle(PassRefPtr<CSSMutableStyleDeclaration>
style, const Position &start, const Position &end)


   RefPtr<CSSValue> textDecorationSpecialProperty =
style->getPropertyCSSValue(CSSPropertyWebkitTextDecorationsInEffect);

   if (textDecorationSpecialProperty) {
       pushDownTextDecorationStyleAtBoundaries(start.downstream(),
end.upstream());
       style = style->copy();
       style->setProperty(CSSPropertyTextDecoration,
textDecorationSpecialProperty->cssText(),
style->getPropertyPriority(CSSPropertyWebkitTextDecorationsInEffect));
   }

Which I don't understand why it exists.

[Eric Seidel (no email)]

This is basically the same as bug

I've written a test.  Our current results:

Test to make sure we remove span tags with no attributes if we removed the last attribute.

On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".


FAIL one underline command converted test to <span class="Apple-style-span" style="text-decoration: underline;">test</span>, expected <u>test</u>
PASS two underline commands converted test to test
FAIL one strikethrough command converted test to <span class="Apple-style-span" style="text-decoration: line-through;">test</span>, expected <s>test</s>
PASS two strikethrough commands converted test to test
FAIL one strikethrough command converted <u>test</u> to <u><span class="Apple-style-span" style="text-decoration: line-through;">test</span></u>, expected <s><u>test</u></s>
FAIL one underline command converted <s>test</s> to <s><span class="Apple-style-span" style="text-decoration: underline;">test</span></s>, expected <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: line-through;">test</span>, expected <s><span style='text-decoration: overline'>test</span></s>
FAIL one underline command converted <span style='text-decoration: overline'>test</span> to <span class="Apple-style-span" style="text-decoration: underline;">test</span>, expected <u><span style='text-decoration: overline'>test</span></u>
PASS successfullyParsed is true

TEST COMPLETE


Of course FF does even worse, since FF seems to want to put the styling on the containing div, instead of inserting a <u> or a <span> inside the editable div (like my results expect).

On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
FAIL one underline command converted test to test, expected <u>test</u>
PASS two underline commands converted test to test
FAIL one strikethrough command converted test to test, expected <s>test</s>
PASS two strikethrough commands converted test to test
FAIL one strikethrough command converted <u>test</u> to <u>test</u>, expected <s><u>test</u></s>
FAIL one underline command converted <s>test</s> to <s>test</s>, expected <u><s>test</s></u>
FAIL one strikethrough command converted <span style='text-decoration: overline'>test</span> to <span style="text-decoration: overline;">test</span>, expected <s><span style='text-decoration: overline'>test</span></s>
FAIL one underline command converted <span style='text-decoration: overline'>test</span> to <span style="text-decoration: overline;">test</span>, expected <u><span style='text-decoration: overline'>test</span></u>
PASS successfullyParsed is true

TEST COMPLETE

[Eric Seidel (no email)]

If anyone has suggestions as to how I should improve my test so that I can get FF to do more of what I would expect, I'm all ears.  I could insert some dummy tags/letters between the test content and the enclosing <div> to prevent FF from adding the styles to the enclosing div.  But I'd ideally like to find a slicker solution.

[Eric Seidel (no email)]

Created attachment 26960 [details]
the js portion of my current testcase (drop in editing/execCommand/resources to use)

[Eric Seidel (no email)]

When I fix this, it will also fix bug 22810.

[Eric Seidel (no email)]

Hum... I think I need to fix how we remove styling tags during editing command execution first before I can really fix this stuff in a sane way.  ApplyStyleCommand needs to be smarter about what styles an html styling tag (like b, i, u, s, etc.) actually is applying to the content in question.  Until isHTMLStyleNode and removeHTMLStyleNode can be fixed to correctly handle things like <u style="text-decoration: strikethrough"> we can't use the removeHTMLStyleNode logic from within the text-decoration codepath and thus are left with our current "just wrap it in a span with a canceling style" behavior.

[Eric Seidel (no email)]

Per my above comment, marking this as blocked by bug 23496.

[Eric Seidel (no email)]

This does not have to be blocked by bug 23496.

This bug is tricky to fix, because to do it right we'll need to fix how toggleCSSStyle works.  Right now the ApplyStyleCommand (which gets generated by the toggle callback) does not really know that it's supposed to remove just one CSS value from a CSSValueList.  Instead, it knows how to set a css property to a specific value.  We may need to make a whole new Command to handle text-decoration (or toggles in general).

[Julie Parent]

*** Bug 26610 has been marked as a duplicate of this bug. ***

[Ryosuke Niwa]

Created attachment 33318 [details]
fixes the bug (non-CSS mode still unsupported)

[Eric Seidel (no email)]

Comment on attachment 33318 [details]
fixes the bug (non-CSS mode still unsupported)

Just use a raw pointer:
 967                     RefPtr<CSSValueList> valueList = static_cast<CSSValueList*>(property.value());

These should be static:
                     RefPtr<CSSPrimitiveValue> underline = CSSPrimitiveValue::createIdentifier(CSSValueUnderline);
 969                     RefPtr<CSSPrimitiveValue> lineThrough = CSSPrimitiveValue::createIdentifier(CSSValueLineThrough);
 97
Watch out for the LOCAL_STATIC macro.  You can't just use "static" here due to a GCC bug.

Otherwise this looks great.  I'd like to see the patch one more time though.

[Ryosuke Niwa]

Created attachment 33369 [details]
fixed per comment

[Eric Seidel (no email)]

Comment on attachment 33369 [details]
fixed per comment

Should add a comment before:
 104     ASSERT(!mutableStyle->getPropertyCSSValue(CSSPropertyTextDecoration) || !mutableStyle->getPropertyCSSValue(CSSPropertyWebkitTextDecorationsInEffect));

This comment is confusing:
 132             // Always use text-decoration NOT to expose -webkit-text-decoration-in-effect.

Otherwise looks great!

[Ryosuke Niwa]

Landed in http://trac.webkit.org/changeset/46286.