Bug 173117 – Delete content of a single cell table should not delete the whole table

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

[Javier Fernandez]

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.

[Javier Fernandez]

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.

[Javier Fernandez]

I have filed https://github.com/w3c/editing/issues/163 to try to reach some consensus about the expected behavior.

[Ryosuke Niwa]

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.

[Javier Fernandez]

(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.

[Javier Fernandez]

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.

[Javier Fernandez]

Created attachment 315171 [details]
Patch

[Build Bot]

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.

[Ryosuke Niwa]

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.

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Javier Fernandez]

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.

[Ryosuke Niwa]

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.

[Javier Fernandez]

Created attachment 344272 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Javier Fernandez]

Created attachment 344283 [details]
Patch

[Ryosuke Niwa]

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.

[Javier Fernandez]

Created attachment 344584 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[Javier Fernandez]

Created attachment 344608 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[Javier Fernandez]

None of the last failures detected by the bots seems to be related to my patch.

[Javier Fernandez]

Created attachment 344876 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[WebKit Commit Bot]

Comment on attachment 344876 [details]
Patch

Clearing flags on attachment: 344876

Committed r233885: <https://trac.webkit.org/changeset/233885>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/42277125>