Bug 122602 – contentEditable deleting lists when list items are block level

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

[Colin Devroe]

Created attachment 213888 [details]
An HTML file to reproduce bug.

When a list item within a contentEditable area is floated and/or displayed as a block level element contentEditable will remove the entire list when the Enter key is hit twice.

Steps to reproduce: 
1. Add a contentEditable area
2. Add a list (UL or OL)
3. Style the LIs to be display block or floated left (both result in same issue)
4. Load HTML in browser
5. Put cursor in the last list item of the list
6. Hit enter twice

Expected behavior: The list is discontinued and a paragraph tag or DIV is created leaving the UL intact.

Result: The entire UL disappears from DOM and a DIV with a BR appears.

Attached is an HTML file that can reproduce the issue.

[Ryosuke Niwa]

Unfortunately, we don't do a good job of supporting author styled being applied to elements inside content editable region. While I do agree this is a bug, I don't think we can prioritize bugs like this given the time & the resource constraints we have.

[Colin Devroe]

Ryosuke: Thanks for the response. We found a workaround... by forcing a doctype and manually setting display: list-item the problem goes away, even on floated elements.

I wonder if there is any argument to be made that this is not a bug. Because list items probably shouldn't ever be set as display block nowadays? Is that true?

[Santosh Mahto]

Created attachment 215251 [details]
under construction

[Santosh Mahto]

Created attachment 215255 [details]
Patch

[Santosh Mahto]

Description:
Currently when list item is empty then on hitting Enter it should be deleted.
which is done here  CompositeEditCommand::breakOutOfEmptyListItem()

This function check for isListItem() to decide about deletion.
So if listItem is last item then this line casues actual deletion:
 removeNode(isListItem(previousListNode.get()) || isListElement(previousListNode.get()) ? emptyListItem.get() : listNode.get());


But here isListItem() doesnot decide properly about listItems.

Actually this is how renderobject are created.

case1:              LI : no display  -------------> RenderListItem.
case2:              LI : dislay: block -------------> RenderBlock
case3:              LI :display:LIST_ITEM -------> RenderListItem.

isListItem return true in case 1 and 2 becasue it check of type of renderobject which is listItem , perfect
But in case2 since renderObject is block so isListItem return false. 

But all are listItem.
So I modified the code to check tag name  then renderObject to confirm it is listitem.

Please reply if anywhere i confused or any suggestion

[Darin Adler]

Comment on attachment 215255 [details]
Patch

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

> Source/WebCore/editing/htmlediting.cpp:546
>  bool isListItem(const Node *n)
>  {
> -    return n && n->renderer() && n->renderer()->isListItem();
> +    return n && (n->hasTagName(liTag) || (n->renderer() && n->renderer()->isListItem()));
>  }

Looks like this would make it impossible to delete a list that was built with an element other than <li> and CSS style.

[Santosh Mahto]

(In reply to comment #6)
> (From update of attachment 215255 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=215255&action=review
> 
> > Source/WebCore/editing/htmlediting.cpp:546
> >  bool isListItem(const Node *n)
> >  {
> > -    return n && n->renderer() && n->renderer()->isListItem();
> > +    return n && (n->hasTagName(liTag) || (n->renderer() && n->renderer()->isListItem()));
> >  }
> 
> Looks like this would make it impossible to delete a list that was built with an element other than <li> and CSS style.

I guess you are talking related to ["Generally when listitem is empty then we
 delete the listitem on hitting "Enter"] 

Yes, empty list will be deleted if either it is <li> or renderer is RenderListItem  else not.

But this behavior has been here before, before when renderer is RenderListItem then it is deleted,  only case.
May I know what other way list can be created. Proabaly we can expand more this function def.

Thanks..

[Ryosuke Niwa]

Any element inside ul/ol should be considered as a list item.  At least that's what WebKit currently does.

[Darin Adler]

I was referring to a list that was built out of some other element, such as a custom tag:

    <style>item { display: list-item }</style>
    <ol><item>first</item><item>second</item>

Does the old code work for lists like that?

[Ryosuke Niwa]

(In reply to comment #9)
> I was referring to a list that was built out of some other element, such as a custom tag:
> 
>     <style>item { display: list-item }</style>
>     <ol><item>first</item><item>second</item>
> 
> Does the old code work for lists like that?

We do support that.

[Santosh Mahto]

(In reply to comment #9)
> I was referring to a list that was built out of some other element, such as a custom tag:
> 
>     <style>item { display: list-item }</style>
>     <ol><item>first</item><item>second</item>
> 
> Does the old code work for lists like that?

Yes, old code will work. because second  test(renderer) will pass.
     return n && (n->hasTagName(liTag) ||
  PASS-->                     (n->renderer() &&    n->renderer()->isListItem()));

But Without my  patch. This will fail.
     <style>item { display: block}</style>
     <ol><li>first</li><li>second</li>

[Santosh Mahto]

It looks to me adding test for Darin points will be good.

So I am adding a test which will display the behavior when listitem is made of 
<item> and display:list-item.

Although this new test will not be close to this bug as this bug is for display:block/float on listitem(li) not for display:list-item.
But its better to add to show behavior for this kind of list.

If there is any objection, please comment.

[Santosh Mahto]

Created attachment 215318 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 215318 [details]
Patch

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

> Source/WebCore/editing/htmlediting.cpp:545
> -    return n && n->renderer() && n->renderer()->isListItem();
> +    return n && (n->hasTagName(liTag) || (n->renderer() && n->renderer()->isListItem()));

A better check than n->hasTagName will be isListElement(n->parentNode()).

[Santosh Mahto]

Created attachment 215404 [details]
Patch

[Santosh Mahto]

(In reply to comment #14)
> (From update of attachment 215318 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=215318&action=review
> 
> > Source/WebCore/editing/htmlediting.cpp:545
> > -    return n && n->renderer() && n->renderer()->isListItem();
> > +    return n && (n->hasTagName(liTag) || (n->renderer() && n->renderer()->isListItem()));
> 
> A better check than n->hasTagName will be isListElement(n->parentNode()).

updated this in latest patch.thanks

[Ryosuke Niwa]

Comment on attachment 215404 [details]
Patch

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

> LayoutTests/ChangeLog:13
> +        * editing/execCommand/hit-enter-twice-atendof-block-styled-listitem-expected.txt: Added.
> +        * editing/execCommand/hit-enter-twice-atendof-block-styled-listitem.html: Added.

I would have named this as inserting-paragraphs-twice instead.
Also why is atendof one word?

> LayoutTests/editing/execCommand/hit-enter-twice-atendof-block-styled-listitem-expected.txt:2
> +This test verify When hitting "Enter" twice at the end of last listitem styled with display:block/float does not cause complete list to be removed.
> +Expected behavior is list should not be removed and one extra empty line should get added.

Please fix the capitalization "When". Also I would phrase it as inserting paragraph as opposed to "hitting 'Enter'"
since this bug reproduces whenever a paragraph is inserted.

> LayoutTests/editing/execCommand/hit-enter-twice-atendof-block-styled-listitem-expected.txt:30
> +After hitting First Enter:

Why is "First Enter" capitalized?

> LayoutTests/editing/execCommand/hit-enter-twice-atendof-block-styled-listitem-expected.txt:60
> +After hitting second Enter:

Ditto about Enter.  Please at least be consistent.

> LayoutTests/editing/execCommand/hit-enter-twice-atendof-block-styled-listitem.html:20
> +        Markup.description('This test verify When hitting "Enter" twice at the end of last listitem styled with display:block/float does not cause complete list to be removed.\nExpected behavior is list should not be removed and one extra empty line should get added.');

Typo: This test *verifies* inserting two paragraphs… does not cause the entire list to be removed.
Either "verifies that the entire list is not removed when ~" or "verifies inserting paragraphs twice… does not remove the entire list".
Otherwise, the sentence doesn't make much sense.

[Santosh Mahto]

Created attachment 215517 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 215517 [details]
Patch

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

> Source/WebCore/ChangeLog:19
> +        When listitems are styled with display:block/float then inserting paragraph twice at end
> +        of listitem delete entire list. Generally when listitem is empty then we
> +        delete the listitem on inserting paragraph. In this issue, on
> +        inserting first paragraph one empty listitem is created, and on inserting
> +        second paragraph we try to delete that empty listitem. but it misbehave becasue
> +        of incomplete definition of htmlediting::isLisItem() and entire list is deleted.
> +
> +        htmlediting::isListItem() check only render object to decide whether
> +        it is list or not, so if any LI element is block level then isListItem
> +        return false.
> +        Now after this patch if parent of current node is list element then node is
> +        treated as listItem.

Could you reformat this change log so that each line end at a similar length / at a sensible location.
e.g. ending a line with "on" is awkward.

[Santosh Mahto]

Created attachment 215548 [details]
patch for landing

[WebKit Commit Bot]

Comment on attachment 215548 [details]
patch for landing

Clearing flags on attachment: 215548

Committed r158314: <http://trac.webkit.org/changeset/158314>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.