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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Jun 24 01:58:45 PDT 2009


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





------- Comment #6 from rniwa at google.com  2009-06-24 01:58 PDT -------
(In reply to comment #5)
> (From update of attachment 31700 [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.


-- 
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.



More information about the webkit-unassigned mailing list