Bug 23496 – execCommand('bold') will remove <b style='font-style:italic'>, it should probably replace it with <span style='font-style:italic'

• Component: HTML Editing
• Status: RESOLVED FIXED

[Eric Seidel (no email)]

execCommand('bold') will remove <b foo='bar'>, it should probably replace it with <span foo='bar'>.

I'm not sure what the proper behavior here is.

Our current (epic fail!) behavior is:

Test to make sure we do not remove extra styling hidden on html styling elements (b, i, s, etc.) when removing those elements.

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


FAIL one underline command converted <u style='font-weight: bold'>test</u> to <u style="font-weight: bold; "><span class="Apple-style-span" style="text-decoration: none;">test</span></u>, expected <b>test</b>
FAIL one bold command converted <b style='text-decoration: underline'>test</b> to test, expected <u>test</u>
FAIL one strikethrough command converted <s style='text-decoration: underline; font-weight: bold'>test</s> to <s style="font-weight: bold; ">test</s>, expected <b><u>test</u></b>
FAIL one bold command converted <b style='text-decoration: overline'>test</b> to test, expected <span style='text-decoration: overline'>test</span>
FAIL one underline command converted <u foo='bar'>test</u> to <u foo="bar" style=""><span class="Apple-style-span" style="text-decoration: none;">test</span></u>, expected <span foo='bar'>test</span>
PASS successfullyParsed is true

TEST COMPLETE

[Eric Seidel (no email)]

Firefox did slightly better:

Test to make sure we do not remove extra styling hidden on html styling elements (b, i, s, etc.) when removing those elements.

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

TEST COMPLETE

At least their styling looks correct after the commands, even if the resulting html doesn't look exactly as expected.  FF also fails to maintain foo="bar", which I'm not sure if we care about our not.

[Eric Seidel (no email)]

Created attachment 26959 [details]
javascript portion of the test case in question (drop in editing/execCommands to use)

[Eric Seidel (no email)]

I've written a skeleton patch to fix this, which I'm about to attach.  I think I may need some advice from Justin as to how best write my proposed replaceNodeWithSpanPreservingAttributesAndChildren method, since I don't think editing has had to deal with "attribute transfer" between nodes before.

[Eric Seidel (no email)]

Created attachment 26966 [details]
First pass, incomplete fix

 WebCore/css/CSSStyleDeclaration.h     |    1 +
 WebCore/dom/NamedAttrMap.h            |    1 +
 WebCore/editing/ApplyStyleCommand.cpp |   75 +++++++++++++++++++-------------
 WebCore/editing/ApplyStyleCommand.h   |    3 +-
 4 files changed, 47 insertions(+), 33 deletions(-)

[Eric Seidel (no email)]

I might just turn replaceImplicitlyStyledElementWithSpanAndRemoveIfWithoutAttributes into replaceImplictlyStyledElementWithSpan, or even replaceHTMLStyleNodeWithSpan (to be more like the old naming, and more concise), of course that would mean that we would end up converting <b> to <span> every time before we ended up removing it, and that seems a bit wasteful.   Maybe others will have better suggestions of how to divide the logic (or name these fancy new methods) once I get a working patch together.

[Eric Seidel (no email)]

Sigh.  This is why I stopped writing patches @ 2AM.  I can definitely make this cleaner.  I still will likely need some thoughts from Justin as to how I should do any "replaceWithSpan" logic in a proper "undoable editing" way.

[Eric Seidel (no email)]

Created attachment 27306 [details]
Fix and test case

 LayoutTests/ChangeLog                              |   14 +++
 .../convert-style-elements-to-spans-expected.txt   |   14 +++
 .../convert-style-elements-to-spans.html           |   13 +++
 .../resources/convert-style-elements-to-spans.js   |   47 +++++++++++
 WebCore/ChangeLog                                  |   40 +++++++++
 WebCore/WebCore.xcodeproj/project.pbxproj          |    8 ++
 WebCore/css/CSSStyleDeclaration.h                  |    1 +
 WebCore/dom/NamedAttrMap.h                         |    2 +
 WebCore/editing/ApplyStyleCommand.cpp              |   70 ++++++++++------
 WebCore/editing/ApplyStyleCommand.h                |    4 +-
 WebCore/editing/CompositeEditCommand.cpp           |    6 ++
 WebCore/editing/CompositeEditCommand.h             |    1 +
 .../RemoveNodePreservingChildrenCommand.cpp        |    3 +-
 WebCore/editing/ReplaceNodeWithSpanCommand.cpp     |   87 ++++++++++++++++++++
 WebCore/editing/ReplaceNodeWithSpanCommand.h       |   60 ++++++++++++++
 15 files changed, 341 insertions(+), 29 deletions(-)

[Eric Seidel (no email)]

The double-bold case is still not fixed.  It's not made worse either by this change, but I may yet have the stamina for one more round of debugging for that case.

I post this w/ that one case broken, as I require Justin's comment about whether or not I wrote ReplaceNodeWithSpanCommand correctly or not.

[Eric Seidel (no email)]

(In reply to comment #8)
> The double-bold case is still not fixed.  It's not made worse either by this
> change, but I may yet have the stamina for one more round of debugging for that
> case.

I realized last night why the double-bold case is broken.

void ApplyStyleCommand::replaceWithSpanOrRemoveIfWithoutAttributes(HTMLElement*& elem)

is supposed to update the "elem" pointer so that the caller knows what the element has turned into, and can continue processing it.  When I updated that function to use an editing command (so i would be undoable), I lost that functionality.  I'm not quite sure how I should get it back yet.

I could add a ::spanElement() accessor on the command, but I don't think that will work well in the undo case, not sure.  Starting to seem like I'll need to test undo of this command too.  I'd still be very interested in Justin's comment here.

[Eric Seidel (no email)]

Created attachment 27325 [details]
Improved fix and improved test case, ready for final review

 LayoutTests/ChangeLog                              |   14 ++
 .../convert-style-elements-to-spans-expected.txt   |   20 +++
 .../convert-style-elements-to-spans.html           |   13 ++
 .../resources/convert-style-elements-to-spans.js   |  125 ++++++++++++++++++++
 WebCore/ChangeLog                                  |   40 ++++++
 WebCore/WebCore.xcodeproj/project.pbxproj          |    8 ++
 WebCore/css/CSSStyleDeclaration.h                  |    1 +
 WebCore/dom/NamedAttrMap.h                         |    2 +
 WebCore/editing/ApplyStyleCommand.cpp              |   73 ++++++++----
 WebCore/editing/ApplyStyleCommand.h                |    4 +-
 WebCore/editing/CompositeEditCommand.cpp           |   12 ++
 WebCore/editing/CompositeEditCommand.h             |    2 +
 .../RemoveNodePreservingChildrenCommand.cpp        |    3 +-
 WebCore/editing/ReplaceNodeWithSpanCommand.cpp     |   87 ++++++++++++++
 WebCore/editing/ReplaceNodeWithSpanCommand.h       |   62 ++++++++++
 15 files changed, 437 insertions(+), 29 deletions(-)

[Justin Garcia]

Don't need to (In reply to comment #10)
> Created an attachment (id=27325) [review]

Don't need to create your own EditCommand here.  They're generally reserved for the very lowest level operations like node removal or setting an attribute, or user initiated operations like paste, delete, etc. that need their own Undo item.  I'd create CompositeEditCommand::replaceNodeWithSpan.

[Justin Garcia]

(In reply to comment #11)
> Don't need to (In reply to comment #10)
> > Created an attachment (id=27325) [review] [review]
> 
> Don't need to create your own EditCommand here.  They're generally reserved for
> the very lowest level operations like node removal or setting an attribute, or
> user initiated operations like paste, delete, etc. that need their own Undo
> item.  I'd create CompositeEditCommand::replaceNodeWithSpan.

I see what you've done...I'd put everything inside the CompositeEditCommand::replaceNodeWithSpanPreservingChildrenAndAttributes that you created.

[Eric Seidel (no email)]

(In reply to comment #12)
> (In reply to comment #11)
> I see what you've done...I'd put everything inside the
> CompositeEditCommand::replaceNodeWithSpanPreservingChildrenAndAttributes that
> you created.

I'm not currently using undo-able subcommands.  I could move to using such though... I guess I wasn't really excited about an additional malloc for every attribute move...  But maybe I've overcomplicated things?

[Justin Garcia]

Comment on attachment 27325 [details]
Improved fix and improved test case, ready for final review

+    // Similar to isSpanWithoutAttributesOrUnstyleStyleSpan, but does not look for apple-style-span

Would capitalize the 'a' in 'apple-style-span'.  Missing period.

+        // FIXME: This should probably be re-written to lookup the tagname in a
+        // hash and match against an expected property/value pair

Missing period.

I would do all the work of replacing a node with a span inside replaceNodeWithSpanPreservingChildrenAndAttributes instead of creating your own EditCommand, but r=me as is.

[Eric Seidel (no email)]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
        M       LayoutTests/ChangeLog
        A       LayoutTests/editing/execCommand/convert-style-elements-to-spans-expected.txt
        A       LayoutTests/editing/execCommand/convert-style-elements-to-spans.html
        A       LayoutTests/editing/execCommand/resources/convert-style-elements-to-spans.js
        M       WebCore/ChangeLog
        M       WebCore/WebCore.xcodeproj/project.pbxproj
        M       WebCore/css/CSSStyleDeclaration.h
        M       WebCore/dom/NamedAttrMap.h
        M       WebCore/editing/ApplyStyleCommand.cpp
        M       WebCore/editing/ApplyStyleCommand.h
        M       WebCore/editing/CompositeEditCommand.cpp
        M       WebCore/editing/CompositeEditCommand.h
        M       WebCore/editing/RemoveNodePreservingChildrenCommand.cpp
        A       WebCore/editing/ReplaceNodeWithSpanCommand.cpp
        A       WebCore/editing/ReplaceNodeWithSpanCommand.h
Committed r44435

[Brent Fulgham]

Looks like this is missing Windows project file updates.  GTK may need updates as well.

[Eric Seidel (no email)]

Makes a man want gyp. :)

[Ryosuke Niwa]

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