Bug 20215

Summary: execCommand can't remove presentational tags (u, s, & strike)
Product: WebKit Reporter: David Bloom <futurama>
Component: HTML EditingAssignee: Ryosuke Niwa <rniwa>
Status: RESOLVED FIXED    
Severity: Normal CC: eric, jparent, justin.garcia, ojan, vkarun
Priority: P2 Keywords: GoogleBug
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 23366, 23496    
Bug Blocks: 22810, 23892, 24333    
Attachments:
Description Flags
the js portion of my current testcase (drop in editing/execCommand/resources to use)
none
fixes the bug (non-CSS mode still unsupported)
eric: review-
fixed per comment eric: review+

Description David Bloom 2008-07-29 14:31:31 PDT
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>
Comment 1 Eric Seidel (no email) 2009-01-15 13:31:42 PST
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;
}
Comment 2 Eric Seidel (no email) 2009-01-15 13:49:59 PST
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.
Comment 3 Eric Seidel (no email) 2009-01-23 00:30:54 PST
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
Comment 4 Eric Seidel (no email) 2009-01-23 00:34:22 PST
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.
Comment 5 Eric Seidel (no email) 2009-01-23 00:35:01 PST
Created attachment 26960 [details]
the js portion of my current testcase (drop in editing/execCommand/resources to use)
Comment 6 Eric Seidel (no email) 2009-01-23 00:53:44 PST
When I fix this, it will also fix bug 22810.
Comment 7 Eric Seidel (no email) 2009-01-23 00:58:25 PST
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.
Comment 8 Eric Seidel (no email) 2009-01-23 00:59:03 PST
Per my above comment, marking this as blocked by bug 23496.
Comment 9 Eric Seidel (no email) 2009-02-11 00:39:57 PST
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).
Comment 10 Julie Parent 2009-06-22 10:54:36 PDT
*** Bug 26610 has been marked as a duplicate of this bug. ***
Comment 11 Ryosuke Niwa 2009-07-23 00:01:43 PDT
Created attachment 33318 [details]
fixes the bug (non-CSS mode still unsupported)
Comment 12 Eric Seidel (no email) 2009-07-23 13:02:20 PDT
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.
Comment 13 Ryosuke Niwa 2009-07-23 13:56:08 PDT
Created attachment 33369 [details]
fixed per comment
Comment 14 Eric Seidel (no email) 2009-07-23 14:01:50 PDT
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!
Comment 15 Ryosuke Niwa 2009-07-23 14:20:54 PDT
Landed in http://trac.webkit.org/changeset/46286.