Bug 45784 – Deleting line break before h1 converts h1 to span

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

[Ryosuke Niwa]

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.

[Aryeh Gregor]

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.

[Ojan Vafai]

What should happen if you start with "hello<h1 style='color:red'>world</h1>"?

[Aryeh Gregor]

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.

[Johan "Spocke" Sörlin]

Created attachment 105648 [details]
Displays the incorrect merging of a h1 and p element.

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

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>.

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

Created attachment 107425 [details]
fixes the bug

[Kenneth Rohde Christiansen]

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 :-(

[WebKit Review Bot]

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

[Ryosuke Niwa]

(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+?

[Kenneth Rohde Christiansen]

Comment on attachment 107425 [details]
fixes the bug

I thought that I did, uups :-)

[Ryosuke Niwa]

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.

[Enrica Casucci]

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?

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

Created attachment 107511 [details]
Fixed per comments

[Ryosuke Niwa]

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.

[Kenneth Rohde Christiansen]

Comment on attachment 107511 [details]
Fixed per comments

OK better. Sorry for not catching the error before.

[Enrica Casucci]

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?

[WebKit Review Bot]

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

[Ryosuke Niwa]

(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?

[Enrica Casucci]

(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.

[Ryosuke Niwa]

(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.

[Ryosuke Niwa]

Would that address your concern, Enrica?

[Enrica Casucci]

(In reply to comment #24)
> Would that address your concern, Enrica?

Yes, thank you :-)

[Ryosuke Niwa]

(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!

[Ryosuke Niwa]

Committed r95335: <http://trac.webkit.org/changeset/95335>

[Ryosuke Niwa]

Added the forgotten test in http://trac.webkit.org/changeset/95337.

[Johan "Spocke" Sörlin]

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.

[Ryosuke Niwa]

(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.

[Ryosuke Niwa]

Apparently, I misunderstood the issue here. I need to figure out the right fix.

[Ryosuke Niwa]

Created attachment 108427 [details]
work in progress

[Ryosuke Niwa]

so the new approach is to only serialize inline style decls of ancestor elements when we are not annotating for interchange.

[Ryosuke Niwa]

Created attachment 109894 [details]
fixes the bug for good

[WebKit Review Bot]

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

[Enrica Casucci]

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 {
        ...
    }
}

[Ryosuke Niwa]

Committed r96870: <http://trac.webkit.org/changeset/96870>

[Johan "Spocke" Sörlin]

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.

[Ryosuke Niwa]

(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.

[Ryosuke Niwa]

This change likely caused https://bugs.webkit.org/show_bug.cgi?id=112329.