Bug 57148

Summary: Delete table in contentEditable/designMode produces odd contents
Product: WebKit Reporter: Moxiecode Systems <spam>
Component: HTML EditingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, darin, dglazkov, enrica, rniwa, sullivan, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
URL: http://tinymce.moxiecode.com/safari/delete_table.htm
See Also: https://bugs.webkit.org/show_bug.cgi?id=173117
Bug Depends on: 61323, 61327, 61330, 61332, 61333, 61402    
Bug Blocks: 6627    
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ec2-cr-linux-03
none
Patch
none
Archive of layout-test-results from ec2-cr-linux-01
none
Patch
none
Patch
none
Patch
none
Patch
none
Archive of layout-test-results from ec2-cr-linux-01
none
Patch
none
Patch
none
Patch none

Description Moxiecode Systems 2011-03-26 07:22:40 PDT
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>
Comment 1 Annie Sullivan 2011-05-23 12:09:18 PDT
Created attachment 94453 [details]
Patch
Comment 2 Ryosuke Niwa 2011-05-23 12:11:20 PDT
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.
Comment 3 WebKit Review Bot 2011-05-23 12:54:01 PDT
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
Comment 4 WebKit Review Bot 2011-05-23 12:54:06 PDT
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
Comment 5 Annie Sullivan 2011-05-23 12:57:57 PDT
(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.
Comment 6 Annie Sullivan 2011-05-23 12:58:19 PDT
(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...
Comment 7 Annie Sullivan 2011-05-25 12:56:21 PDT
Created attachment 94843 [details]
Patch
Comment 8 Annie Sullivan 2011-05-25 12:58:38 PDT
(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.
Comment 9 WebKit Review Bot 2011-05-25 14:30:57 PDT
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
Comment 10 WebKit Review Bot 2011-05-25 14:31:02 PDT
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
Comment 11 Ryosuke Niwa 2011-05-25 16:57:43 PDT
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.
Comment 12 Ryosuke Niwa 2011-05-25 17:25:40 PDT
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?
Comment 13 Annie Sullivan 2011-05-26 10:53:13 PDT
Created attachment 95004 [details]
Patch
Comment 14 Ryosuke Niwa 2011-05-26 10:56:24 PDT
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.
Comment 15 Annie Sullivan 2011-05-26 11:01:55 PDT
Created attachment 95005 [details]
Patch
Comment 16 Annie Sullivan 2011-05-26 11:02:08 PDT
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.
Comment 17 Ryosuke Niwa 2011-05-26 11:06:58 PDT
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.
Comment 18 Ryosuke Niwa 2011-05-26 11:10:38 PDT
Also, you should rename OnlyInheritableProperties in enum named PropertiesToInclude declared in EditingStyle.h
Comment 19 Ryosuke Niwa 2011-05-26 11:12:15 PDT
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.
Comment 20 Darin Adler 2011-05-26 12:28:23 PDT
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.
Comment 21 Annie Sullivan 2011-05-26 13:07:23 PDT
(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.
Comment 22 Annie Sullivan 2011-05-26 13:12:49 PDT
Created attachment 95025 [details]
Patch
Comment 23 Ryosuke Niwa 2011-05-26 13:35:06 PDT
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.
Comment 24 Ryosuke Niwa 2011-05-26 13:35:35 PDT
(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.
Comment 25 Annie Sullivan 2011-05-26 14:02:59 PDT
> 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
Comment 26 Annie Sullivan 2011-05-26 14:25:23 PDT
Created attachment 95040 [details]
Patch
Comment 27 Annie Sullivan 2011-05-26 14:25:55 PDT
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.
Comment 28 Ryosuke Niwa 2011-05-26 14:30:43 PDT
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.
Comment 29 WebKit Review Bot 2011-05-26 15:01:49 PDT
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
Comment 30 WebKit Review Bot 2011-05-26 15:01:55 PDT
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
Comment 31 Annie Sullivan 2011-05-26 15:09:34 PDT
Created attachment 95053 [details]
Patch
Comment 32 Ryosuke Niwa 2011-05-26 15:19:55 PDT
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.
Comment 33 Annie Sullivan 2011-05-26 15:33:53 PDT
Created attachment 95057 [details]
Patch
Comment 34 WebKit Commit Bot 2011-05-26 21:18:37 PDT
Comment on attachment 95057 [details]
Patch

Clearing flags on attachment: 95057

Committed r87466: <http://trac.webkit.org/changeset/87466>
Comment 35 WebKit Commit Bot 2011-05-26 21:18:52 PDT
All reviewed patches have been landed.  Closing bug.
Comment 36 Annie Sullivan 2011-05-27 19:42:02 PDT
Created attachment 95248 [details]
Patch
Comment 37 Annie Sullivan 2011-05-27 19:42:50 PDT
Comment on attachment 95248 [details]
Patch

Sorry, wrong bug.