Bug 173117 - Delete content of a single cell table should not delete the whole table
Summary: Delete content of a single cell table should not delete the whole table
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Javier Fernandez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-06-08 15:28 PDT by Javier Fernandez
Modified: 2018-07-17 01:51 PDT (History)
10 users (show)

See Also:


Attachments
Test case to reproduce the issue (202 bytes, text/html)
2017-06-08 15:28 PDT, Javier Fernandez
no flags Details
New test case with just 1 cell (193 bytes, text/html)
2017-06-30 15:23 PDT, Javier Fernandez
no flags Details
Patch (2.73 KB, patch)
2017-07-11 14:50 PDT, Javier Fernandez
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews107 for mac-elcapitan-wk2 (1.25 MB, application/zip)
2017-07-11 16:18 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews102 for mac-elcapitan (1023.01 KB, application/zip)
2017-07-11 16:22 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews123 for ios-simulator-wk2 (983.62 KB, application/zip)
2017-07-11 17:16 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews117 for mac-elcapitan (1.96 MB, application/zip)
2017-07-12 00:04 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews112 for mac-elcapitan (1.98 MB, application/zip)
2017-07-12 03:09 PDT, Build Bot
no flags Details
Patch (11.78 KB, patch)
2018-07-04 03:31 PDT, Javier Fernandez
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-sierra (2.30 MB, application/zip)
2018-07-04 04:36 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews107 for mac-sierra-wk2 (2.80 MB, application/zip)
2018-07-04 04:42 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews201 for win-future (12.81 MB, application/zip)
2018-07-04 05:11 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews116 for mac-sierra (3.02 MB, application/zip)
2018-07-04 05:20 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews126 for ios-simulator-wk2 (2.24 MB, application/zip)
2018-07-04 05:21 PDT, Build Bot
no flags Details
Patch (13.35 KB, patch)
2018-07-04 06:51 PDT, Javier Fernandez
no flags Details | Formatted Diff | Diff
Patch (12.40 KB, patch)
2018-07-09 07:07 PDT, Javier Fernandez
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews124 for ios-simulator-wk2 (2.24 MB, application/zip)
2018-07-09 08:56 PDT, Build Bot
no flags Details
Patch (12.40 KB, patch)
2018-07-09 13:44 PDT, Javier Fernandez
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-sierra (2.31 MB, application/zip)
2018-07-09 15:13 PDT, Build Bot
no flags Details
Patch (12.40 KB, patch)
2018-07-12 13:24 PDT, Javier Fernandez
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews200 for win-future (12.78 MB, application/zip)
2018-07-12 15:05 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Javier Fernandez 2017-06-08 15:28:53 PDT
Created attachment 312353 [details]
Test case to reproduce the issue

Hitting delete on the unique cell of the table in the attached test case will cause the whole table to be deleted. 
This behavior doesn't match Firefox or IE.
Comment 1 Javier Fernandez 2017-06-09 01:56:55 PDT
I think this is clearly an interoperability issue, since Firefox and IE/Edge don't delete the whole table even if the content of the single cell table is deleted. It's also rather inconsistent that we claim we should do it i bug #24238 with a table with more than one cell. 

I've got a patch to solve this bug so it matches what other browser do, but first we should clarify what's the expected behavior we want. Unfortunately, I couldn't find anything in the spec (either Editing or Table) to help me figure it out.
Comment 2 Javier Fernandez 2017-06-09 02:48:25 PDT
I have filed https://github.com/w3c/editing/issues/163 to try to reach some consensus about the expected behavior.
Comment 3 Ryosuke Niwa 2017-06-09 15:25:11 PDT
Oh, just selecting & deleting the first cell ends up deleting the entire table. This is clearly a bug. There's no way this is the right UX.
Comment 4 Javier Fernandez 2017-06-30 06:58:13 PDT
(In reply to Ryosuke Niwa from comment #3)
> Oh, just selecting & deleting the first cell ends up deleting the entire
> table. This is clearly a bug. There's no way this is the right UX.

However, this bug seems to contradict what bug #24238 claim needs to be fixed. Could you please clarify what should be the expected behavior ? It seems that both Firefox and IE decide to do nothing and keep the table structure, even when the row, and the whole table actually, is empty.
Comment 5 Javier Fernandez 2017-06-30 15:23:01 PDT
Created attachment 314301 [details]
New test case with just 1 cell

I'm not able to reproduce the bug with the original test case. Only when there is one cell, removing the last character lead to delete the whole table.
Comment 6 Javier Fernandez 2017-07-11 14:50:20 PDT
Created attachment 315171 [details]
Patch
Comment 7 Build Bot 2017-07-11 15:34:28 PDT
Attachment 315171 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:20:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 8 Ryosuke Niwa 2017-07-11 15:43:18 PDT
Comment on attachment 315171 [details]
Patch

Do you know when this code was added? Follow svn/git blame should reveal which revision added the code you're removing. We need to understand why that code was there.
Comment 9 Build Bot 2017-07-11 16:18:38 PDT
Comment on attachment 315171 [details]
Patch

Attachment 315171 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/4103356

New failing tests:
editing/unsupported-content/table-delete-003.html
editing/deleting/delete-last-char-in-table.html
editing/unsupported-content/table-delete-001.html
Comment 10 Build Bot 2017-07-11 16:18:39 PDT
Created attachment 315184 [details]
Archive of layout-test-results from ews107 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 11 Build Bot 2017-07-11 16:22:02 PDT
Comment on attachment 315171 [details]
Patch

Attachment 315171 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/4103396

New failing tests:
editing/unsupported-content/table-delete-003.html
editing/deleting/delete-last-char-in-table.html
editing/unsupported-content/table-delete-001.html
Comment 12 Build Bot 2017-07-11 16:22:03 PDT
Created attachment 315187 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 13 Build Bot 2017-07-11 17:16:20 PDT
Comment on attachment 315171 [details]
Patch

Attachment 315171 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4103604

New failing tests:
editing/unsupported-content/table-delete-003.html
editing/deleting/delete-last-char-in-table.html
editing/unsupported-content/table-delete-001.html
Comment 14 Build Bot 2017-07-11 17:16:21 PDT
Created attachment 315194 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5
Comment 15 Build Bot 2017-07-12 00:04:00 PDT
Comment on attachment 315171 [details]
Patch

Attachment 315171 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4105031

New failing tests:
editing/unsupported-content/table-delete-003.html
editing/deleting/delete-last-char-in-table.html
editing/unsupported-content/table-delete-001.html
Comment 16 Build Bot 2017-07-12 00:04:01 PDT
Created attachment 315211 [details]
Archive of layout-test-results from ews117 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 17 Build Bot 2017-07-12 03:09:13 PDT
Comment on attachment 315171 [details]
Patch

Attachment 315171 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4105703

New failing tests:
editing/unsupported-content/table-delete-003.html
editing/deleting/delete-last-char-in-table.html
editing/unsupported-content/table-delete-001.html
Comment 18 Build Bot 2017-07-12 03:09:14 PDT
Created attachment 315223 [details]
Archive of layout-test-results from ews112 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 19 Javier Fernandez 2017-07-21 03:25:48 PDT
It seems the patch causes some layout tests failures. All of them expect the table to be removed, so it's understandable why the patch causes such failures. We could think on rebaseline them, but I don't think it's a good idea.

The patch avoids a table to be removed even when the content of the last cell is removed and becomes an empty table. This behavior could be sensible for the test case used to file this bug: a table with a single cell. However, the cases verified by the tests table-delete-001.html and table-delete-003.html expect that empty table rows are removed as well. 

If we apply only the proposed patch we will end with a table without any tr element, which we could not select or edit anymore. If we want to avoid the table to be removed we might avoid the empty tr elements to be removed as well. This is what Firefox and IE do, however, I think current WebKit behavior makes sense and perhaps it's more useful in other uses cases.

Hence, if we don't want to avoid empty rows to be deleted, we must forget about this patch and close the bug as WONTFIX. Additionally, I think we should close the bug #24238 as well for the same reason.
Comment 20 Ryosuke Niwa 2018-06-14 10:18:19 PDT
table-delete-001.html and table-delete-003.html are both concerned about cases where the selection extends before/after the table. In those cases, deleting the entire table is the right behavior. If only the contents of a table cell is removed, then keeping the table makes sense. Indeed, this seems to match the behavior of TextEdit on macOS so that's probably what we want.
Comment 21 Javier Fernandez 2018-07-04 03:31:09 PDT
Created attachment 344272 [details]
Patch
Comment 22 Build Bot 2018-07-04 04:36:40 PDT
Comment on attachment 344272 [details]
Patch

Attachment 344272 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/8435210

New failing tests:
editing/unsupported-content/table-delete-005.html
editing/deleting/deleting-relative-positioned-special-element.html
Comment 23 Build Bot 2018-07-04 04:36:42 PDT
Created attachment 344274 [details]
Archive of layout-test-results from ews100 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 24 Build Bot 2018-07-04 04:42:39 PDT
Comment on attachment 344272 [details]
Patch

Attachment 344272 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/8435217

New failing tests:
editing/unsupported-content/table-delete-005.html
editing/deleting/deleting-relative-positioned-special-element.html
Comment 25 Build Bot 2018-07-04 04:42:41 PDT
Created attachment 344276 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6
Comment 26 Build Bot 2018-07-04 05:10:55 PDT
Comment on attachment 344272 [details]
Patch

Attachment 344272 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8435383

New failing tests:
editing/unsupported-content/table-delete-005.html
editing/deleting/deleting-relative-positioned-special-element.html
Comment 27 Build Bot 2018-07-04 05:11:06 PDT
Created attachment 344278 [details]
Archive of layout-test-results from ews201 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews201  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment 28 Build Bot 2018-07-04 05:20:38 PDT
Comment on attachment 344272 [details]
Patch

Attachment 344272 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/8435345

New failing tests:
editing/unsupported-content/table-delete-005.html
editing/deleting/deleting-relative-positioned-special-element.html
Comment 29 Build Bot 2018-07-04 05:20:40 PDT
Created attachment 344279 [details]
Archive of layout-test-results from ews116 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 30 Build Bot 2018-07-04 05:21:13 PDT
Comment on attachment 344272 [details]
Patch

Attachment 344272 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/8435346

New failing tests:
editing/unsupported-content/table-delete-005.html
editing/deleting/deleting-relative-positioned-special-element.html
Comment 31 Build Bot 2018-07-04 05:21:15 PDT
Created attachment 344280 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4
Comment 32 Javier Fernandez 2018-07-04 06:51:51 PDT
Created attachment 344283 [details]
Patch
Comment 33 Ryosuke Niwa 2018-07-06 16:28:21 PDT
Comment on attachment 344283 [details]
Patch

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

> Source/WebCore/editing/TypingCommand.cpp:764
> -    CompositeEditCommand::deleteSelection(selectionToDelete, m_smartDelete);
> +    CompositeEditCommand::deleteSelection(selectionToDelete, m_smartDelete, true, false, expandForSpecialElements, true);

Please add an inline comment for each true & false to indicate which boolean is set to true & false.

> Source/WebCore/editing/TypingCommand.cpp:867
> -    CompositeEditCommand::deleteSelection(selectionToDelete, m_smartDelete);
> +    CompositeEditCommand::deleteSelection(selectionToDelete, m_smartDelete, true, false, expandForSpecialElements, true);

Ditto.

> LayoutTests/editing/unsupported-content/table-delete-004.html:1
> +<html> 

Missing DOCTYPE. Please don't name these functions as table-delete-00x.
Give them more descriptive name.

> LayoutTests/editing/unsupported-content/table-delete-004.html:5
> +.editing { 

We don't do these styles anymore.

> LayoutTests/editing/unsupported-content/table-delete-004.html:11
> +<script src=../editing.js language="JavaScript" type="text/JavaScript" ></script>

No need to specify language or type.

> LayoutTests/editing/unsupported-content/table-delete-004.html:18
> +<div contenteditable id="root" style="word-wrap: break-word; -khtml-nbsp-mode: space; -khtml-line-break: after-white-space;">

Why are we using all these inline styles? I don't think we need them at all.

> LayoutTests/editing/unsupported-content/table-delete-004.html:30
> +<script>

We don't really indent script content anymore.

> LayoutTests/editing/unsupported-content/table-delete-004.html:31
> +    Markup.description('For this test delete the last character of a single-cell table. Expected Results: Only the charactet is deleted.The table structure should not be affected.')

Nits: instead of prefixing the sentence with "Expected Results:", just say "only the table cell should be deleted, and not the table itself".
There's a lot of types and missing space, etc... here.

> LayoutTests/editing/unsupported-content/table-delete-004.html:33
> +    var element = document.getElementById("test");

Use const.

> LayoutTests/editing/unsupported-content/table-delete-005.html:31
> +    Markup.description('For this test delete the last character of a single-cell table. Expected Results: Only the charactet is deleted.The table structure should not be affected.')

Forward delete?

> LayoutTests/editing/unsupported-content/table-delete-006.html:31
> +    Markup.description('For this test select and delete the last character of a single-cell table. Expected Results: The whole table structure is deleted.')

This is not a grammatically sound sentence.
This test selects & deletes the last characters of a single-cell table.
We're not asking the viewer to do that to test.
Comment 34 Javier Fernandez 2018-07-09 07:07:09 PDT
Created attachment 344584 [details]
Patch
Comment 35 Build Bot 2018-07-09 08:56:54 PDT
Comment on attachment 344584 [details]
Patch

Attachment 344584 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/8482672

New failing tests:
animations/needs-layout.html
Comment 36 Build Bot 2018-07-09 08:56:56 PDT
Created attachment 344589 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4
Comment 37 Javier Fernandez 2018-07-09 13:44:15 PDT
Created attachment 344608 [details]
Patch
Comment 38 Build Bot 2018-07-09 15:13:27 PDT
Comment on attachment 344608 [details]
Patch

Attachment 344608 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/8485999

New failing tests:
media/media-fullscreen-return-to-inline.html
Comment 39 Build Bot 2018-07-09 15:13:29 PDT
Created attachment 344624 [details]
Archive of layout-test-results from ews100 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 40 Javier Fernandez 2018-07-09 15:35:24 PDT
None of the last failures detected by the bots seems to be related to my patch.
Comment 41 Javier Fernandez 2018-07-12 13:24:03 PDT
Created attachment 344876 [details]
Patch
Comment 42 Build Bot 2018-07-12 15:05:46 PDT
Comment on attachment 344876 [details]
Patch

Attachment 344876 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8519424

New failing tests:
http/tests/security/video-poster-cross-origin-crash2.html
Comment 43 Build Bot 2018-07-12 15:05:58 PDT
Created attachment 344886 [details]
Archive of layout-test-results from ews200 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews200  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment 44 WebKit Commit Bot 2018-07-17 01:48:56 PDT
Comment on attachment 344876 [details]
Patch

Clearing flags on attachment: 344876

Committed r233885: <https://trac.webkit.org/changeset/233885>
Comment 45 WebKit Commit Bot 2018-07-17 01:48:58 PDT
All reviewed patches have been landed.  Closing bug.
Comment 46 Radar WebKit Bug Importer 2018-07-17 01:51:19 PDT
<rdar://problem/42277125>