Bug 21712 – Indent on li creates new ol instead of merging with existing ol, numbering incorrect.

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

[Julie Parent]

Repro steps:
1. Type one<enter>two<enter>three<enter>four
2. Select all, click on ordered list
3. Select "two" and "three" and click indent, forming a sublist.
4. Select "three" and click outdent, joining three back with the outer list.
5. Select "three" and click indent, bringing three back into the sublist.

Result: Instead of add "there" into the ol that "two" is in, a new ol is created, so both "two" and "three" are numbered 1.
1. one
  1. two
  1. three
2. four

Expected result: "three" would be the second li in the same ol as "two"
Works as expected in IE, FF 3

Incorrect result also seen in Chrome.

Alternate repro steps:
1. Type one<enter>two<enter>three<enter>four
2. Select all, click on ordered list
3. Select "two"and click indent, forming a sublist.
4. Select "three" and click indent

Result: "three" and "two" are both numbered 1.

[Ryosuke Niwa]

Created attachment 31661 [details]
demonstrates the bug

[Ryosuke Niwa]

Created attachment 31668 [details]
produces unexpected behavior in Firefox

I'm currently making a patch for this bug.  And I'll be posting in the next half an hour or so but there is a slight compilation about this fix.  The attachment tests what happens if we indented a list item that is sandwiched between two nested lists.  While I expected the result to be one big nested list with three items, Firefox combined the indented item to the previous list, and left the third list alone; i.e. of the three lists produced by indentation, Firefox combined the first two.  For consistency, my patch would produce the exact result Firefox produces.  It is also the case that combining all lists requires much more code.

[Ojan Vafai]

Firefox's behavior in the second test case is clearly wrong. It would be best to try and fix both these cases in the same patch if possible. I'm not familiar with the indent code, but I imagine the more involved fix for the second patch would also fix the first patch.

You might also want to file the Firefox bug at bugzilla.mozilla.org.

[Ryosuke Niwa]

Created attachment 31700 [details]
The patch for IndentOutdentCommand with 10 tests

[Darin Adler]

Comment on attachment 31700 [details]
The patch for IndentOutdentCommand with 10 tests

Thanks for tackling this!

> +            Element* enclosingListItem = static_cast<Element*>(enclosingListChild(endOfCurrentParagraph.deepEquivalent().node()));

It's dangerous to do a typecast. You have to have some reason to know the type is correct. If enclosingListChild guarantees it returns an Element*, then your patch should change the function's return type. If it does not guarantee that, then you need a type check before casting to Element*. Or it's possible that enclosingListChild doesn't always return an Element*, but for some reason you know that in this case it does.

> +            Element* previousListItem = enclosingListItem->previousElementSibling();
> +            Element* nextListItem = enclosingListItem->nextElementSibling();

These element sibling functions will skip over intervening text nodes. That means there could be an arbitrary amount of text, perhaps multiple pages of text, between these list items. In that case, the behavior of this code seems incorrect; list items will hop across text when indenting. You should make a test case like this and I think then you'll see this code needs to be changed.

> +            QualifiedName listTag = listNode->tagQName();

It's more efficient to have this local be a const QualifiedName&.

> +                appendNode(listItem, (Element*)previousListItem);

The typecast here is currently unnecessary. Please remove it. But probably to fix the issues above you'll change the code further. Also, it should be static_cast, not a C++-style cast.

> +                newListNode = (Element*)previousListItem;

Ditto.

> +            }
> +            else if (nextListItem && nextListItem->hasTagName(listTag)) {

The else should be on the same line with the closing brace.

> +                insertNodeAt(listItem, Position((Element*)nextListItem, 0));

Unnecessary typecast.

> +                newListNode = (Element*)nextListItem;

Ditto.

> +            }
> +            else {

The else should be on the same line with the closing brace.

I'm also not entirely certain we want this editing behavior in all contexts. I guess we probably do. Someone here at Apple should check to make sure we want this behavior in Apple's Mail application.

review- because of the issues mentioned above

[Ryosuke Niwa]

(In reply to comment #5)
> (From update of attachment 31700 [details] [review])
> Thanks for tackling this!

You're welcome

> > +            Element* enclosingListItem = static_cast<Element*>(enclosingListChild(endOfCurrentParagraph.deepEquivalent().node()));
> 
> It's dangerous to do a typecast. You have to have some reason to know the type
> is correct. If enclosingListChild guarantees it returns an Element*, then your
> patch should change the function's return type. If it does not guarantee that,
> then you need a type check before casting to Element*. Or it's possible that
> enclosingListChild doesn't always return an Element*, but for some reason you
> know that in this case it does.

Yes, enclosingListChild always returns a pointer to a HTMLElement.
I'll change the prototype & def. of the function.


> > +            Element* previousListItem = enclosingListItem->previousElementSibling();
> > +            Element* nextListItem = enclosingListItem->nextElementSibling();
> 
> These element sibling functions will skip over intervening text nodes. That
> means there could be an arbitrary amount of text, perhaps multiple pages of
> text, between these list items. In that case, the behavior of this code seems
> incorrect; list items will hop across text when indenting. You should make a
> test case like this and I think then you'll see this code needs to be changed.

Yes, you're right.  But I noticed that the nextSibling of the current list item is a text node,
and I need to somehow skip empty text nodes around it.
Since LI node is always in a list node, I thought it's safe to assume that there is no text in the same level.
But if you have a better of dealing (skipping empty text nodes), please let me know.

> > +            QualifiedName listTag = listNode->tagQName();
> 
> It's more efficient to have this local be a const QualifiedName&.

Thank you to point that out.
Unnecessary Element* casts are removed & coding style is enforced (as far as I concerned).

> I'm also not entirely certain we want this editing behavior in all contexts. I
> guess we probably do. Someone here at Apple should check to make sure we want
> this behavior in Apple's Mail application.

I'll really appreciate if you can get consensus ASAP.

[Ryosuke Niwa]

Created attachment 31878 [details]
fixes the problem with indenting nested lists

This patch modifies the behavior of WebKit when indenting lists so as to merge nested lists together when they're immediately next to each other.  This patch also fixes the bug that when indenting a list, the id of a parent list is copied to the nested list.

[Eric Seidel (no email)]

Comment on attachment 31878 [details]
fixes the problem with indenting nested lists

too complicated.  We talked in person.  There has to be a simpler solution to this.  Have julie look at this with you.  Style violations in the patch as well.

[Ryosuke Niwa]

See also https://bugs.webkit.org/show_bug.cgi?id=26762

[Ryosuke Niwa]

This bug has been solved almost completely in the changeset 45316.
http://trac.webkit.org/changeset/45316

There are some edge cases in which WebKit behaves inappropriately but I suggest to file new bugs for those.

[Julie Parent]

The original bug is fixed, but I'm filing a follow up on selection behavior issues.

[Julie Parent]

*** Bug 24865 has been marked as a duplicate of this bug. ***

[Julie Parent]

*** Bug 18105 has been marked as a duplicate of this bug. ***

[Julie Parent]

*** Bug 18023 has been marked as a duplicate of this bug. ***