[Webkit-unassigned] [Bug 21712] Indent on li creates new ol instead of merging with existing ol, numbering incorrect.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=21712


darin at apple.com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #31700|review?                     |review-
               Flag|                            |




------- Comment #5 from darin at apple.com  2009-06-23 10:33 PDT -------
(From update of attachment 31700)
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


-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.