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.
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.
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?
Created attachment 215251 [details] under construction
Created attachment 215255 [details] Patch
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
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.
(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..
Any element inside ul/ol should be considered as a list item. At least that's what WebKit currently does.
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?
(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.
(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>
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.
Created attachment 215318 [details] Patch
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()).
Created attachment 215404 [details] Patch
(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
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.
Created attachment 215517 [details] Patch
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.
Created attachment 215548 [details] patch for landing
Comment on attachment 215548 [details] patch for landing Clearing flags on attachment: 215548 Committed r158314: <http://trac.webkit.org/changeset/158314>
All reviewed patches have been landed. Closing bug.