Bug 35281 – when deleting lines between lists, we should try to merge the lists

• Component: HTML Editing
• Status: NEW

[Tony Chang]

For example:
  *  one
  *  two
  *  ^
  *  four

If the cursor is at ^, pressing enter breaks out of the list and now we have two lists.
  * one
  * two
^
  * four

Pressing backspace next should merge two lists.  Instead we end up with two separate lists with a visible space between the two.

[Tony Chang]

Created attachment 49262 [details]
Patch

[Tony Chang]

Adding Ryosuke, master of editable lists.

[Tony Chang]

(In reply to comment #1)
> Created an attachment (id=49262) [details]
> Patch

Oh, I'm not sure this is the best place to put the merge code.  I originally tried to put it in the DeleteSelection code, but DeleteSelection gets triggered by lots of places and this caused the calls to canMergeLists in IndentOutdentCommand to break.  I was able to work around this with enough code tweaks, but it seemed like the DeleteSelection code is used in too many places for me to change the behavior safely.

[Ryosuke Niwa]

(In reply to comment #3)
> (In reply to comment #1)
> > Created an attachment (id=49262) [details] [details]
> > Patch
> 
> Oh, I'm not sure this is the best place to put the merge code.  I originally
> tried to put it in the DeleteSelection code, but DeleteSelection gets triggered
> by lots of places and this caused the calls to canMergeLists in
> IndentOutdentCommand to break.  I was able to work around this with enough code
> tweaks, but it seemed like the DeleteSelection code is used in too many places
> for me to change the behavior safely.

You could not modify IndentOutdentCommand instead?  Is TypingCommand (guessing from the name) triggered from execCommand or events caused by mice?

[Tony Chang]

Created attachment 50193 [details]
Patch

[Tony Chang]

(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #1)
> > > Created an attachment (id=49262) [details] [details] [details]
> > > Patch
> > 
> > Oh, I'm not sure this is the best place to put the merge code.  I originally
> > tried to put it in the DeleteSelection code, but DeleteSelection gets triggered
> > by lots of places and this caused the calls to canMergeLists in
> > IndentOutdentCommand to break.  I was able to work around this with enough code
> > tweaks, but it seemed like the DeleteSelection code is used in too many places
> > for me to change the behavior safely.
> 
> You could not modify IndentOutdentCommand instead?

I've attached a separate patch which does the merge in DeleteSelection.  One test still fails, but if you think that approach is more appropriate, I can try to fix the test and upload a patch for review.

> Is TypingCommand (guessing
> from the name) triggered from execCommand or events caused by mice?

It is triggered by execCommand (see the test) but is not triggered by a cut event.  I'm not sure of any other way to delete a line using the mouse.

[Ryosuke Niwa]

Comment on attachment 50193 [details]
Patch

> -        if (node->hasTagName(brTag))
> +        if (node->hasTagName(brTag)) {
> +            VisiblePosition beforePlaceholder = caretAfterDelete.previous(true);
> +            VisiblePosition afterPlaceholder = caretAfterDelete.next(true);
>              removeNodeAndPruneAncestors(node);
> +            HTMLElement* before = enclosingList(beforePlaceholder.deepEquivalent().node());
> +            HTMLElement* after = enclosingList(afterPlaceholder.deepEquivalent().node());
> +            if (canMergeLists(before, after))
> +                mergeIdenticalElements(before, after);
> +        }

This change alone seem to pass your test case.
Why are the other changes also included in this patch?
i.e. Why do we need other changes?
If each other of other changes address other bugs / cases,
then we should also test those as well.

[Tony Chang]

(In reply to comment #7)
> This change alone seem to pass your test case.
> Why are the other changes also included in this patch?
> i.e. Why do we need other changes?
> If each other of other changes address other bugs / cases,
> then we should also test those as well.

The other changes are to avoid regressing other test cases.  Because we merge the nodes earlier, the DOM structure isn't what we expect in InsertListCommand and IndentOutdentCommand.

[Ryosuke Niwa]

(In reply to comment #8)
> The other changes are to avoid regressing other test cases.  Because we merge
> the nodes earlier, the DOM structure isn't what we expect in InsertListCommand
> and IndentOutdentCommand.

I see. Now I see a bunch of tests failing without your other changes. I think I understand most of your fixes but I'm not sure about the last one in TypingCommand.cpp. As far as I analyzed the problem lies in MoveParagraph called in the second block of InsertListCommand::doApply().  MoveParagraph is called to move the contents of ol into the new LI element created but it leaves ol behind.

In particular,

		UL	0x11990a6c0
			LI	0x11a725660
				#text	0x11a726560 "One"
			LI	0x1053e9280
				BR	0x11a7271f0
*		BR	0x11a7264a0
		OL	0x11990aad0
			UL	0x1068d6ec0
				LI	0x1068ee590
					#text	0x1068e4460 "Three"
				LI	0x11a00b7f0
					#text	0x1068e61d0 "Four"

becomes

				UL	0x1068d6ec0
					LI	0x11a725660
						#text	0x11a726560 "One"
					LI	0x1053e9280
						BR	0x11a7271f0
					LI	0x1068ee590
						#text	0x1068e4460 "Three"
					LI	0x11a00b7f0
						#text	0x1068e61d0 "Four"
				OL	0x11990aad0

due to the code you added in cleanupAfterDeletion(). This is a due to a bug in canMergeLists(). We must ensure that the two litss have the same enclosing list when merging. Here, we're merging un inside of ol and ul even though they belong to the to different levels.  With the following canMergeLists,

bool canMergeLists(Element* firstList, Element* secondList)
{
    if (!firstList || !secondList)
        return false;

    return firstList->hasTagName(secondList->tagQName())// make sure the list types match (ol vs. ul)
    && firstList->isContentEditable() && secondList->isContentEditable()// both lists are editable
    && firstList->rootEditableElement() == secondList->rootEditableElement()// don't cross editing boundaries
    && enclosingList(firstList) == enclosingList(secondList)
    && isVisiblyAdjacent(positionInParentAfterNode(firstList), positionInParentBeforeNode(secondList));
    // Make sure there is no visible content between this li and the previous list
}

TypingCommand does not need to be changed.

[Ryosuke Niwa]

Created attachment 51136 [details]
fixes canMergeLists

I attach my svn diff. It passes all editing layout tests locally.

[Tony Chang]

Comment on attachment 49262 [details]
Patch

Ok, then let's go with the latter approach.  I will try to fix up the patch on Monday (JST).

[Ryosuke Niwa]

(In reply to comment #11)
> (From update of attachment 49262 [details])
> Ok, then let's go with the latter approach.  I will try to fix up the patch on
> Monday (JST).

My patch for the bug 19539 contains the identical change to InsertListCommand::doApply: nextList && previousList > canMergeLists(nextList, previousList).  It might make sense to land that patch first because the bug 19539 address the problem in InsertListCommand more directly.

[Tony Chang]

Created attachment 51385 [details]
Patch

[Tony Chang]

(In reply to comment #13)
> Created an attachment (id=51385) [details]
> Patch

Updated patch with the fix from Ryosuke.  No commit-queue because as mentioned in comment 12, there's overlap with bug 19539.

[Tony Chang]

Created attachment 53932 [details]
Patch

[Tony Chang]

(In reply to comment #15)
> Created an attachment (id=53932) [details]
> Patch

Rev'ed the patch now that 19539 has landed.

[Ryosuke Niwa]

(In reply to comment #16)
> (In reply to comment #15)
> > Created an attachment (id=53932) [details] [details]
> > Patch
> 
> Rev'ed the patch now that 19539 has landed.

Ah... thanks for fixing my mistakes.  I should have paid more attentions.

[Adele Peterson]

Comment on attachment 53932 [details]
Patch

The tests aren't included in this patch.  I actually couldn't see a visible space when trying to test this manually in TOT, so I'd like to try out the test case to see the behavior difference.

[Tony Chang]

Created attachment 54248 [details]
Patch

[Tony Chang]

(In reply to comment #18)
> (From update of attachment 53932 [details])
> The tests aren't included in this patch.  I actually couldn't see a visible
> space when trying to test this manually in TOT, so I'd like to try out the test
> case to see the behavior difference.

Sorry, new patch up with layout tests.

[Darin Adler]

Comment on attachment 54248 [details]
Patch

I see five distinct code changes here and only one test case. Does this test case cover all five changes? In other words, if I take out each one of the five changes, is there a test that will fail? If not, I'd like to see additional test cases.

[Tony Chang]

(In reply to comment #21)
> (From update of attachment 54248 [details])
> I see five distinct code changes here and only one test case. Does this test
> case cover all five changes? In other words, if I take out each one of the five
> changes, is there a test that will fail? If not, I'd like to see additional
> test cases.

If you take out any one of the five changes either an existing test will fail or the new test will fail.

[Adam Barth]

Ojan or Enrica: Interested in reviewing this patch?

[Enrica Casucci]

Comment on attachment 54248 [details]
Patch

> diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
> index 51a4e50..6605887 100644
> --- a/LayoutTests/ChangeLog
> +++ b/LayoutTests/ChangeLog
> @@ -1,3 +1,13 @@
> [phone]  Tony Chang  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        when deleting lines between lists, we should try to merge the lists
> +        https://bugs.webkit.org/show_bug.cgi?id=35281
> +
> +        * editing/deleting/merge-lists-expected.txt: Added.
> +        * editing/deleting/merge-lists.html: Added.
> +
>  2010-04-25  Andrey Kosyakov  <[e-mail]>
>  
>          Reviewed by Adam Barth.
> diff --git a/LayoutTests/editing/deleting/merge-lists-expected.txt b/LayoutTests/editing/deleting/merge-lists-expected.txt
> new file mode 100644
> index 0000000..1754250
> --- /dev/null
> +++ b/LayoutTests/editing/deleting/merge-lists-expected.txt
> @@ -0,0 +1,9 @@
> +While on an empty list item, pressing enter should break out of the list and pressing delete should then merge the two lists together. There should be a single list below with 3 list items.
> +one
> +two
> +three
> +PASS: 
> +<li>one</li>
> +<li>two</li>
> +<li>three</li>
> +
> diff --git a/LayoutTests/editing/deleting/merge-lists.html b/LayoutTests/editing/deleting/merge-lists.html
> new file mode 100644
> index 0000000..b9d6274
> --- /dev/null
> +++ b/LayoutTests/editing/deleting/merge-lists.html
> @@ -0,0 +1,23 @@
> +<body contentEditable="true">
> +<div>While on an empty list item, pressing enter should break out of the list and pressing delete
> +should then merge the two lists together.  There should be a single list below with 3 list items.
> +<ul id="ul">
> +  <li>one</li>
> +  <li>two</li>
> +  <li id="test"></li>
> +  <li>three</li>
> +</ul>
> +<div id="results">FAIL</div>
> +<script src="../editing.js"></script>
> +<script>
> +function editingTest()
> +{
> +    execTypeCharacterCommand("\n");
> +    execDeleteCommand();
> +    if (document.getElementsByTagName("ul").length != 1)
> +        throw "Wrong number of lists."
> +    document.getElementById("results").innerText = "PASS: " + document.getElementById("ul").innerHTML;
> +}
> +
> +runDumpAsTextEditingTest(false);
> +</script>
> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> index e329fc2..f11f803 100644
> --- a/WebCore/ChangeLog
> +++ b/WebCore/ChangeLog
> @@ -1,3 +1,21 @@
> [phone]  Tony Chang  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        when deleting lines between lists, we should try to merge the lists
> +        https://bugs.webkit.org/show_bug.cgi?id=35281
> +
> +        Test: editing/deleting/merge-lists.html
> +
> +        * editing/CompositeEditCommand.cpp:
> +        (WebCore::CompositeEditCommand::cleanupAfterDeletion): try to merge adjacent lists after a deletion
> +        * editing/IndentOutdentCommand.cpp:
> +        (WebCore::IndentOutdentCommand::tryIndentingAsListItem):
> +        * editing/InsertListCommand.cpp:
> +        (WebCore::InsertListCommand::doApply):
> +        * editing/htmlediting.cpp:
> +        (WebCore::canMergeLists):
> +
>  2010-04-25  Andrey Kosyakov  <[e-mail]>
>  
>          Reviewed by Adam Barth.
> diff --git a/WebCore/editing/CompositeEditCommand.cpp b/WebCore/editing/CompositeEditCommand.cpp
> index 9dc918d..6811563 100644
> --- a/WebCore/editing/CompositeEditCommand.cpp
> +++ b/WebCore/editing/CompositeEditCommand.cpp
> @@ -807,8 +807,15 @@ void CompositeEditCommand::cleanupAfterDeletion()
>          Position position = caretAfterDelete.deepEquivalent().downstream();
>          Node* node = position.node();
>          // Normally deletion will leave a br as a placeholder.
> -        if (node->hasTagName(brTag))
> +        if (node->hasTagName(brTag)) {
> +            VisiblePosition beforePlaceholder = caretAfterDelete.previous(true);
> +            VisiblePosition afterPlaceholder = caretAfterDelete.next(true);
>              removeNodeAndPruneAncestors(node);
> +            HTMLElement* before = enclosingList(beforePlaceholder.deepEquivalent().node());
> +            HTMLElement* after = enclosingList(afterPlaceholder.deepEquivalent().node());
> +            if (canMergeLists(before, after))
> +                mergeIdenticalElements(before, after);
> +        }
>          // If the selection to move was empty and in an empty block that 
>          // doesn't require a placeholder to prop itself open (like a bordered
>          // div or an li), remove it during the move (the list removal code
> diff --git a/WebCore/editing/IndentOutdentCommand.cpp b/WebCore/editing/IndentOutdentCommand.cpp
> index 0f3975b..4e88cd2 100644
> --- a/WebCore/editing/IndentOutdentCommand.cpp
> +++ b/WebCore/editing/IndentOutdentCommand.cpp
> @@ -99,6 +99,8 @@ bool IndentOutdentCommand::tryIndentingAsListItem(const VisiblePosition& endOfCu
>          mergeIdenticalElements(previousList, newList);
>      if (canMergeLists(newList.get(), nextList))
>          mergeIdenticalElements(newList, nextList);
> +    if (canMergeLists(previousList, nextList))
> +        mergeIdenticalElements(previousList, nextList);
>  
>      return true;
>  }
> diff --git a/WebCore/editing/InsertListCommand.cpp b/WebCore/editing/InsertListCommand.cpp
> index cd6838b..2f3f638 100644
> --- a/WebCore/editing/InsertListCommand.cpp
> +++ b/WebCore/editing/InsertListCommand.cpp
> @@ -136,7 +136,7 @@ void InsertListCommand::doApply()
>      Node* listChildNode = enclosingListChild(selectionNode);
>      bool switchListType = false;
>      if (listChildNode) {
> -        // Remove the list chlild.
> +        // Remove the list child.
>          HTMLElement* listNode = enclosingList(listChildNode);
>          if (!listNode)
>              listNode = fixOrphanedListChild(listChildNode);
> @@ -263,12 +263,12 @@ void InsertListCommand::doApply()
>              nextList = outermostEnclosingList(nextPosition.deepEquivalent().node(), enclosingList(listElement.get()));
>          }
>          moveParagraph(start, end, VisiblePosition(Position(placeholder.get(), 0)), true);
> -        if (m_listElement) {
> +        if (m_listElement && m_listElement->inDocument()) {
>              if (canMergeLists(previousList, m_listElement.get()))
>                  mergeIdenticalElements(previousList, m_listElement.get());
>              if (canMergeLists(m_listElement.get(), nextList))
>                  mergeIdenticalElements(m_listElement.get(), nextList);
> -        } else if (canMergeLists(nextList, previousList))
> +        } else if (canMergeLists(previousList, nextList))
>              mergeIdenticalElements(previousList, nextList);
>      }
>  }
> diff --git a/WebCore/editing/htmlediting.cpp b/WebCore/editing/htmlediting.cpp
> index b7a062b..bfbf929 100644
> --- a/WebCore/editing/htmlediting.cpp
> +++ b/WebCore/editing/htmlediting.cpp
> @@ -835,6 +835,7 @@ bool canMergeLists(Element* firstList, Element* secondList)
>      return firstList->hasTagName(secondList->tagQName())// make sure the list types match (ol vs. ul)
>      && firstList->isContentEditable() && secondList->isContentEditable()// both lists are editable
>      && firstList->rootEditableElement() == secondList->rootEditableElement()// don't cross editing boundaries
> +    && enclosingList(firstList) == enclosingList(secondList)
>      && isVisiblyAdjacent(positionInParentAfterNode(firstList), positionInParentBeforeNode(secondList));
>      // Make sure there is no visible content between this li and the previous list
>  }

This makes sense to me.

[Ryosuke Niwa]

Comment on attachment 54248 [details]
Patch

.> diff --git a/LayoutTests/editing/deleting/merge-lists-expected.txt b/LayoutTests/editing/deleting/merge-lists-expected.txt
> new file mode 100644
> index 0000000..1754250
> --- /dev/null
> +++ b/LayoutTests/editing/deleting/merge-lists-expected.txt
> @@ -0,0 +1,9 @@
> +While on an empty list item, pressing enter should break out of the list and pressing delete should then merge the two lists together. There should be a single list below with 3 list items.
> +one
> +two
> +three
> +PASS: 
> +<li>one</li>
> +<li>two</li>
> +<li>three</li>
> +
>

I would like to see the intermediate HTML after you broke out of the list because if that didn't happen, we won't be testing what we claim to test.  Or you can start with the HTML after you broke out of the test.  But I prefer adding one more dump for the better test coverage.

> --- a/WebCore/editing/CompositeEditCommand.cpp
> +++ b/WebCore/editing/CompositeEditCommand.cpp
> @@ -807,8 +807,15 @@ void CompositeEditCommand::cleanupAfterDeletion()
>          Position position = caretAfterDelete.deepEquivalent().downstream();
>          Node* node = position.node();
>          // Normally deletion will leave a br as a placeholder.
> -        if (node->hasTagName(brTag))
> +        if (node->hasTagName(brTag)) {
> +            VisiblePosition beforePlaceholder = caretAfterDelete.previous(true);
> +            VisiblePosition afterPlaceholder = caretAfterDelete.next(true);
>              removeNodeAndPruneAncestors(node);
> +            HTMLElement* before = enclosingList(beforePlaceholder.deepEquivalent().node());
> +            HTMLElement* after = enclosingList(afterPlaceholder.deepEquivalent().node());

I'm skeptical about this enclosingList business here. What if the list list of the previous or the next lists were inner lists?
For example, if we had <ul><ul><li>hello</li></ul><li></li> and then did what the reproduction steps describe here, won't be obtaining the inner list for before and fail to merge?
My patch for https://bugs.webkit.org/show_bug.cgi?id=33668 fixes this problem in InsertListCommand

> +            if (canMergeLists(before, after))
> +                mergeIdenticalElements(before, after);
> +        }

LGTM.

> index 0f3975b..4e88cd2 100644
> --- a/WebCore/editing/IndentOutdentCommand.cpp
> +++ b/WebCore/editing/IndentOutdentCommand.cpp
> @@ -99,6 +99,8 @@ bool IndentOutdentCommand::tryIndentingAsListItem(const VisiblePosition& endOfCu
>          mergeIdenticalElements(previousList, newList);
>      if (canMergeLists(newList.get(), nextList))
>          mergeIdenticalElements(newList, nextList);
> +    if (canMergeLists(previousList, nextList))
> +        mergeIdenticalElements(previousList, nextList);

Are you sure this code path is ever executed?  The only case this will be executed is if both previousList and nextList can be merged with newList.  But then the first if statement will merge previousList and newList and leaves newList.  We then merge newList and nextList. So the code should work as is unless I'm missing something.


> diff --git a/WebCore/editing/InsertListCommand.cpp b/WebCore/editing/InsertListCommand.cpp
> index cd6838b..2f3f638 100644
> --- a/WebCore/editing/InsertListCommand.cpp
> +++ b/WebCore/editing/InsertListCommand.cpp
> @@ -263,12 +263,12 @@ void InsertListCommand::doApply()
>              nextList = outermostEnclosingList(nextPosition.deepEquivalent().node(), enclosingList(listElement.get()));
>          }
>          moveParagraph(start, end, VisiblePosition(Position(placeholder.get(), 0)), true);
> -        if (m_listElement) {
> +        if (m_listElement && m_listElement->inDocument()) {

I suppose JavaScript can modify the DOM but shouldn't the canMergeLists be harmless in that case?

>              if (canMergeLists(previousList, m_listElement.get()))
>                  mergeIdenticalElements(previousList, m_listElement.get());
>              if (canMergeLists(m_listElement.get(), nextList))
>                  mergeIdenticalElements(m_listElement.get(), nextList);
> -        } else if (canMergeLists(nextList, previousList))
> +        } else if (canMergeLists(previousList, nextList))

Oops, my bad.

> --- a/WebCore/editing/htmlediting.cpp
> +++ b/WebCore/editing/htmlediting.cpp
> @@ -835,6 +835,7 @@ bool canMergeLists(Element* firstList, Element* secondList)
>      return firstList->hasTagName(secondList->tagQName())// make sure the list types match (ol vs. ul)
>      && firstList->isContentEditable() && secondList->isContentEditable()// both lists are editable
>      && firstList->rootEditableElement() == secondList->rootEditableElement()// don't cross editing boundaries
> +    && enclosingList(firstList) == enclosingList(secondList)
>      && isVisiblyAdjacent(positionInParentAfterNode(firstList), positionInParentBeforeNode(secondList));
>      // Make sure there is no visible content between this li and the previous list
>  }

Good change.

[Tony Chang]

Comment on attachment 54248 [details]
Patch

Sounds like this is conflicting with some patches rniwa has in flight.  I will revisit after his patch for issue 33668 lands.

[Tony Chang]

Created attachment 69529 [details]
Patch

[Tony Chang]

The newest patch removes the changes except for the list merging.  I also updated MergeIdenticalElementsCommand to support merging into the first element.  Without this, we would break during moveParagraphsWithClones which expects the new list to exist after calling cleanupAfterDeletion().

I updated the test to show the intermediate HTML.  

"""I'm skeptical about this enclosingList business here. What if the list list of the previous or the next lists were inner lists?
For example, if we had <ul><ul><li>hello</li></ul><li></li> and then did what the reproduction steps describe here, won't be obtaining the inner list for before and fail to merge?"""

If this is the editable text:

* * Hello
* ^

I'm not sure what we're merging here.  I added a test for:

* * Hello
* ^
* * World

Where pressing backspace should merge into a single double nested list.  If the lists are at different depths, I don't think they should merge.

[Ryosuke Niwa]

Comment on attachment 69529 [details]
Patch

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

> WebCore/editing/CompositeEditCommand.h:46
> +    enum MergeDirection {
> +        MergeIntoFirst,
> +        MergeIntoSecond,
> +    };
> +

I'm not sure if MergeDirection is the right name because direction just sounds like it's related to LTR/RTL.  But I can't think of a better one.  Maybe ElementToMergeInto?

> WebCore/editing/MergeIdenticalElementsCommand.cpp:84
> +    if (m_direction == MergeIntoFirst)
> +        insertNodeAfter(m_element2.get(), m_element1.get());

It seems like this is the only reason we're making it inherit from CompositeEditCommand?  We should just call appendChild on its parent when it's at the end, and call insertBefore otherwise.

> WebCore/editing/MergeIdenticalElementsCommand.h:33
> -class MergeIdenticalElementsCommand : public SimpleEditCommand {
> +class MergeIdenticalElementsCommand : public CompositeEditCommand {

I don't think we want to make this command CompositeEditCommand.  If we're absolutely required to do this, then we should remove doUnapply and doReapply.

[Ryosuke Niwa]

(In reply to comment #28)
> If this is the editable text:
> 
> * * Hello
> * ^
> 
> I'm not sure what we're merging here.  I added a test for:
> 
> * * Hello
> * ^
> * * World
> 
> Where pressing backspace should merge into a single double nested list.  If the lists are at different depths, I don't think they should merge.

So you get <ul><ul><li>hello</li><li>world</li></ul></ul>, right?  That sounds like a correct behavior to me.  Sorry my grammar was really bad in the earlier comment but what I intended to say was that enclosingList incorrectly obtains the inner most list as supposed to the list of the appropriate level.  For example, if we had:

> * * Hello
> ^
> * World

We should be getting

> * * Hello
> * World

Where * Hello and World belongs to the same list: <ul><ul><li>Hello</li></ul><li>World</li></ul>.  With your original patch, I think enclosingList would have grabbed ul wrapping "Hello" and failed to merge the outer list, resulting in <ul><ul><li>Hello</li></ul></ul><ul><li>World</li></ul>.

[Tony Chang]

Created attachment 69543 [details]
Patch

[Tony Chang]

(In reply to comment #29)
> I'm not sure if MergeDirection is the right name because direction just sounds like it's related to LTR/RTL.  But I can't think of a better one.  Maybe ElementToMergeInto?

Renamed to MergeIdenticalElementsDirection.

> > WebCore/editing/MergeIdenticalElementsCommand.cpp:84
> > +    if (m_direction == MergeIntoFirst)
> > +        insertNodeAfter(m_element2.get(), m_element1.get());
> 
> It seems like this is the only reason we're making it inherit from CompositeEditCommand?  We should just call appendChild on its parent when it's at the end, and call insertBefore otherwise.

Ok, restored it as a SimpleEditCommand.

> For example, if we had:
> 
> > * * Hello
> > ^
> > * World
> 
> We should be getting
> 
> > * * Hello
> > * World
> 
> Where * Hello and World belongs to the same list: <ul><ul><li>Hello</li></ul><li>World</li></ul>.  With your original patch, I think enclosingList would have grabbed ul wrapping "Hello" and failed to merge the outer list, resulting in <ul><ul><li>Hello</li></ul></ul><ul><li>World</li></ul>.

With this patch, we still don't merge the outer lists.  I'm not 100% convinced we should merge in this case.  I tried it in IE9 and it produced:

* * HelloWorld

But that doesn't seem much better.  Anyway, I think this case should be a different bug/patch.

[Ryosuke Niwa]

(In reply to comment #32)
> Ok, restored it as a SimpleEditCommand.

Great!  Do we currently have a test for undoing/redoing MergeIdenticalElements?
If not, could you add one since you're making a lot of changes?

> > For example, if we had:
> > 
> > > * * Hello
> > > ^
> > > * World
> > 
> > We should be getting
> > 
> > > * * Hello
> > > * World
> > 
> > Where * Hello and World belongs to the same list: <ul><ul><li>Hello</li></ul><li>World</li></ul>.  With your original patch, I think enclosingList would have grabbed ul wrapping "Hello" and failed to merge the outer list, resulting in <ul><ul><li>Hello</li></ul></ul><ul><li>World</li></ul>.
> 
> With this patch, we still don't merge the outer lists.  I'm not 100% convinced we should merge in this case.  I tried it in IE9 and it produced.

Take a look at mergeWithNeighboringLists in InsertListCommand.cpp.  Using that function should solve this problem.

[Tony Chang]

(In reply to comment #33)
> (In reply to comment #32)
> > Ok, restored it as a SimpleEditCommand.
> 
> Great!  Do we currently have a test for undoing/redoing MergeIdenticalElements?

editing/undo/undo-indent.html covers this.

[Ryosuke Niwa]

Could someone review the patch Tony posted before it gets too old?

[Eric Seidel (no email)]

Comment on attachment 69543 [details]
Patch

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

I'm confused why direction is needed.

> WebCore/editing/CompositeEditCommand.cpp:807
> +            VisiblePosition beforePlaceholder = caretAfterDelete.previous(true);
> +            VisiblePosition afterPlaceholder = caretAfterDelete.next(true);
>              removeNodeAndPruneAncestors(node);
> +            HTMLElement* before = enclosingList(beforePlaceholder.deepEquivalent().node());
> +            HTMLElement* after = enclosingList(afterPlaceholder.deepEquivalent().node());

Do we need an enclosingList function which takes a visible position?  and/or a canMergeListsAt(visiblePosition) function?

> WebCore/editing/CompositeEditCommand.h:42
> +    enum MergeIdenticalElementsDirection {

Seems this should just be its own header since it's needed in SimpleEditComand.

> WebCore/editing/MergeIdenticalElementsCommand.h:50
> +    CompositeEditCommand::MergeIdenticalElementsDirection m_direction;

Seems this enum wants its own header...

[Tony Chang]

Created attachment 72127 [details]
Patch

[Tony Chang]

(In reply to comment #36)
> (From update of attachment 69543 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=69543&action=review
> 
> I'm confused why direction is needed.

Without this, we would break during moveParagraphsWithClones which expects the new list to exist after calling cleanupAfterDeletion().  The old behavior (MergeIntoSecond) is expected by some of the existing callers that try to keep a pointer to the second list.

> Do we need an enclosingList function which takes a visible position?  and/or a canMergeListsAt(visiblePosition) function?

Added.

> Seems this should just be its own header since it's needed in SimpleEditComand.
> Seems this enum wants its own header...

Done.

[Adam Barth]

(In reply to comment #35)
> Could someone review the patch Tony posted before it gets too old?

^^^ rniwa?

[Alexis Menard (darktears)]

Comment on attachment 72127 [details]
Patch

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

> LayoutTests/ChangeLog:5
> +        when deleting lines between lists, we should try to merge the lists

The subject should start with an upper case letter I believe.

> WebCore/ChangeLog:5
> +        when deleting lines between lists, we should try to merge the lists

Same.

> WebCore/editing/MergeIdenticalElementsDirection.h:2
> + * Copyright (C) 2010 Google Inc. All rights reserved.

2011?

[Eric Seidel (no email)]

Comment on attachment 72127 [details]
Patch

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

I like this!  I'm ready to r+ this if updated.

> WebCore/WebCore.xcodeproj/project.pbxproj:1303
> +		53E34AEC127907C20060B999 /* MergeIdenticalElementsDirection.h in Headers */ = {isa = PBXBuildFile; fileRef = 53E34AEB127907C20060B999 /* MergeIdenticalElementsDirection.h */; settings = {ATTRIBUTES = (Private, ); }; };

Why does this need to be private instead of project?

> WebCore/editing/MergeIdenticalElementsDirection.h:37
> +typedef enum {
> +    MergeIntoFirst,
> +    MergeIntoSecond,
> +} MergeIdenticalElementsDirection;

Is this modern syntax?

[Aryeh Gregor]

The current editing spec has this bug too, which I filed: http://www.w3.org/Bugs/Public/show_bug.cgi?id=13976

[Shezan Baig]

Is this worth pursuing?  It seems like it would remove some editing flexibility.  For example, it wouldn't allow lists to be siblings without an extra linebreak.  i.e, if I have:
<ul>
    <li>first</li>
    <li>second</li>
</ul>
<br/>[selection-caret]
<ul>
    <li>another first</li>
    <li>another second</li>
</ul>

If this bug is fixed, it would be impossible for an editor to get this markup:

<ul>
    <li>first</li>
    <li>second</li>
</ul>
<ul>
    <li>another first</li>
    <li>another second</li>
</ul>

Which may be desirable, since <ul>s have margin before and after.  Forcing them to have a <br/> introduces too much spacing between the lists.

I think the lists should only be merged if the user presses backspace at:

<ul>
    <li>first</li>
    <li>second</li>
</ul>
<ul>
    <li>[selection-caret]another first</li>
    <li>another second</li>
</ul>

Thoughts?