Bug 173148 - Not possible to remove the 'li' element inside the table cell
Summary: Not possible to remove the 'li' element inside the table cell
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-09 03:51 PDT by Javier Fernandez
Modified: 2017-08-08 05:23 PDT (History)
10 users (show)

See Also:


Attachments
Test case to reproduce the issue (362 bytes, text/html)
2017-06-09 03:51 PDT, Javier Fernandez
no flags Details
Patch (35.38 KB, patch)
2017-06-12 14:08 PDT, Javier Fernandez
no flags Details | Formatted Diff | Diff
Patch (20.87 KB, patch)
2017-06-29 17:26 PDT, Javier Fernandez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Javier Fernandez 2017-06-09 03:51:47 PDT
Created attachment 312421 [details]
Test case to reproduce the issue

What steps will reproduce the problem?
(1) Load the attached test case
(2) Place the cursor at any of the li items
(3) hit backspace

What is the expected result?

Both list items can be deleted.

What happens instead?

None of these list items can be deleted by hitting backspace.
Comment 1 Javier Fernandez 2017-06-09 03:52:50 PDT
This looks like a clear interoperability issue, as both Firefox and IE/Edge allow deleting the list item element inside the table cel
Comment 2 Javier Fernandez 2017-06-12 14:08:56 PDT
Created attachment 312695 [details]
Patch
Comment 3 Alex Christensen 2017-06-13 13:31:44 PDT
Comment on attachment 312695 [details]
Patch

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

It would be nice if there were a web platform test for this and a link to the relevant specification.
This seems like it might be good, but I'm not the right reviewer for this code.

> LayoutTests/editing/assert_selection.js:3
> +// found in the LICENSE file.

The LICENSE file isn't included here.
Comment 4 Javier Fernandez 2017-06-14 00:44:09 PDT
(In reply to Alex Christensen from comment #3)
> Comment on attachment 312695 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=312695&action=review
> 
> It would be nice if there were a web platform test for this and a link to
> the relevant specification.

Oh, there is, indeed. I added such test as part of my patch for Blink. The problem is that WebKit doesn't import the WPT tests yet. I was tempted to do it but I thought there were lots of failures and wasn't sure it was a good idea. 


> > LayoutTests/editing/assert_selection.js:3
> > +// found in the LICENSE file.
> 
> The LICENSE file isn't included here.

Indeed, Ill add it. Thanks.
Comment 5 Darin Adler 2017-06-14 07:25:31 PDT
(In reply to Javier Fernandez from comment #4)
> (In reply to Alex Christensen from comment #3)
> > It would be nice if there were a web platform test for this and a link to
> > the relevant specification.
> 
> Oh, there is, indeed. I added such test as part of my patch for Blink. The
> problem is that WebKit doesn't import the WPT tests yet. I was tempted to do
> it but I thought there were lots of failures and wasn't sure it was a good
> idea. 

I think it’s a good idea.
Comment 6 Javier Fernandez 2017-06-19 13:55:01 PDT
@rniwa, any comment on this issue ? Thanks.
Comment 7 Ryosuke Niwa 2017-06-21 00:54:07 PDT
Comment on attachment 312695 [details]
Patch

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

Code change looks okay but the test needs to be placed elsewhere or be rewritten (preferred).

> LayoutTests/editing/assert_selection.js:1
> +// Copyright 2016 The Chromium Authors. All rights reserved.

This code is just incomprehensible.
Either we should add this under LayoutTests/imported/ or not add it and write our own test instead.

> LayoutTests/editing/deleting/delete-list-items-in-table-cell-expected.txt:6
> +
> +PASS Delete - When deleting the second ordered list items in a table cell its content is merged with the first list item. 
> +PASS Delete - When deleting the second unordered list items in a table cell its content is merged with the first list item. 
> +PASS Delete - When deleting the last ordered list item in a table cell we must delete the whole ordered list. 
> +PASS Delete - When deleting the last unrdered list item in a table cell we must delete the whole unordered list. 
> +

This output doesn't tell us what kind of markup is being tested at all.
Please use dump-as-markup.js to dump the content as the content is edited.
See other tests.
Comment 8 Ryosuke Niwa 2017-06-21 01:03:29 PDT
Also, please add a test case where the table cell is the root editable element, and other case where the entire editable content is inside a (non-editable) table cell.
Comment 9 Javier Fernandez 2017-06-29 17:26:13 PDT
Created attachment 314194 [details]
Patch
Comment 10 Javier Fernandez 2017-07-20 15:20:21 PDT
@rniwa, could you please take another look ?
Comment 11 Ryosuke Niwa 2017-07-25 21:12:49 PDT
Comment on attachment 314194 [details]
Patch

r=me. The patch looks great now.
Comment 12 WebKit Commit Bot 2017-08-08 05:22:36 PDT
Comment on attachment 314194 [details]
Patch

Clearing flags on attachment: 314194

Committed r220398: <http://trac.webkit.org/changeset/220398>
Comment 13 WebKit Commit Bot 2017-08-08 05:22:38 PDT
All reviewed patches have been landed.  Closing bug.
Comment 14 Radar WebKit Bug Importer 2017-08-08 05:23:26 PDT
<rdar://problem/33773900>