Handle special case of merging lists in mergeParagraphs()

Handle special case of merging lists in mergeParagraphs()

Consider a html content with two consecutive lists. When cursor is placed
at the end of first list and forward delete is executed, we should
merge the second list with the first one.

Currently, the content of the first <li> of second list is appended to
the content of last <li> of first list and two lists are still retained.
This is incorrect. Uploaded patch fixes this erroneous behavior.

Patch introduces the behavior that aligns with that of both FF and IE.

BUG=327624
TEST=LayoutTests/editing/deleting/merge-lists.html

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=165218

[vanihegde, 2013-12-11 11:52:42]

Please have a look. Thanks!

[yosin_UTC9, 2013-12-12 04:20:44]

https://codereview.chromium.org/112633003/diff/1/Source/core/editing/DeleteSe...
File Source/core/editing/DeleteSelectionCommand.cpp (right):

https://codereview.chromium.org/112633003/diff/1/Source/core/editing/DeleteSe...
Source/core/editing/DeleteSelectionCommand.cpp:643: Node*
listItemInFirstParagraph = enclosingNodeOfType(m_upstreamStart, &isListItem);
nit: You don't need to have "&" for isListItem.

https://codereview.chromium.org/112633003/diff/1/Source/core/editing/DeleteSe...
Source/core/editing/DeleteSelectionCommand.cpp:644: Node*
listItemInSecondParagraph = enclosingNodeOfType(m_downstreamEnd, &isListItem);
nit: You don't need to have "&" for isListItem.

https://codereview.chromium.org/112633003/diff/1/Source/core/editing/DeleteSe...
Source/core/editing/DeleteSelectionCommand.cpp:645: if (listItemInFirstParagraph
&& listItemInSecondParagraph
ni: Can we merge if-conditions of L645 and L647?

[vanihegde, 2013-12-12 07:44:14]

Uploaded patch with nits fixed.

[vanihegde, 2013-12-12 08:29:46]

We can as well git rid of the check (probably you meant the same in your
comment)
listItemInFirstParagraph->parentElement() !=
listItemInSecondParagraph->parentElement()

This will be taken care in canMergeLists where we check isVisiblyAdjacent.
If both are same lists, isVisiblyAdjacent would fail.

[leviw_travelin_and_unemployed, 2013-12-18 19:54:34]

https://codereview.chromium.org/112633003/diff/40001/LayoutTests/editing/dele...
File LayoutTests/editing/deleting/merge-lists-expected.txt (right):

https://codereview.chromium.org/112633003/diff/40001/LayoutTests/editing/dele...
LayoutTests/editing/deleting/merge-lists-expected.txt:13: |    
"<#selection-caret>four"
Everything about this patch looks good except the final selection. I wouldn't
expect the caret to be before the "four" if I hit forward-delete from "three."

[vanihegde, 2013-12-19 05:15:04]

On 2013/12/18 19:54:34, Levi wrote:
>
https://codereview.chromium.org/112633003/diff/40001/LayoutTests/editing/dele...
> File LayoutTests/editing/deleting/merge-lists-expected.txt (right):
> 
>
https://codereview.chromium.org/112633003/diff/40001/LayoutTests/editing/dele...
> LayoutTests/editing/deleting/merge-lists-expected.txt:13: |    
> "<#selection-caret>four"
> Everything about this patch looks good except the final selection. I wouldn't
> expect the caret to be before the "four" if I hit forward-delete from "three."


Probably I need to handle this in mergeIdenticalElements().
I will look into it.
FYI, FF also has the same behavior i.e. caret before the "four".

[vanihegde, 2014-01-15 09:21:41]

The new patch addresses your comment, but I have come across another issue.

What do you think the behavior should be when caret is placed before
'four'(first item in second list) and backspace is hit?

Behavior in IE and FF:
Both break out of the list. Should we align our behavior accordingly?
If yes, I think we should address this issue separately.

Current behavior in chrome:
The content of the first <li> of second list is appended to 
the content of last <li> of first list and two lists are still retained.

Behavior with this patch:
Two lists are merged, and the caret is after 'three' (May be the caret should
be before 'four' in this case?).
If we intend to align our behavior with that of IE and FF, this can be addressed
while fixing the same. So once that is done, we should hit this newly added code
only when 'forward delete' is executed to merge the lists.

Let me know your thoughts on this.

[yosin_UTC9, 2014-01-15 09:27:27]

On 2014/01/15 09:21:41, vanihegde wrote:
> The new patch addresses your comment, but I have come across another issue.
> 
> What do you think the behavior should be when caret is placed before
> 'four'(first item in second list) and backspace is hit?
> 
> Behavior in IE and FF:
> Both break out of the list. Should we align our behavior accordingly?
> If yes, I think we should address this issue separately.
> 
> Current behavior in chrome:
> The content of the first <li> of second list is appended to 
> the content of last <li> of first list and two lists are still retained.
> 
> Behavior with this patch:
> Two lists are merged, and the caret is after 'three' (May be the caret should
> be before 'four' in this case?).
> If we intend to align our behavior with that of IE and FF, this can be
addressed
> while fixing the same. So once that is done, we should hit this newly added
code
> only when 'forward delete' is executed to merge the lists.
> 
> Let me know your thoughts on this.

Let's follow IE and FF's. We don't want to make Blink only behavior for this.

[vanihegde, 2014-01-15 09:37:44]

In that case, can we conclude on this patch and take up the other issue
separately?

[leviw_travelin_and_unemployed, 2014-01-15 17:58:42]

lgtm

[commit-bot: I haz the power, 2014-01-16 06:32:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vani.hegde@samsung.com/112633003/60001

[commit-bot: I haz the power, 2014-01-16 07:23:37]

Retried try job too often on linux_blink for step(s) wtf_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...

[commit-bot: I haz the power, 2014-01-16 09:36:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vani.hegde@samsung.com/112633003/60001

[commit-bot: I haz the power, 2014-01-16 10:11:50]

Retried try job too often on linux_blink for step(s) wtf_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...

[commit-bot: I haz the power, 2014-01-16 12:38:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vani.hegde@samsung.com/112633003/60001

[commit-bot: I haz the power, 2014-01-16 13:10:36]

Change committed as 165218