Reproduction steps: 1. Place the caret before "world" in "hello<h1>world</h1>" 2. Hit delete (backspace). Expected result: hello<h1 style="display:inline">world</h1> Actual result: hello<span class="Apple-style-span" style="font-size: 32px; font-weight: bold; ">world</span> This is bad particularly when the user inserts new paragraph again between hello and world because then we'll have: hello<div><span class="Apple-style-span" style="font-size: 32px; font-weight: bold; ">world</span></div> which looks exactly like: hello<h1>world</h1> but acts completely differently.
Why isn't the expected result just "helloworld"? AFAICT, that's what all other browsers do, and also what the spec currently requires. Bug 30966 is related (same problem in the other direction). <h1 style=display:inline> seems very alarming to me.
What should happen if you start with "hello<h1 style='color:red'>world</h1>"?
Per my spec, it should become "hello<span style=color:red>world</span>" or "hello<font color=red>world</font>", depending on styleWithCSS. The approach I use is defined here: http://aryeh.name/spec/editing/editing.html#record-the-values Basically, I specify that we preserve only styles that were inline styles of some kind to start with (either style="" or font/b/u/i/etc.). So if a style was created by a CSS rule, we don't restore it if it gets lost. This matches the behavior I observe in Word 2007 when creating markup like this (set second line to Heading 1, make it red, backspace). IE10PP2, Firefox 8.0a2, and Opera 11.50 all remove the markup entirely, including coloring, which is wrong. IMO, what I specify is what matches user expectations. If something looks like a heading, it should *be* a heading, so <span style="font-size 32px; font-weight:bold"> is extremely confusing. <h1 style="display: inline"> is also confusing -- who ever heard of an inline heading? But you can't do it anyway, because if you have <p>hello</p><h1>world</h1> you *cannot* produce <p>hello<h1 style="display:inline">world</h1></p> because the text/html parser doesn't allow <h1> as a child of <p>. If a document contains that markup, it will be parsed to a DOM like <p>hello</p><h1 style="display:inline">world</h1><p></p> with the <h1> auto-closing the <p> and the </p> being interpreted like <p>. So block elements inside <p> are not an option: when you serialize and reparse you'll get a different DOM. Getting rid of the <h1> and its styles, like Word 2007, makes the most sense IMO.
Created attachment 105648 [details] Displays the incorrect merging of a h1 and p element.
Apparently, this is a top priority bug for TinyMCE because my getting rid of Apple-style-style makes it impossible for them to work around this bug by removing Apple-style-span after the merge.
Fixing this bug will change the behavior of WebKit to not maintain the editing styles when paragraphs are merged by deletion. e.g. deleting the line break between "hello" and "world" in <h1>hello</h1>world will generate <h1>helloworld</h1> instead of <h1>hello<span...>world</span></h1>.
It turned out that fixing this bug is much easier than I had initially thought. We already have a code to do a similar trick for Mail blockquote so I can just apply the same technique on h1-h6 and other elements that retain structures and appearances.
Created attachment 107425 [details] fixes the bug
Comment on attachment 107425 [details] fixes the bug View in context: https://bugs.webkit.org/attachment.cgi?id=107425&action=review Looks good to me, r=me but please fix the file names before committing. > Source/WebCore/editing/markup.cpp:474 > + if (nodeRetainsStructureAndIsNotListOrTable(commonAncestorBlock) Can we find a word that covers list and table (and maybe grid in the future?)? > LayoutTests/ChangeLog:14 > + * editing/deleting/merge-paragrpah-from-address-expected.txt: Added. > + * editing/deleting/merge-paragrpah-from-address.html: Added. > + * editing/deleting/merge-paragrpah-from-h6-expected.txt: Added. paragraph! All are spelled wrongly :-(
Comment on attachment 107425 [details] fixes the bug Attachment 107425 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9655943 New failing tests: editing/deleting/merge-whitespace-pre.html
(In reply to comment #9) > (From update of attachment 107425 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=107425&action=review > > Looks good to me, r=me but please fix the file names before committing. Thanks for the review but could you put r+?
Comment on attachment 107425 [details] fixes the bug I thought that I did, uups :-)
I filed https://bugs.webkit.org/show_bug.cgi?id=68168 to help people identify the regression in the case they want to merge this patch on top of r93001.
Comment on attachment 107425 [details] fixes the bug View in context: https://bugs.webkit.org/attachment.cgi?id=107425&action=review > Source/WebCore/editing/markup.cpp:477 > return commonAncestorBlock; I'm not sure I understand how checking nodeRetainsStructureAndIsNotListOrTable || olTag || ulTag is equivalent to the previous implementation. How about tableTag and xmpTag, don't we have to deal with those too?
Comment on attachment 107425 [details] fixes the bug View in context: https://bugs.webkit.org/attachment.cgi?id=107425&action=review >> Source/WebCore/editing/markup.cpp:474 >> + if (nodeRetainsStructureAndIsNotListOrTable(commonAncestorBlock) > > Can we find a word that covers list and table (and maybe grid in the future?)? Oops, I just realized that I picked a bad function name. It should really be something like isBlockToRetainAppearance. >> Source/WebCore/editing/markup.cpp:477 >> return commonAncestorBlock; > > I'm not sure I understand how checking nodeRetainsStructureAndIsNotListOrTable || olTag || ulTag is equivalent to the previous implementation. How about tableTag and xmpTag, don't we have to deal with those too? Oops, you're right. Will fix.
Created attachment 107511 [details] Fixed per comments
Comment on attachment 107511 [details] Fixed per comments View in context: https://bugs.webkit.org/attachment.cgi?id=107511&action=review > Source/WebCore/ChangeLog:24 > + (WebCore::nodeRetainsStructureAndIsNotListOrTable): Extracted from ancestorToRetainStructureAndAppearance. I'll update the change log before landing this patch.
Comment on attachment 107511 [details] Fixed per comments OK better. Sorry for not catching the error before.
Comment on attachment 107511 [details] Fixed per comments View in context: https://bugs.webkit.org/attachment.cgi?id=107511&action=review I don't think the patch should be landed as is. I'd like an answer to the question on isMailBlockQuote. Thanks! > Source/WebCore/editing/markup.cpp:356 > + This is still not equivalent to isMailBlockQuote in case the node tag is a block quote. There is no check on the type attribute. Why is it ok to skip it?
Comment on attachment 107511 [details] Fixed per comments Attachment 107511 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9694149 New failing tests: editing/deleting/merge-whitespace-pre.html
(In reply to comment #19) > > Source/WebCore/editing/markup.cpp:356 > > + > > This is still not equivalent to isMailBlockQuote in case the node tag is a block quote. There is no check on the type attribute. Why is it ok to skip it? Because blockquote is also one of those cases where this bug manifests. It appears that I forgot to add a test case for this so I'll add one before I land. Does this address your concern?
(In reply to comment #21) > (In reply to comment #19) > > > Source/WebCore/editing/markup.cpp:356 > > > + > > > > This is still not equivalent to isMailBlockQuote in case the node tag is a block quote. There is no check on the type attribute. Why is it ok to skip it? > > Because blockquote is also one of those cases where this bug manifests. It appears that I forgot to add a test case for this so I'll add one before I land. Does this address your concern? Sorry, I probably did not explain correctly what my concern is. The new function you're adding will return true if the node tag is block quote. isMailblockQuote returns true is the tag is block quote AND the type attribute is "cite". My question was "why is this equivalent", unless the intent is to provide the new behavior for any block quote, regardless of the value of the type attribute.
(In reply to comment #22) > Sorry, I probably did not explain correctly what my concern is. The new function you're adding will return true if the node tag is block quote. isMailblockQuote returns true is the tag is block quote AND the type attribute is "cite". Right, because we shouldn't be preserving the style of blockquote either when we have: <blockquote>hello</blockquote>world or hello<blockquote>world</blockquote> >My question was "why is this equivalent", unless the intent is to provide the new behavior for any block quote, regardless of the value of the type attribute. It is not equivalent, and I'm changing the behavior. It's just that I had forgotten to add a test for blockquote.
Would that address your concern, Enrica?
(In reply to comment #24) > Would that address your concern, Enrica? Yes, thank you :-)
(In reply to comment #25) > (In reply to comment #24) > > Would that address your concern, Enrica? > > Yes, thank you :-) Great. I'll add a test and land the patch. Thanks for catching these!
Committed r95335: <http://trac.webkit.org/changeset/95335>
Added the forgotten test in http://trac.webkit.org/changeset/95337.
Created attachment 107938 [details] Second test where it produces a span with the runtime style Added a new test file. It seems that the styles gets cloned from the runtime styles compare it with other browser IE or FF.
(In reply to comment #29) > Created an attachment (id=107938) [details] > Second test where it produces a span with the runtime style > > Added a new test file. It seems that the styles gets cloned from the runtime styles compare it with other browser IE or FF. Apparently, we have to add p to the list.
Apparently, I misunderstood the issue here. I need to figure out the right fix.
Created attachment 108427 [details] work in progress
so the new approach is to only serialize inline style decls of ancestor elements when we are not annotating for interchange.
Created attachment 109894 [details] fixes the bug for good
Comment on attachment 109894 [details] fixes the bug for good Attachment 109894 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9969096 New failing tests: editing/deleting/delete-block-merge-contents-004.html editing/deleting/delete-block-merge-contents-010.html editing/deleting/delete-block-merge-contents-016.html editing/deleting/delete-listitem-001.html editing/deleting/delete-block-merge-contents-001.html editing/deleting/delete-line-012.html editing/deleting/delete-block-merge-contents-009.html editing/deleting/delete-block-merge-contents-012.html editing/deleting/delete-block-merge-contents-014.html editing/deleting/delete-block-merge-contents-007.html editing/deleting/delete-block-merge-contents-006.html editing/deleting/delete-block-merge-contents-005.html editing/deleting/delete-block-merge-contents-013.html editing/deleting/delete-block-merge-contents-002.html editing/deleting/delete-block-merge-contents-003.html editing/deleting/delete-block-merge-contents-017.html editing/deleting/merge-whitespace-pre.html editing/deleting/delete-block-merge-contents-008.html editing/deleting/delete-block-merge-contents-015.html editing/deleting/delete-br-010.html
Comment on attachment 109894 [details] fixes the bug for good View in context: https://bugs.webkit.org/attachment.cgi?id=109894&action=review This looks great. Only one small comment. > Source/WebCore/editing/ReplaceSelectionCommand.cpp:758 > +inline Node* nodeToSplitToAvoidPastingIntoInlineNodesWithStyle(const Position& insertionPos) The function name is a bit ugly in my opinion, but very descriptive and that is what is important! > Source/WebCore/editing/markup.cpp:366 > + } I would prefer this if else block to be: if (parentOfHighestNode) { if (shouldAnnotate()) { .... } else { ... } }
Committed r96870: <http://trac.webkit.org/changeset/96870>
Created attachment 110125 [details] Dir attribute gets inherited as style Seems to be working way better now. Found one issue though not sure if this should be reported as a separate bug or not. But attaching a test file for that.
(In reply to comment #38) > Created an attachment (id=110125) [details] > Dir attribute gets inherited as style > > Seems to be working way better now. Found one issue though not sure if this should be reported as a separate bug or not. But attaching a test file for that. Hm... I think we need to ignore dir attribute for now. It's hard to copy them properly.
This change likely caused https://bugs.webkit.org/show_bug.cgi?id=112329.