Bug 81656 – Backspace/delete produces combined font/span from existing inline elements

• Component: HTML Editing
• Status: NEW

[Johan "Spocke" Sörlin]

Backspace/delete produces a font or span element of the combined text styles when you delete the last character in a paragraph.

Expected results:
It should not touch any existing inline elements and only delete the character and insert a BR element to place the caret.

Actual results:
It combines all visual styles into a span or font element.

Steps to reproduce scenario 1:
1. Open the attached URL.
2. Click "Dump HTML".
3. Notice that the "1" is wrapped in a span.
4. Place the caret after the "1".
5. Press backspace.
6. Click "Dump HTML".
7. Notice that the span is now replaced with a font element.

Steps to reproduce scenario 2:
1. Open the attached URL.
2. Click "Dump HTML".
3. Notice that the "2" is wrapped in a b.
4. Click the "Set StyleWithCSS"
5. Place the caret after the "2".
6. Press backspace.
7. Click "Dump HTML".
8. Notice that the b is now replaced with a span element.

[Ryosuke Niwa]

Aryeh,

Do you know the expected behavior here?

[Aryeh Gregor]

Testing at <http://aryeh.name/tmp/editing/autoimplementation.html#delete> with input '<p><span style="font-size: large">1[]</span></p>', I find the spec says to produce:

  <p><span style="font-size: large">{}<br></span></p>

which is as requested.  Firefox 14.0a1 seems to remove the tag entirely in the second case, which is a bug.  Opera Next 12.00 alpha seems to behave per spec.  IE 10 Developer Preview deletes the entire paragraph here when you backspace, which is an egregious bug, but seems to leave the span/b tags alone.

Relevant parts of the spec are not exactly simple, but if you step through them I think you'll find that it matches the autoimplementation:

http://dvcs.w3.org/hg/editing/raw-file/tip/editing.html#the-delete-command
http://dvcs.w3.org/hg/editing/raw-file/tip/editing.html#delete-the-selection

Basically, in this case the text node is removed from its parent, then it notices that the block is now empty, so it adds a <br>.  The interesting line is

"""
If the block node of parent has no visible children, and parent is editable or an editing host, call createElement("br") on the context object and append the result as the last child of parent.
"""

"parent" here is the <span> or <b> or such, and this is run after its text node child is removed.  If you look at "View comments", this step was deliberately done before stripping wrappers like <span>/<b>, due to <https://www.w3.org/Bugs/Public/show_bug.cgi?id=13831>.  That was in turn inspired by WebKit bug 7360.


In summary, I think this bug is legitimate, and WebKit should match the spec.  Whatever code removes the empty tags here should come after the code that adds a <br> -- then by the time it checks whether the <span>/<b>/etc. is empty, it won't be, because it will have a <br> in it.

[Parag Radke]

Created attachment 143818 [details]
Praposed patch

Please give your feed back.

[Ryosuke Niwa]

Comment on attachment 143818 [details]
Praposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=143818&action=review

This patch makes things worse, not better.

> Source/WebCore/ChangeLog:12
> +        Covered by existing test, updated expected.txt.

Definitely not. r- because of the lack of the tests.

> Source/WebCore/editing/DeleteSelectionCommand.cpp:527
> +                    if (isTableStructureNode(node.get()))
> +                        removeNode(node);
> +                    else {
> +                        while (n != node) {
> +                            removeNode(n);
> +                            // see http://dvcs.w3.org/hg/editing/raw-file/tip/editing.html#delete-the-selection point 22 
> +                            if (!nParent->hasChildNodes() && isOnlyChildOfParent(nParent)) {
> +                                RefPtr<Node> placeholder = m_needPlaceholder ? createBreakElement(document()).get() : 0;
> +                                if (placeholder) {
> +                                    if (m_sanitizeMarkup)
> +                                        removeRedundantBlocks();
> +                                    nParent->appendChild(placeholder.get());
> +                                    m_needPlaceholder = false;
> +                                }
> +                                break;
> +                            }
> +                            // if nParent->isOnlyChildOfParent() is false we are going to remove nParent.
> +                            if (nParent != node) {
> +                                n = nParent->parentNode();
> +                                removeNode(nParent);
> +                                nParent = n->parentNode();
> +                                continue;
> +                            }
> +                            removeNode(node);
> +                            break;
> +                            n = nParent;
> +                            nParent = nParent->parentNode();
> +                        }
> +                        node =0;
> +                    } 

I don't think we can implement the spec partially like this. There are a lot of inter-dependencies between different sections in the spec.

> LayoutTests/editing/deleting/delete-and-cleanup-expected.txt:8
> -PASS confirmedMarkup is '<b><br></b>'
> -PASS confirmedMarkup is '<b><div style="border: solid red"><br></div></b>'
> +PASS confirmedMarkup is '<b><i><br></i></b>'
> +PASS confirmedMarkup is '<b><div style="border: solid red"><i><br></i></div></b>'

These outputs look strictly worse.

> LayoutTests/editing/deleting/delete-and-cleanup.html:41
> +testDelete("div", "<div><b><div><i>Hello</i></div></b></div>", "<b><i><br></i></b>");
> +testDelete("div", "<div><b><div style=\"border: solid red\"><i>Hello</i></div></b></div>", "<b><div style=\"border: solid red\"><i><br></i></div></b>");

Ditto.

> LayoutTests/editing/style/block-style-005-expected.txt:9
> +execDeleteCommand: <div id="test" class="editing"><font size="7">x</font></div><div id="test" class="editing"><font size="7"><br></font></div><div id="test" class="editing"><font size="7"><br></font></div>

Ditto.

> LayoutTests/platform/gtk/editing/deleting/collapse-whitespace-3587601-fix-expected.txt:56
> +        RenderInline {SPAN} at (0,0) size 0x28
> +          RenderBR {BR} at (14,14) size 0x28

Ditto.

[Ryosuke Niwa]

(In reply to comment #2)
> Testing at <http://aryeh.name/tmp/editing/autoimplementation.html#delete> with input '<p><span style="font-size: large">1[]</span></p>', I find the spec says to produce:
> 
>   <p><span style="font-size: large">{}<br></span></p>
> 
> which is as requested.  Firefox 14.0a1 seems to remove the tag entirely in the second case, which is a bug.

Firefox's behavior makes more sense. The thing is, we have to remove the span if the user moves the caret elsewhere and bring it back since that should reset the style. And it's very undesirable to leave spans like this when the user moved the caret elsewhere because the span is going to linger around in the markup and slowly bloats the HTML over time.

> In summary, I think this bug is legitimate, and WebKit should match the spec.  Whatever code removes the empty tags here should come after the code that adds a <br> -- then by the time it checks whether the <span>/<b>/etc. is empty, it won't be, because it will have a <br> in it.

While I do agree that merging span with font is bad, I don't think we should be leaving span behind. The spec needs to change here.

[Ryosuke Niwa]

(In reply to comment #0)
> Expected results:
> It should not touch any existing inline elements and only delete the character and insert a BR element to place the caret.

I don't think this is a reasonable expectation because leaving inline elements after deletion can bloat the markup over time.

> Steps to reproduce scenario 1:
> 1. Open the attached URL.
> 2. Click "Dump HTML".
> 3. Notice that the "1" is wrapped in a span.
> 4. Place the caret after the "1".
> 5. Press backspace.
> 6. Click "Dump HTML".
> 7. Notice that the span is now replaced with a font element.
> 
> Steps to reproduce scenario 2:
> 1. Open the attached URL.
> 2. Click "Dump HTML".
> 3. Notice that the "2" is wrapped in a b.
> 4. Click the "Set StyleWithCSS"
> 5. Place the caret after the "2".
> 6. Press backspace.
> 7. Click "Dump HTML".
> 8. Notice that the b is now replaced with a span element.

I think you meant to add step 5.5 which is to type "1" and "2" respectively. Otherwise, WebKit correctly removes all inline elements as expected. Frankly, I'm not convinced that we want this behavior. The author should know whether they want CSS or "presentational" elements.

[Johan "Spocke" Sörlin]

(In reply to comment #6)

Common desktop office suites like word etc keep the formatting when you delete the last character in a P tag. So when you type again it will be the right font size that the paragraph had. But sure it bloats up the HTML a bit since you might then have paragraphs like:
<p><span class="x"><b><i><br></i></b></span></p>

It think it would look strange if you backspaced in this contents:
<p><span style="font-size:large">a</span></p>
<p><span style="font-size:large">b|</span></p>
<p><span style="font-size:large">c</span></p>

And it would produce:
<p><span style="font-size:large">a</span></p>
<p>|</p>
<p><span style="font-size:large">c</span></p>

Since the line height would change when you remove the inline element. For a developer it would be ok but not for n00bs that doesn't understand that rich text editors produce HTML and that the inner element got removed.

[Ryosuke Niwa]

(In reply to comment #7)
> Common desktop office suites like word etc keep the formatting when you delete the last character in a P tag.

I don't think this is true at least for bolding.

> So when you type again it will be the right font size that the paragraph had. But sure it bloats up the HTML a bit since you might then have paragraphs like:
> <p><span class="x"><b><i><br></i></b></span></p>

Yes.

> It think it would look strange if you backspaced in this contents:
> <p><span style="font-size:large">a</span></p>
> <p><span style="font-size:large">b|</span></p>
> <p><span style="font-size:large">c</span></p>
> 
> And it would produce:
> <p><span style="font-size:large">a</span></p>
> <p>|</p>
> <p><span style="font-size:large">c</span></p>
> 
> Since the line height would change when you remove the inline element. For a developer it would be ok but not for n00bs that doesn't understand that rich text editors produce HTML and that the inner element got removed.

Yeah, maybe we need to treat font-size special.

[Aryeh Gregor]

(In reply to comment #5)
> And it's very undesirable to leave spans like this when the user moved the caret elsewhere because the span is going to linger around in the markup and slowly bloats the HTML over time.

I agree if the tags don't have any effect -- e.g., leaving just <b></b> anywhere with no contents is wrong, because it doesn't do anything.  But here, the tags serve a very important purpose: they remember the style that the paragraph had.  In word processors, AFAICT, the style of each line is remembered even if all text on the line is deleted.  Try this test:

1) Type "foo\nbar\nbaz".

2) Select all and make bold.

3) Delete the three letters "bar", so the middle line is empty.

4) Move the cursor someplace else, then move it back to the empty middle line.

5) Type "quz".

In LibreOffice 3.4.4, the word "quz" is bold.  This is as expected: the user made all three lines bold, and all three lines should stay bold even if text is deleted from them.  The tag should only be removed if the line itself is removed.

Think about it from the perspective of a user who doesn't know anything about HTML.  The user made a whole range of text bold, so from then on, anything within that range of text should be bold.  If you type "foobarbaz", delete "bar", and then type "quz" instead, "quz" is bolded.  Why should the linebreaks make any difference?

The only reason there's a difference right now in browsers is because bolding "foobarbaz" produces "<b>foobarbaz</b>", with only one <b> tag.  Bolding "<p>foo<p>bar<p>baz" produces "<p><b>foo</b><p><b>bar</b><p><b>baz</b>", with three <b> tags.  This is an implementation detail that users shouldn't be exposed to.  It should work the same as if we produced "<b><p>foo<p>bar<p>baz</p></b>" -- deleting "bar" should leave that position bold.

> While I do agree that merging span with font is bad, I don't think we should be leaving span behind. The spec needs to change here.

I disagree, for the reasons given.  The spec matches word processors (at least LibreOffice), and I think it matches user expectations.  I don't see why the user would expect it to behave any differently depending on whether "bar" is on its own line or not.  See also <https://www.w3.org/Bugs/Public/show_bug.cgi?id=13831>, where Ehsan seems to agree with me.  If you can convince him, then I'll change the spec to match what the majority of browsers want, but I think the spec is right.

(In reply to comment #6)
> I don't think this is a reasonable expectation because leaving inline elements after deletion can bloat the markup over time.

Using the least markup necessary to achieve expected results is not bloat.  If the expected result is that deleting all text on the line and then later coming back and typing on the line results in bold text, then leaving the tags is necessary.  We should try to use the minimum markup necessary, but no less.

(In reply to comment #8)
> (In reply to comment #7)
> > Common desktop office suites like word etc keep the formatting when you delete the last character in a P tag.
> 
> I don't think this is true at least for bolding.

My test-case above found that LibreOffice does preserve bold when you delete the last character on a line.  I'm almost sure that Word does too.  I'll get back to you when I have a chance to test it.

[Aryeh Gregor]

(In reply to comment #9)
> 1) Type "foo\nbar\nbaz".
> 
> 2) Select all and make bold.
> 
> 3) Delete the three letters "bar", so the middle line is empty.
> 
> 4) Move the cursor someplace else, then move it back to the empty middle line.
> 
> 5) Type "quz".
> 
> In LibreOffice 3.4.4, the word "quz" is bold.

asmodai in #whatwg confirms that Word 2010 behaves this way too (thanks, asmodai!).

[Ryosuke Niwa]

Okay, it sounds like we just need to revert http://trac.webkit.org/changeset/112444 then. I don't think we want to support preserving the original elements just yet.