Bug 57148 – Delete table in contentEditable/designMode produces odd contents

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

[Moxiecode Systems]

When deleting a table in a contentEditable/designMode odd contents:

Steps to reproduce:
1. Open the attached URL.
2. Place caret behind the X inside editable area below.
3. Press backspace twice.
4. Click the Dump HTML link observe the HTML produced.

Expected results:
It should produce something like an empty editor area or a BR for placing the caret.

Actual results:
<span class="Apple-style-span" style="-webkit-border-horizontal-spacing: 2px; -webkit-border-vertical-spacing: 2px;"><br></span>

[Annie Sullivan]

Created attachment 94453 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 94453 [details]
Patch

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

> Source/WebCore/editing/EditingStyle.cpp:727
> +    m_mutableStyle->removeProperty(CSSPropertyWebkitBorderHorizontalSpacing);
> +    m_mutableStyle->removeProperty(CSSPropertyWebkitBorderVerticalSpacing);

It's not clear to me why we'd have to remove these two properties.  Are these two properties only ones that we care about?  It seems like there should be a set of properties we'd need to remove.

[WebKit Review Bot]

Comment on attachment 94453 [details]
Patch

Attachment 94453 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8729285

New failing tests:
editing/deleting/delete-select-all-001.html
editing/deleting/5206311-2.html
editing/deleting/5026848-2.html
editing/deleting/delete-block-table.html
editing/selection/4895428-4.html
editing/deleting/5115601.html
editing/selection/4895428-1.html
editing/deleting/5032066.html
editing/deleting/5026848-3.html

[WebKit Review Bot]

Created attachment 94466 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Annie Sullivan]

(In reply to comment #2)
> (From update of attachment 94453 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=94453&action=review
> 
> > Source/WebCore/editing/EditingStyle.cpp:727
> > +    m_mutableStyle->removeProperty(CSSPropertyWebkitBorderHorizontalSpacing);
> > +    m_mutableStyle->removeProperty(CSSPropertyWebkitBorderVerticalSpacing);
> 
> It's not clear to me why we'd have to remove these two properties.  Are these two properties only ones that we care about?  It seems like there should be a set of properties we'd need to remove.

We talked about this offline, and we're still thinking about where the best place to put code that removes properties like this is. The code review is on hold until we figure it out.

In the meantime, we wanted to check the behavior of other browsers. So I looked at Firefox 4 on Mac and IE9 on Windows 7. Neither browser had the behavior described in the initial report where if you delete the last bit of text in a table, the entire table is deleted. When I deleted the entire table manually, all styles were removed. In Firefox, the table was replace with "<br>", and in IE, it was replaced with nothing.

By contrast, WebKit keeps some styles which may be useful. For example, if you have "<table><tr><td style="font-size:xx-large">X</td></tr></table>", and you backspace to delete "X", WebKit deletes the table and adds "<font class="Apple-style-span" size="6">" around the text, so the text size is preserved. I couldn't get IE or Firefox to delete the table while preserving the font size, color, etc.

[Annie Sullivan]

(In reply to comment #3)
> (From update of attachment 94453 [details])
> Attachment 94453 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/8729285
> 
> New failing tests:
> editing/deleting/delete-select-all-001.html
> editing/deleting/5206311-2.html
> editing/deleting/5026848-2.html
> editing/deleting/delete-block-table.html
> editing/selection/4895428-4.html
> editing/deleting/5115601.html
> editing/selection/4895428-1.html
> editing/deleting/5032066.html
> editing/deleting/5026848-3.html

Taking a look at these now...

[Annie Sullivan]

Created attachment 94843 [details]
Patch

[Annie Sullivan]

(In reply to comment #7)
> Created an attachment (id=94843) [details]
> Patch

Now that the layout tests are updated, you can see that with this patch, we no longer generate a bunch of style spans with vertical/horizontal border spacing. I think these spans are unnecessary.

Please take a look at the patch and let me know if there's a better way to accomplish this.

[WebKit Review Bot]

Comment on attachment 94843 [details]
Patch

Attachment 94843 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8729984

New failing tests:
editing/pasteboard/data-transfer-items.html
fast/events/ondrop-text-html.html
editing/pasteboard/onpaste-text-html.html

[WebKit Review Bot]

Created attachment 94861 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Ryosuke Niwa]

Comment on attachment 94843 [details]
Patch

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

> LayoutTests/ChangeLog:8
> +        Update test expectations and add new test for the case in the bug.

You should describe what kind of test you're adding and what kind of rebaselines are done.

> LayoutTests/editing/deleting/delete-last-char-in-table-expected.txt:1
> +See this bug: https://bugs.webkit.org/show_bug.cgi?id=57148 "Delete table in contentEditable/designMode produces odd contents".

This is a little verbose.  I'd prefer to have just the bug number since the description below makes the context very clear.

> LayoutTests/editing/deleting/delete-last-char-in-table.html:11
> +<style>
> +.editing { 
> +    border: 2px solid red; 
> +    padding: 12px; 
> +    font-size: 24px; 
> +}
> +</style>

You don't need this style, do you?  We used to have this style when we didn't have dump-as-markup but we don't need to add them anymore.

> LayoutTests/editing/deleting/delete-last-char-in-table.html:21
> +<script src=../editing.js language="JavaScript" type="text/JavaScript" ></script>

Please get rid of language and type attributes and also double-quote the url.

> Source/WebCore/ChangeLog:8
> +        Removes -webkit-border-horizontal-spacing and -webkit-border-vertical-spacing from the list of properties which should be preserved during edit operations.

This line is too long.  Please split into two lines.

[Ryosuke Niwa]

Enrica & Darin,

What do you think of the idea to remove -webkit-border-horizontal-spacing and -webkit-border-vertical-spacing from the list of inheritable properties in EditingStyle.cpp?

[Annie Sullivan]

Created attachment 95004 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 95004 [details]
Patch

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

> LayoutTests/ChangeLog:10
> +        should not leave behind a style span.
> +        Rebaseline tests now that -webkit-border-horizontal-spacing and -webkit-border-vertical-spacing

Please put a blank line between paragraphs if you're starting a new paragraph in new line.

[Annie Sullivan]

Created attachment 95005 [details]
Patch

[Annie Sullivan]

Comment on attachment 95004 [details]
Patch

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

>> LayoutTests/ChangeLog:10
>> +        Rebaseline tests now that -webkit-border-horizontal-spacing and -webkit-border-vertical-spacing
> 
> Please put a blank line between paragraphs if you're starting a new paragraph in new line.

Done.

[Ryosuke Niwa]

Comment on attachment 95004 [details]
Patch

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

> Source/WebCore/editing/EditingStyle.cpp:-71
> -    CSSPropertyWebkitBorderHorizontalSpacing,
> -    CSSPropertyWebkitBorderVerticalSpacing,

You should update the comments above the list since this list no longer contains all inheritable properties.

[Ryosuke Niwa]

Also, you should rename OnlyInheritableProperties in enum named PropertiesToInclude declared in EditingStyle.h

[Ryosuke Niwa]

I'm not sure what a good alternative name is.  Maybe text properties?

Also, maybe we should remove border-collapse property as well since there's virtually no use of that property in editing.

[Darin Adler]

I think it’s fine to adjust this list of properties. If it has to do with editing, we may want to move that list somewhere else.

I don’t understand the ramification of removing these specific properties. I think that generally if you are using a particular editing user interface, the properties you don’t want to copy are the ones that have no UI to adjust them. Hence, any code inside WebKit has to make a lot of assumptions about UI. That’s not great, and worth thinking over.

Google Docs might have one set of properties it can adjust in Chrome. Some other properties might be adjustable only in Safari because it has different menu items. And then uses outside the web such as Mail might have yet another set of properties. I’m concerned about trying to use a single list for all of these.

The old concept, inheritable properties, was something that was more easily specified, but I see now that the decision of which properties to deal with is more context-dependent than I had thought.

[Annie Sullivan]

(In reply to comment #20)
> I think it’s fine to adjust this list of properties. If it has to do with editing, we may want to move that list somewhere else.
> 
> I don’t understand the ramification of removing these specific properties. I think that generally if you are using a particular editing user interface, the properties you don’t want to copy are the ones that have no UI to adjust them. Hence, any code inside WebKit has to make a lot of assumptions about UI. That’s not great, and worth thinking over.

In this case, we're copying the table border properties when we delete a table cell. As you can see in the layout test results, there are two cases here:

1. The table border properties get left over in a styled span when the entire table is deleted. This is the case in the original bug report, and it doesn't make any sense to have table border CSS when there is no longer any table.

2. The table border properties get copied into a styled span around the contents of a deleted cell when it gets merged into another cell. The cell that the contents were merged into has its own border properties, and the user most likely intended that those border properties be used.

> 
> Google Docs might have one set of properties it can adjust in Chrome. Some other properties might be adjustable only in Safari because it has different menu items. And then uses outside the web such as Mail might have yet another set of properties. I’m concerned about trying to use a single list for all of these.

I agree this is really painful. I used to work on Google Docs, and it was really frustrating because there was no way to determine what styles would be included, or how they would be included. Writing a good rich text editor app involves an enormous amount of cleanup around things like this. But I don't think there's a good way to solve this problem in the general case without getting some agreement around a specification--e.g. how to specify the list across browsers. A spec is being worked on, but I'm sure it will take quite some time to get it down to this level of detail. In the meantime, copying the table borders is adding a lot of cruft and this affects TinyMCE, which is a very popular rich text editor used by a lot of sites.

[Annie Sullivan]

Created attachment 95025 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 95025 [details]
Patch

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

> LayoutTests/editing/deleting/delete-last-char-in-table-expected.txt:6
> +When deleteing the last character in a table deletes the table, no styled spans should be left behind. To test manually, place cursor after "X" and do a backward delete. No styled span should be created.
> +
> +
> +execDeleteCommand: <br>

Maybe it's clear when this test passes from this output.  Can you add a some check?  e.g. by checking document.getElementsByTagName('span').length == 0.

> LayoutTests/editing/deleting/delete-last-char-in-table.html:5
> +<p>See bug 57148.</p>
> +<p>When deleteing the last character in a table deletes the table, no styled spans should be left behind. To test manually, place cursor after "X" and do a backward delete. No styled span should be created.</p>

I would combine these two paragraphs.

> Source/WebCore/editing/EditingStyle.cpp:-54
> -    CSSPropertyBorderCollapse,

We need a test for this.  r- due to the lack of test for border-collapse.

> Source/WebCore/editing/EditingStyle.h:62
> +    enum PropertiesToInclude { AllProperties, OnlyEditingInheritableProperties, InheritablePropertiesAndBackgroundColorInEffect };

You should also rename InheritablePropertiesAndBackgroundColorInEffect.

[Ryosuke Niwa]

(In reply to comment #23)
> Maybe it's clear when this test passes from this output.  Can you add a some check?  e.g. by checking document.getElementsByTagName('span').length == 0.

Meant to say *not* clear.

[Annie Sullivan]

> We need a test for this.  r- due to the lack of test for border-collapse.

I'm updating my test to include this now. But just so people can see how it normally gets included, you can go to this url, select X and Y and hit backspace. It includes border-collapse in the span: http://simple-rte.rniwa.com/?editor=%3Ctable%20style%3D%22border-collapse%3Acollapse%22%3E%3Ctbody%3E%3Ctr%3E%3Ctd%3EX%3C/td%3E%3Ctd%3EY%3C/td%3E%3C/tr%3E%3C/tbody%3E%3C/table%3E&script=editor%28%29.focus%28%29%3B

[Annie Sullivan]

Created attachment 95040 [details]
Patch

[Annie Sullivan]

Comment on attachment 95025 [details]
Patch

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

>> LayoutTests/editing/deleting/delete-last-char-in-table-expected.txt:6
>> +execDeleteCommand: <br>
> 
> Maybe it's clear when this test passes from this output.  Can you add a some check?  e.g. by checking document.getElementsByTagName('span').length == 0.

I did this by adding a result element. I see this done in a couple different ways in existing tests (e.g. some use document.write()), so please let me know if there is a preferred way to do this.

>> LayoutTests/editing/deleting/delete-last-char-in-table.html:5
>> +<p>When deleteing the last character in a table deletes the table, no styled spans should be left behind. To test manually, place cursor after "X" and do a backward delete. No styled span should be created.</p>
> 
> I would combine these two paragraphs.

Done. I added a line break because the text got really long.

>> Source/WebCore/editing/EditingStyle.cpp:-54
>> -    CSSPropertyBorderCollapse,
> 
> We need a test for this.  r- due to the lack of test for border-collapse.

I added a border-collapse style to the delete-last-char-in-table test. It fails on TOT and passes after my change.

[Ryosuke Niwa]

Comment on attachment 95040 [details]
Patch

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

r=me. Looking at new expected results, I say that this is a progression.

> LayoutTests/ChangeLog:13
> +        Add a test for the case given in the bug: deleting an unstyled table
> +        should not leave behind a style span.
> +
> +        Rebaseline tests now that -webkit-border-horizontal-spacing and -webkit-border-vertical-spacing
> +        style spans are no longer added on table and table cell deletion, and no longer applied when
> +        copying to the clipboard.

It'll be nice to align columns in the future.

> LayoutTests/editing/deleting/delete-last-char-in-table-expected.txt:4
> +PASS

This is great!

> LayoutTests/editing/deleting/delete-last-char-in-table.html:21
> +    var styledSpans = document.getElementsByTagName("span");
> +    result.innerHTML = styledSpans.length == 0 ? "PASS" : "FAIL";

Yup, this works.

[WebKit Review Bot]

Comment on attachment 95040 [details]
Patch

Attachment 95040 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8739294

New failing tests:
editing/pasteboard/data-transfer-items.html
fast/events/ondrop-text-html.html
editing/pasteboard/onpaste-text-html.html

[WebKit Review Bot]

Created attachment 95049 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Annie Sullivan]

Created attachment 95053 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 95053 [details]
Patch

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

> LayoutTests/ChangeLog:13
> +        Add a test for the case given in the bug: deleting an unstyled table
> +        should not leave behind a style span.
> +
> +        Rebaseline tests now that -webkit-border-horizontal-spacing and -webkit-border-vertical-spacing
> +        style spans are no longer added on table and table cell deletion, and no longer applied when
> +        copying to the clipboard.

Since you'd have to update change log, please align columns.

> LayoutTests/ChangeLog:25
> +        * editing/selection/4895428-4-expected.txt:

editing/pasteboard/data-transfer-items-expected.txt
fast/events/ondrop-text-html-expected.txt
editing/pasteboard/onpaste-text-html-expected.txt
should be listed here and lexicologically sorted.

[Annie Sullivan]

Created attachment 95057 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 95057 [details]
Patch

Clearing flags on attachment: 95057

Committed r87466: <http://trac.webkit.org/changeset/87466>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Annie Sullivan]

Created attachment 95248 [details]
Patch

[Annie Sullivan]

Comment on attachment 95248 [details]
Patch

Sorry, wrong bug.