Bug 23496 - execCommand('bold') will remove <b style='font-style:italic'>, it should probably replace it with <span style='font-style:italic'
Summary: execCommand('bold') will remove <b style='font-style:italic'>, it should prob...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.5
: P2 Normal
Assignee: Eric Seidel (no email)
URL:
Keywords:
: 23370 (view as bug list)
Depends on:
Blocks: 20215 22810
  Show dependency treegraph
 
Reported: 2009-01-22 23:56 PST by Eric Seidel (no email)
Modified: 2009-07-27 11:46 PDT (History)
3 users (show)

See Also:


Attachments
javascript portion of the test case in question (drop in editing/execCommands to use) (2.06 KB, application/x-javascript)
2009-01-23 00:13 PST, Eric Seidel (no email)
no flags Details
First pass, incomplete fix (9.75 KB, patch)
2009-01-23 02:16 PST, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Fix and test case (30.04 KB, patch)
2009-02-03 17:51 PST, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Improved fix and improved test case, ready for final review (34.36 KB, patch)
2009-02-04 14:29 PST, Eric Seidel (no email)
justin.garcia: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2009-01-22 23:56:26 PST
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
Comment 1 Eric Seidel (no email) 2009-01-22 23:59:59 PST
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.
Comment 2 Eric Seidel (no email) 2009-01-23 00:13:30 PST
Created attachment 26959 [details]
javascript portion of the test case in question (drop in editing/execCommands to use)
Comment 3 Eric Seidel (no email) 2009-01-23 02:15:31 PST
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.
Comment 4 Eric Seidel (no email) 2009-01-23 02:16:36 PST
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(-)
Comment 5 Eric Seidel (no email) 2009-01-23 02:18:19 PST
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.
Comment 6 Eric Seidel (no email) 2009-01-23 07:51:17 PST
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.
Comment 7 Eric Seidel (no email) 2009-02-03 17:51:57 PST
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(-)
Comment 8 Eric Seidel (no email) 2009-02-03 17:54:07 PST
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.
Comment 9 Eric Seidel (no email) 2009-02-04 09:50:21 PST
(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.
Comment 10 Eric Seidel (no email) 2009-02-04 14:29:46 PST
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(-)
Comment 11 Justin Garcia 2009-02-04 14:56:19 PST
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.
Comment 12 Justin Garcia 2009-02-04 14:58:58 PST
(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.
Comment 13 Eric Seidel (no email) 2009-02-04 15:06:23 PST
(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?
Comment 14 Justin Garcia 2009-02-11 00:34:28 PST
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.
Comment 15 Eric Seidel (no email) 2009-06-04 15:17:40 PDT
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
Comment 16 Brent Fulgham 2009-06-04 15:45:45 PDT
Looks like this is missing Windows project file updates.  GTK may need updates as well.
Comment 17 Eric Seidel (no email) 2009-06-04 16:05:12 PDT
Makes a man want gyp. :)
Comment 18 Ryosuke Niwa 2009-07-27 11:46:59 PDT
*** Bug 23370 has been marked as a duplicate of this bug. ***