Bug 45784

Summary: Deleting line break before h1 converts h1 to span
Product: WebKit Reporter: Ryosuke Niwa <rniwa>
Component: HTML EditingAssignee: Ryosuke Niwa <rniwa>
Status: RESOLVED FIXED    
Severity: Normal CC: adele, ayg, cshu, darin, dglazkov, enrica, jparent, justin.garcia, kenneth, ojan, sullivan, tonikitoo, tony, webkit.review.bot
Priority: P1 Keywords: HasReduction
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 68649, 68857, 68865, 68874    
Bug Blocks: 68168, 68413    
Attachments:
Description Flags
Displays the incorrect merging of a h1 and p element.
none
fixes the bug
none
Fixed per comments
kenneth: review+, webkit.review.bot: commit-queue-
Second test where it produces a span with the runtime style
none
work in progress
none
fixes the bug for good
enrica: review+, webkit.review.bot: commit-queue-
Dir attribute gets inherited as style none

Description Ryosuke Niwa 2010-09-14 14:45:57 PDT
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.
Comment 1 Aryeh Gregor 2011-08-19 13:58:58 PDT
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.
Comment 2 Ojan Vafai 2011-08-20 09:30:15 PDT
What should happen if you start with "hello<h1 style='color:red'>world</h1>"?
Comment 3 Aryeh Gregor 2011-08-22 12:05:42 PDT
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.
Comment 4 Johan "Spocke" Sörlin 2011-08-30 10:35:21 PDT
Created attachment 105648 [details]
Displays the incorrect merging of a h1 and p element.
Comment 5 Ryosuke Niwa 2011-08-30 11:14:52 PDT
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.
Comment 6 Ryosuke Niwa 2011-08-30 11:18:36 PDT
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>.
Comment 7 Ryosuke Niwa 2011-09-14 17:13:33 PDT
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.
Comment 8 Ryosuke Niwa 2011-09-14 17:14:15 PDT
Created attachment 107425 [details]
fixes the bug
Comment 9 Kenneth Rohde Christiansen 2011-09-15 02:21:02 PDT
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 10 WebKit Review Bot 2011-09-15 04:46:36 PDT
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
Comment 11 Ryosuke Niwa 2011-09-15 08:25:40 PDT
(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 12 Kenneth Rohde Christiansen 2011-09-15 09:13:33 PDT
Comment on attachment 107425 [details]
fixes the bug

I thought that I did, uups :-)
Comment 13 Ryosuke Niwa 2011-09-15 09:26:02 PDT
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 14 Enrica Casucci 2011-09-15 10:01:11 PDT
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 15 Ryosuke Niwa 2011-09-15 10:25:48 PDT
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.
Comment 16 Ryosuke Niwa 2011-09-15 11:00:10 PDT
Created attachment 107511 [details]
Fixed per comments
Comment 17 Ryosuke Niwa 2011-09-15 11:08:19 PDT
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 18 Kenneth Rohde Christiansen 2011-09-15 12:24:31 PDT
Comment on attachment 107511 [details]
Fixed per comments

OK better. Sorry for not catching the error before.
Comment 19 Enrica Casucci 2011-09-15 12:41:38 PDT
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 20 WebKit Review Bot 2011-09-15 13:44:00 PDT
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
Comment 21 Ryosuke Niwa 2011-09-15 16:11:08 PDT
(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?
Comment 22 Enrica Casucci 2011-09-16 09:06:20 PDT
(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.
Comment 23 Ryosuke Niwa 2011-09-16 09:16:07 PDT
(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.
Comment 24 Ryosuke Niwa 2011-09-16 10:59:20 PDT
Would that address your concern, Enrica?
Comment 25 Enrica Casucci 2011-09-16 11:12:59 PDT
(In reply to comment #24)
> Would that address your concern, Enrica?

Yes, thank you :-)
Comment 26 Ryosuke Niwa 2011-09-16 11:22:00 PDT
(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!
Comment 27 Ryosuke Niwa 2011-09-16 16:59:05 PDT
Committed r95335: <http://trac.webkit.org/changeset/95335>
Comment 28 Ryosuke Niwa 2011-09-16 17:56:15 PDT
Added the forgotten test in http://trac.webkit.org/changeset/95337.
Comment 29 Johan "Spocke" Sörlin 2011-09-19 16:09:43 PDT
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.
Comment 30 Ryosuke Niwa 2011-09-19 16:31:56 PDT
(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.
Comment 31 Ryosuke Niwa 2011-09-22 12:24:36 PDT
Apparently, I misunderstood the issue here. I need to figure out the right fix.
Comment 32 Ryosuke Niwa 2011-09-22 17:28:02 PDT
Created attachment 108427 [details]
work in progress
Comment 33 Ryosuke Niwa 2011-09-22 17:43:44 PDT
so the new approach is to only serialize inline style decls of ancestor elements when we are not annotating for interchange.
Comment 34 Ryosuke Niwa 2011-10-05 17:47:51 PDT
Created attachment 109894 [details]
fixes the bug for good
Comment 35 WebKit Review Bot 2011-10-05 18:43:38 PDT
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 36 Enrica Casucci 2011-10-06 14:58:10 PDT
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 {
        ...
    }
}
Comment 37 Ryosuke Niwa 2011-10-06 16:15:01 PDT
Committed r96870: <http://trac.webkit.org/changeset/96870>
Comment 38 Johan "Spocke" Sörlin 2011-10-07 04:53:36 PDT
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.
Comment 39 Ryosuke Niwa 2011-10-07 17:32:50 PDT
(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.
Comment 40 Ryosuke Niwa 2013-04-02 02:54:53 PDT
This change likely caused https://bugs.webkit.org/show_bug.cgi?id=112329.