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.
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.
(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.
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.
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 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.
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
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
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
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
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
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.
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.
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
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
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
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
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 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.
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
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
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
2017-06-08 15:28 PDT, Javier Fernandez
2017-06-30 15:23 PDT, Javier Fernandez
2017-07-11 14:50 PDT, Javier Fernandez
2017-07-11 16:18 PDT, Build Bot
2017-07-11 16:22 PDT, Build Bot
2017-07-11 17:16 PDT, Build Bot
2017-07-12 00:04 PDT, Build Bot
2017-07-12 03:09 PDT, Build Bot
2018-07-04 03:31 PDT, Javier Fernandez
2018-07-04 04:36 PDT, EWS Watchlist
2018-07-04 04:42 PDT, EWS Watchlist
2018-07-04 05:11 PDT, EWS Watchlist
2018-07-04 05:20 PDT, EWS Watchlist
2018-07-04 05:21 PDT, EWS Watchlist
2018-07-04 06:51 PDT, Javier Fernandez
2018-07-09 07:07 PDT, Javier Fernandez
2018-07-09 08:56 PDT, EWS Watchlist
2018-07-09 13:44 PDT, Javier Fernandez
2018-07-09 15:13 PDT, EWS Watchlist
2018-07-12 13:24 PDT, Javier Fernandez
2018-07-12 15:05 PDT, EWS Watchlist