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.
I have filed https://github.com/w3c/editing/issues/163 to try to reach some consensus about the expected behavior.
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.
(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.
Created attachment 315171 [details] Patch
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.
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
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 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
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 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
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 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
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 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
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 344272 [details] Patch
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
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 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
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 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
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 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
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 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
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
Created attachment 344283 [details] Patch
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 344584 [details] Patch
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
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 344608 [details] Patch
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
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
None of the last failures detected by the bots seems to be related to my patch.
Created attachment 344876 [details] Patch
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
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 on attachment 344876 [details] Patch Clearing flags on attachment: 344876 Committed r233885: <https://trac.webkit.org/changeset/233885>
All reviewed patches have been landed. Closing bug.
<rdar://problem/42277125>